Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu.

[Michael S. Tsirkin]

On Mon, Mar 16, 2015 at 01:44:22PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> > On Fri, Mar 13, 2015 at 11:47:18AM +1030, Rusty Russell wrote:
> >> Here's my proposed spec patch, which spells this out:
> >> 
> >> diff --git a/content.tex b/content.tex
> >> index 6ba079d..b6345a8 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -600,10 +600,19 @@ them: it is only written to by the device, and read 
> >> by the driver.
> >>  Each entry in the ring is a pair: \field{id} indicates the head entry of 
> >> the
> >>  descriptor chain describing the buffer (this matches an entry
> >>  placed in the available ring by the guest earlier), and \field{len} the 
> >> total
> >> -of bytes written into the buffer. The latter is extremely useful
> >> +of bytes written into the buffer. 
> >> +
> >> +\begin{note}
> >> +\field{len} is extremely useful
> >
> > just "useful" maybe?
> 
> OK.
> 
> >>  for drivers using untrusted buffers: if you do not know exactly
> >
> > replace "you" with "driver" here?
> 
> Yep.
> 
> >> -how much has been written by the device, you usually have to zero
> >> -the buffer to ensure no data leakage occurs.
> >> +how much has been written by the device, a driver would have to zero
> >> +the buffer in advance to ensure no data leakage occurs.
> >> +
> >> +For example, a network driver
> >
> > any driver really, right?
> 
> Well, the block device has an explicit status byte, and an fixed length.
> 
> But there's a subtler detail I was considering when I designed this.
> 
> Imagine a Xen-style "driver domain" which is actually your device; it's
> *untrusted*.  This is possible if the (trusted) host does that actual
> data transfer, *and* reports the length; and such a mechanism is
> generic, so the host doesn't need to whether this is a block, net, or
> other device.
> 
> (Imagine the device-guest has R/O mapping of the avail ring and
>  descriptor table.  Ignoring indirect descriptors you only need a "copy
>  this data to/from this avail entry" helper to make this work).
> 
> > How about something like this:
> >
> > +The device MUST write at least \field{len} bytes to descriptor,
> > +beginning at the first device-writable buffer,
> > +prior to updating the used index field.
> > +The device MAY write more than \field{len} bytes to descriptor.
> > +The driver MUST NOT make assumptions about data in the buffer pointed to
> > +by the descriptor with WRITE flag
> > +beyond the first \field{len} bytes: the data
> > +might be unchanged by the device, or it might be
> > +overwritten by the device.
> > +The driver SHOULD ignore data beyond the first \field{len} bytes.
> 
> I like these, as long as we note that this MAY is to allow error cases,
> otherwise people might think they should just set len to zero.
> 
> Here it is, using the device-writable terminology, and explicitly
> requiring that the device must set len (otherwise the requirements
> about the device obeying len makes it look like it's set by the driver):
> 
> diff --git a/content.tex b/content.tex
> index 6ba079d..2c946a5 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -600,10 +600,19 @@ them: it is only written to by the device, and read by 
> the driver.
>  Each entry in the ring is a pair: \field{id} indicates the head entry of the
>  descriptor chain describing the buffer (this matches an entry
>  placed in the available ring by the guest earlier), and \field{len} the total
> -of bytes written into the buffer. The latter is extremely useful
> -for drivers using untrusted buffers: if you do not know exactly
> -how much has been written by the device, you usually have to zero
> -the buffer to ensure no data leakage occurs.
> +of bytes written into the buffer. 
> +
> +\begin{note}
> +\field{len} is useful
> +for drivers using untrusted buffers: if a driver does not know exactly
> +how much has been written by the device, the driver would have to zero
> +the buffer in advance to ensure no data leakage occurs.
> +
> +For example, a network driver may hand a received buffer directly to
> +an unprivileged userspace application.  If the network device has not
> +overwritten the bytes which were in that buffer, this may leak the
> +contents of freed memory from other processes to the application.
> +\end{note}
>  
>  \begin{note}
>  The legacy \hyperref[intro:Virtio PCI Draft]{[Virtio PCI Draft]}
> @@ -612,6 +621,28 @@ the constant as VRING_USED_F_NO_NOTIFY, but the layout 
> and value were
>  identical.
>  \end{note}
>  
> +\devicenormative{\subsubsection}{Virtqueue Notification Suppression}{Basic 
> Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> +
> +The device MUST set \field{len} prior to updating the used \field{idx}.
> +
> +The device MUST write at least \field{len} bytes to descriptor,
> +beginning at the first device-writable buffer,
> +prior to updating the used \field{idx}.
> +
> +The device MAY write more than \field{len} bytes to descriptor.
> +
> +\begin{note}
> +There are potential error cases where a device might not know what
> +parts of the buffers have been written.  This is why \field{len} may
> +be an underestimate, but that's preferable to the driver believing
> +that uninitialized memory has been overwritten when it has not.
> +\end{note}
> +
> +\drivernormative{\subsubsection}{Virtqueue Notification Suppression}{Basic 
> Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> +
> +The driver MUST NOT make assumptions about data in device-writable buffers
> +beyond the first \field{len} bytes, and SHOULD ignore it.

it -> this data.

Otherwise on first reading I thought "it" refers to len field.

> +
>  \subsection{Virtqueue Notification Suppression}\label{sec:Basic Facilities 
> of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
>  
>  The device can suppress notifications in a manner analogous to the way

Sounds good, let's move discussion to virtio/virtio-dev now?
I think it's 1.1 material - agree?

-- 
MST