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

[Rusty Russell]

"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.

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
 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.
+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 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.
+
+\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