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

[Rusty Russell]

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