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

[Michael S. Tsirkin]

On Fri, Mar 13, 2015 at 11:47:18AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> > On Thu, Mar 12, 2015 at 11:34:35AM +1030, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <address@hidden> writes:
> >> > On Wed, Mar 11, 2015 at 10:06:40PM +1030, Rusty Russell wrote:
> >> >>         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.
> >> >
> >> > Right so what does this "if you do not know exactly how much has been
> >> > written by the device" mean?
> >> 
> >> It means "without this feature, you would not know how much has been
> >> written by the device"...
> >
> > So imagine a situation where device does not know for sure
> > how much was written, like here.
> > Should it set len to value that was written for sure?
> > Or to value that was possibly written?
> 
> In this particular case, it doesn't matter since the failure is marked.
> 
> In general, as the stated purpose of 'len' is to avoid guest
> receive-buffer zeroing, it is implied that it must not overestimate.
> 
> Imagine the case of a guest user process receiving network packets.  If
> the net device says it's written 1000 bytes (but it hasn't) we will hand
> 1000 bytes of uninitialized kernel memory to that process.

Finally, I think I understand. Thanks for your patience.

> 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?

>  for drivers using untrusted buffers: if you do not know exactly

replace "you" with "driver" here?

> -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?

> 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,19 @@ 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} to the number of bytes known to be
> +written to the descriptor, beginning at the first device-writable
> +buffer.

I think "known to be written" is still too indeterministic for my taste.
Reminds me of the Schrödinger's cat experiment for some reason.

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.


> +
> +\begin{note}
> +There are potential error cases where a device might not know what
> +parts of the buffers have been written.  In this case \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}
> +
>  \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