OASIS Virtual I/O Device (VIRTIO) TC

We said what it was for, and noted why. We didn't place any requirements on it, nor clearly spell out the implications of its use. This clarification comes particularly from noticing that QEMU didn't set len correctly, and philosophising over the correct value when an error has occurred. (Wording precision feedback from Michael and Cornelia - Thanks!) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/content.tex b/content.tex index 6ba079d..2ff8c65 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. + +egin{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 could leak the +contents of freed memory from other processes to the application. +end{note} egin{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. + +egin{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} is +permitted to be an underestimate: 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 this data. + 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

On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> We said what it was for, and noted why. We didn't place any requirements
> on it, nor clearly spell out the implications of its use.
>
> This clarification comes particularly from noticing that QEMU didn't
> set len correctly, and philosophising over the correct value when
> an error has occurred.
>
> (Wording precision feedback from Michael and Cornelia - Thanks!)
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Regarding 1.0 versus 1.1: do you think this is a non-material change?
If we do material changes, we need a public review period,
and I feel reviewer's time is better spent reviewing 1.1.


> diff --git a/content.tex b/content.tex
> index 6ba079d..2ff8c65 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

I would add "in particular" here. It's also useful for many other drivers.

> +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 could 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}

Notification Suppression?

> +
> +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} is
> +permitted to be an underestimate: 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}


Same here.

> +
> +The driver MUST NOT make assumptions about data in device-writable buffers
> +beyond the first \field{len} bytes, and SHOULD ignore this data.
> +
> \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

On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: > We said what it was for, and noted why. We didn't place any requirements > on it, nor clearly spell out the implications of its use. > > This clarification comes particularly from noticing that QEMU didn't > set len correctly, and philosophising over the correct value when > an error has occurred. > > (Wording precision feedback from Michael and Cornelia - Thanks!) > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Regarding 1.0 versus 1.1: do you think this is a non-material change? If we do material changes, we need a public review period, and I feel reviewer's time is better spent reviewing 1.1. > diff --git a/content.tex b/content.tex > index 6ba079d..2ff8c65 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. > + > +egin{note} > +field{len} is useful I would add "in particular" here. It's also useful for many other drivers. > +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 could leak the > +contents of freed memory from other processes to the application. > +end{note} > > egin{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} Notification Suppression? > + > +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. > + > +egin{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} is > +permitted to be an underestimate: 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} Same here. > + > +The driver MUST NOT make assumptions about data in device-writable buffers > +beyond the first field{len} bytes, and SHOULD ignore this data. > + > 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

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
>> We said what it was for, and noted why. We didn't place any requirements
>> on it, nor clearly spell out the implications of its use.
>>
>> This clarification comes particularly from noticing that QEMU didn't
>> set len correctly, and philosophising over the correct value when
>> an error has occurred.
>>
>> (Wording precision feedback from Michael and Cornelia - Thanks!)
>>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Regarding 1.0 versus 1.1: do you think this is a non-material change?
> If we do material changes, we need a public review period,
> and I feel reviewer's time is better spent reviewing 1.1.

It's borderline. These requirements are implied by the text but not
spelled out. Clearly it is better to do so, but I don't think it's
worth a full review period.

>> diff --git a/content.tex b/content.tex
>> index 6ba079d..2ff8c65 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
>
> I would add "in particular" here. It's also useful for many other drivers.

OK, I said "particularly 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 could 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}
>
> Notification Suppression?

Hmm, cut & paste bug.

>> +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} is
>> +permitted to be an underestimate: 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}
>
>
> Same here.
>
>> +
>> +The driver MUST NOT make assumptions about data in device-writable buffers
>> +beyond the first \field{len} bytes, and SHOULD ignore this data.
>> +
>> \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

Inter-diff:

diff --git a/content.tex b/content.tex
index 2ff8c65..8e8765d 100644
--- a/content.tex
+++ b/content.tex
@@ -603,7 +603,7 @@ placed in the available ring by the guest earlier), and \field{len} the total
of bytes written into the buffer.

\begin{note}
-\field{len} is useful
+\field{len} is particularly 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.
@@ -621,7 +621,7 @@ 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}
+\devicenormative{\subsubsection}{The Virtqueue Used Ring}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}

The device MUST set \field{len} prior to updating the used \field{idx}.

@@ -638,7 +638,7 @@ permitted to be an underestimate: 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}
+\drivernormative{\subsubsection}{The Virtqueue Used Ring}{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 this data.

"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: >> We said what it was for, and noted why. We didn't place any requirements >> on it, nor clearly spell out the implications of its use. >> >> This clarification comes particularly from noticing that QEMU didn't >> set len correctly, and philosophising over the correct value when >> an error has occurred. >> >> (Wording precision feedback from Michael and Cornelia - Thanks!) >> >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > Regarding 1.0 versus 1.1: do you think this is a non-material change? > If we do material changes, we need a public review period, > and I feel reviewer's time is better spent reviewing 1.1. It's borderline. These requirements are implied by the text but not spelled out. Clearly it is better to do so, but I don't think it's worth a full review period. >> diff --git a/content.tex b/content.tex >> index 6ba079d..2ff8c65 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. >> + >> +egin{note} >> +field{len} is useful > > I would add "in particular" here. It's also useful for many other drivers. OK, I said "particularly 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 could leak the >> +contents of freed memory from other processes to the application. >> +end{note} >> >> egin{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} > > Notification Suppression? Hmm, cut & paste bug. >> +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. >> + >> +egin{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} is >> +permitted to be an underestimate: 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} > > > Same here. > >> + >> +The driver MUST NOT make assumptions about data in device-writable buffers >> +beyond the first field{len} bytes, and SHOULD ignore this data. >> + >> 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 Inter-diff: diff --git a/content.tex b/content.tex index 2ff8c65..8e8765d 100644 --- a/content.tex +++ b/content.tex @@ -603,7 +603,7 @@ placed in the available ring by the guest earlier), and field{len} the total of bytes written into the buffer. egin{note} -field{len} is useful +field{len} is particularly 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. @@ -621,7 +621,7 @@ 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} +devicenormative{subsubsection}{The Virtqueue Used Ring}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} The device MUST set field{len} prior to updating the used field{idx}. @@ -638,7 +638,7 @@ permitted to be an underestimate: 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} +drivernormative{subsubsection}{The Virtqueue Used Ring}{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 this data.

On Mon, Mar 23, 2015 at 02:06:14PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> >> We said what it was for, and noted why. We didn't place any requirements
> >> on it, nor clearly spell out the implications of its use.
> >>
> >> This clarification comes particularly from noticing that QEMU didn't
> >> set len correctly, and philosophising over the correct value when
> >> an error has occurred.
> >>
> >> (Wording precision feedback from Michael and Cornelia - Thanks!)
> >>
> >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >
> > Regarding 1.0 versus 1.1: do you think this is a non-material change?
> > If we do material changes, we need a public review period,
> > and I feel reviewer's time is better spent reviewing 1.1.
>
> It's borderline. These requirements are implied by the text but not
> spelled out. Clearly it is better to do so, but I don't think it's
> worth a full review period.

So, if you think we should include this in 1.0 cs03, then you
need to vote no on the cs03 ballot, and we'll
redo a draft with these changes applied.

> >> diff --git a/content.tex b/content.tex
> >> index 6ba079d..2ff8c65 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
> >
> > I would add "in particular" here. It's also useful for many other drivers.
>
> OK, I said "particularly 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 could 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}
> >
> > Notification Suppression?
>
> Hmm, cut & paste bug.
>
> >> +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} is
> >> +permitted to be an underestimate: 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}
> >
> >
> > Same here.
> >
> >> +
> >> +The driver MUST NOT make assumptions about data in device-writable buffers
> >> +beyond the first \field{len} bytes, and SHOULD ignore this data.
> >> +
> >> \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
>
> Inter-diff:
>
> diff --git a/content.tex b/content.tex
> index 2ff8c65..8e8765d 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -603,7 +603,7 @@ placed in the available ring by the guest earlier), and \field{len} the total
> of bytes written into the buffer.
>
> \begin{note}
> -\field{len} is useful
> +\field{len} is particularly 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.
> @@ -621,7 +621,7 @@ 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}
> +\devicenormative{\subsubsection}{The Virtqueue Used Ring}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
>
> The device MUST set \field{len} prior to updating the used \field{idx}.
>
> @@ -638,7 +638,7 @@ permitted to be an underestimate: 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}
> +\drivernormative{\subsubsection}{The Virtqueue Used Ring}{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 this data.

On Mon, Mar 23, 2015 at 02:06:14PM +1030, Rusty Russell wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: > >> We said what it was for, and noted why. We didn't place any requirements > >> on it, nor clearly spell out the implications of its use. > >> > >> This clarification comes particularly from noticing that QEMU didn't > >> set len correctly, and philosophising over the correct value when > >> an error has occurred. > >> > >> (Wording precision feedback from Michael and Cornelia - Thanks!) > >> > >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > > > Regarding 1.0 versus 1.1: do you think this is a non-material change? > > If we do material changes, we need a public review period, > > and I feel reviewer's time is better spent reviewing 1.1. > > It's borderline. These requirements are implied by the text but not > spelled out. Clearly it is better to do so, but I don't think it's > worth a full review period. So, if you think we should include this in 1.0 cs03, then you need to vote no on the cs03 ballot, and we'll redo a draft with these changes applied. > >> diff --git a/content.tex b/content.tex > >> index 6ba079d..2ff8c65 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. > >> + > >> +egin{note} > >> +field{len} is useful > > > > I would add "in particular" here. It's also useful for many other drivers. > > OK, I said "particularly 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 could leak the > >> +contents of freed memory from other processes to the application. > >> +end{note} > >> > >> egin{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} > > > > Notification Suppression? > > Hmm, cut & paste bug. > > >> +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. > >> + > >> +egin{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} is > >> +permitted to be an underestimate: 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} > > > > > > Same here. > > > >> + > >> +The driver MUST NOT make assumptions about data in device-writable buffers > >> +beyond the first field{len} bytes, and SHOULD ignore this data. > >> + > >> 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 > > Inter-diff: > > diff --git a/content.tex b/content.tex > index 2ff8c65..8e8765d 100644 > --- a/content.tex > +++ b/content.tex > @@ -603,7 +603,7 @@ placed in the available ring by the guest earlier), and field{len} the total > of bytes written into the buffer. > > egin{note} > -field{len} is useful > +field{len} is particularly 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. > @@ -621,7 +621,7 @@ 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} > +devicenormative{subsubsection}{The Virtqueue Used Ring}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} > > The device MUST set field{len} prior to updating the used field{idx}. > > @@ -638,7 +638,7 @@ permitted to be an underestimate: 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} > +drivernormative{subsubsection}{The Virtqueue Used Ring}{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 this data.

"Michael S. Tsirkin" <mst@redhat.com> writes:
> So, if you think we should include this in 1.0 cs03, then you
> need to vote no on the cs03 ballot, and we'll
> redo a draft with these changes applied.

On balance, I think that is necessary. I hate to think of an
implementation getting this wrong.

I encourage others to change their votes to No too.

Thanks,
Rusty.

"Michael S. Tsirkin" <mst@redhat.com> writes: > So, if you think we should include this in 1.0 cs03, then you > need to vote no on the cs03 ballot, and we'll > redo a draft with these changes applied. On balance, I think that is necessary. I hate to think of an implementation getting this wrong. I encourage others to change their votes to No too. Thanks, Rusty.

On Tue, Mar 24, 2015 at 11:35:12AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > So, if you think we should include this in 1.0 cs03, then you
> > need to vote no on the cs03 ballot, and we'll
> > redo a draft with these changes applied.
>
> On balance, I think that is necessary. I hate to think of an
> implementation getting this wrong.
>
> I encourage others to change their votes to No too.
>
> Thanks,
> Rusty.

OK, I closed the ballot, no point it dragging it out more.

On Tue, Mar 24, 2015 at 11:35:12AM +1030, Rusty Russell wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > So, if you think we should include this in 1.0 cs03, then you > > need to vote no on the cs03 ballot, and we'll > > redo a draft with these changes applied. > > On balance, I think that is necessary. I hate to think of an > implementation getting this wrong. > > I encourage others to change their votes to No too. > > Thanks, > Rusty. OK, I closed the ballot, no point it dragging it out more.

On Tue, Mar 24, 2015 at 11:35:12AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > So, if you think we should include this in 1.0 cs03, then you
> > need to vote no on the cs03 ballot, and we'll
> > redo a draft with these changes applied.
>
> On balance, I think that is necessary. I hate to think of an
> implementation getting this wrong.
>
> I encourage others to change their votes to No too.
>
> Thanks,
> Rusty.

OK, what we need:
1. open a new issue
2. submit updated patch to ML
3. set patch as resolution proposal
4. set fixed in version to cs03
5. start ballot on resolution

Rusty, can you pls do 1-4? I have a script to do 5.

--
MST

On Tue, Mar 24, 2015 at 11:35:12AM +1030, Rusty Russell wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > So, if you think we should include this in 1.0 cs03, then you > > need to vote no on the cs03 ballot, and we'll > > redo a draft with these changes applied. > > On balance, I think that is necessary. I hate to think of an > implementation getting this wrong. > > I encourage others to change their votes to No too. > > Thanks, > Rusty. OK, what we need: 1. open a new issue 2. submit updated patch to ML 3. set patch as resolution proposal 4. set fixed in version to cs03 5. start ballot on resolution Rusty, can you pls do 1-4? I have a script to do 5. -- MST

We said what it was for, and noted why. We didn't place any requirements
on it, nor clearly spell out the implications of its use.

This clarification comes particularly from noticing that QEMU didn't
set len correctly, and philosophising over the correct value when
an error has occurred.

(Wording precision feedback from Michael and Cornelia - Thanks!)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
content.tex | 39 +++++++++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/content.tex b/content.tex
index 6ba079d..8e8765d 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 particularly 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 could 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}{The Virtqueue Used Ring}{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} is
+permitted to be an underestimate: that's preferable to the driver believing
+that uninitialized memory has been overwritten when it has not.
+\end{note}
+
+\drivernormative{\subsubsection}{The Virtqueue Used Ring}{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 this data.
+
\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
--
2.1.0

We said what it was for, and noted why. We didn't place any requirements on it, nor clearly spell out the implications of its use. This clarification comes particularly from noticing that QEMU didn't set len correctly, and philosophising over the correct value when an error has occurred. (Wording precision feedback from Michael and Cornelia - Thanks!) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> --- content.tex 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/content.tex b/content.tex index 6ba079d..8e8765d 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. + +egin{note} +field{len} is particularly 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 could leak the +contents of freed memory from other processes to the application. +end{note} egin{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}{The Virtqueue Used Ring}{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. + +egin{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} is +permitted to be an underestimate: that's preferable to the driver believing +that uninitialized memory has been overwritten when it has not. +end{note} + +drivernormative{subsubsection}{The Virtqueue Used Ring}{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 this data. + 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 -- 2.1.0

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Mar 24, 2015 at 11:35:12AM +1030, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > So, if you think we should include this in 1.0 cs03, then you
>> > need to vote no on the cs03 ballot, and we'll
>> > redo a draft with these changes applied.
>>
>> On balance, I think that is necessary. I hate to think of an
>> implementation getting this wrong.
>>
>> I encourage others to change their votes to No too.
>>
>> Thanks,
>> Rusty.
>
> OK, what we need:
> 1. open a new issue
> 2. submit updated patch to ML
> 3. set patch as resolution proposal
> 4. set fixed in version to cs03
> 5. start ballot on resolution
>
> Rusty, can you pls do 1-4? I have a script to do 5.

Done. Thanks!
Rusty.

"Michael S. Tsirkin" <mst@redhat.com> writes: > On Tue, Mar 24, 2015 at 11:35:12AM +1030, Rusty Russell wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> > So, if you think we should include this in 1.0 cs03, then you >> > need to vote no on the cs03 ballot, and we'll >> > redo a draft with these changes applied. >> >> On balance, I think that is necessary. I hate to think of an >> implementation getting this wrong. >> >> I encourage others to change their votes to No too. >> >> Thanks, >> Rusty. > > OK, what we need: > 1. open a new issue > 2. submit updated patch to ML > 3. set patch as resolution proposal > 4. set fixed in version to cs03 > 5. start ballot on resolution > > Rusty, can you pls do 1-4? I have a script to do 5. Done. Thanks! Rusty.

On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> We said what it was for, and noted why. We didn't place any requirements
> on it, nor clearly spell out the implications of its use.
>
> This clarification comes particularly from noticing that QEMU didn't
> set len correctly, and philosophising over the correct value when
> an error has occurred.

I'm having second thoughts about this philosophising here :)
Imagine this driver with untrusted buffers used by multiple
applications. Assume application 1 uses the device.
We get len value from device but with your change applied, we don't
really know whether any bytes > len have been modified.
Passing them to application 2 will thus leak some info from application
1 - thus driver must zero out the rest of buffer before reuse.

To me, it looks like
- existing spec text can be understood to match QEMU behaviour
- it isn't obviously clear what's best

Thoughts?

> (Wording precision feedback from Michael and Cornelia - Thanks!)
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> diff --git a/content.tex b/content.tex
> index 6ba079d..2ff8c65 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 could 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} is
> +permitted to be an underestimate: 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 this data.
> +
> \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

On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: > We said what it was for, and noted why. We didn't place any requirements > on it, nor clearly spell out the implications of its use. > > This clarification comes particularly from noticing that QEMU didn't > set len correctly, and philosophising over the correct value when > an error has occurred. I'm having second thoughts about this philosophising here :) Imagine this driver with untrusted buffers used by multiple applications. Assume application 1 uses the device. We get len value from device but with your change applied, we don't really know whether any bytes > len have been modified. Passing them to application 2 will thus leak some info from application 1 - thus driver must zero out the rest of buffer before reuse. To me, it looks like - existing spec text can be understood to match QEMU behaviour - it isn't obviously clear what's best Thoughts? > (Wording precision feedback from Michael and Cornelia - Thanks!) > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > diff --git a/content.tex b/content.tex > index 6ba079d..2ff8c65 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. > + > +egin{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 could leak the > +contents of freed memory from other processes to the application. > +end{note} > > egin{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. > + > +egin{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} is > +permitted to be an underestimate: 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 this data. > + > 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

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
>> We said what it was for, and noted why. We didn't place any requirements
>> on it, nor clearly spell out the implications of its use.
>>
>> This clarification comes particularly from noticing that QEMU didn't
>> set len correctly, and philosophising over the correct value when
>> an error has occurred.
>
> I'm having second thoughts about this philosophising here :)

That's what philosophising is all about, I think!

> Imagine this driver with untrusted buffers used by multiple
> applications. Assume application 1 uses the device.
> We get len value from device but with your change applied, we don't
> really know whether any bytes > len have been modified.
> Passing them to application 2 will thus leak some info from application
> 1 - thus driver must zero out the rest of buffer before reuse.

I don't see it. Program1 and Program2 read from /dev/fubar.
Internally, /dev/fubar reuses a 4k buffer.

virtio says 1k was written to the buffer, so 1k is copied to Program1.
Buffer is reposted, and virtio says 2k was written to the buffer, so 2k
is copied to Program2.

If Program2 has some other way of accessing the left over buffer, that's
a problem. But I think that will always require special driver
handling.

Cheers,
Rusty.

"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: >> We said what it was for, and noted why. We didn't place any requirements >> on it, nor clearly spell out the implications of its use. >> >> This clarification comes particularly from noticing that QEMU didn't >> set len correctly, and philosophising over the correct value when >> an error has occurred. > > I'm having second thoughts about this philosophising here :) That's what philosophising is all about, I think! > Imagine this driver with untrusted buffers used by multiple > applications. Assume application 1 uses the device. > We get len value from device but with your change applied, we don't > really know whether any bytes > len have been modified. > Passing them to application 2 will thus leak some info from application > 1 - thus driver must zero out the rest of buffer before reuse. I don't see it. Program1 and Program2 read from /dev/fubar. Internally, /dev/fubar reuses a 4k buffer. virtio says 1k was written to the buffer, so 1k is copied to Program1. Buffer is reposted, and virtio says 2k was written to the buffer, so 2k is copied to Program2. If Program2 has some other way of accessing the left over buffer, that's a problem. But I think that will always require special driver handling. Cheers, Rusty.

On Tue, Apr 07, 2015 at 10:49:54AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> >> We said what it was for, and noted why. We didn't place any requirements
> >> on it, nor clearly spell out the implications of its use.
> >>
> >> This clarification comes particularly from noticing that QEMU didn't
> >> set len correctly, and philosophising over the correct value when
> >> an error has occurred.
> >
> > I'm having second thoughts about this philosophising here :)
>
> That's what philosophising is all about, I think!
>
> > Imagine this driver with untrusted buffers used by multiple
> > applications. Assume application 1 uses the device.
> > We get len value from device but with your change applied, we don't
> > really know whether any bytes > len have been modified.
> > Passing them to application 2 will thus leak some info from application
> > 1 - thus driver must zero out the rest of buffer before reuse.
>
> I don't see it. Program1 and Program2 read from /dev/fubar.
> Internally, /dev/fubar reuses a 4k buffer.
>
> virtio says 1k was written to the buffer, so 1k is copied to Program1.
> Buffer is reposted, and virtio says 2k was written to the buffer, so 2k
> is copied to Program2.
>
> If Program2 has some other way of accessing the left over buffer, that's
> a problem. But I think that will always require special driver
> handling.
>
> Cheers,
> Rusty.

I think it's the presence of copies that makes the problem
disappear. But the original problem also wasn't there
with copies I think.

Here's what I had in mind without copies:

Program 1 initializes buffer and supplies buffer to driver
(e.g. len = read(buf, 2048)). driver does get user pages and gives it to
device, which writes to first 2K but tells driver only first 1K is
modified. Now len == 1K, so Program 1 assumes data after first 1K
is unmodified. Program 1 removes some private data from first 1K but
assumes following 1K is unmodified and copies whole buffer to program 2,
leaking info in the second 1K. Why copy whole 2K you ask?
Maybe exact length is also private info we don't want to leak.


--
MST

On Tue, Apr 07, 2015 at 10:49:54AM +0930, Rusty Russell wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: > >> We said what it was for, and noted why. We didn't place any requirements > >> on it, nor clearly spell out the implications of its use. > >> > >> This clarification comes particularly from noticing that QEMU didn't > >> set len correctly, and philosophising over the correct value when > >> an error has occurred. > > > > I'm having second thoughts about this philosophising here :) > > That's what philosophising is all about, I think! > > > Imagine this driver with untrusted buffers used by multiple > > applications. Assume application 1 uses the device. > > We get len value from device but with your change applied, we don't > > really know whether any bytes > len have been modified. > > Passing them to application 2 will thus leak some info from application > > 1 - thus driver must zero out the rest of buffer before reuse. > > I don't see it. Program1 and Program2 read from /dev/fubar. > Internally, /dev/fubar reuses a 4k buffer. > > virtio says 1k was written to the buffer, so 1k is copied to Program1. > Buffer is reposted, and virtio says 2k was written to the buffer, so 2k > is copied to Program2. > > If Program2 has some other way of accessing the left over buffer, that's > a problem. But I think that will always require special driver > handling. > > Cheers, > Rusty. I think it's the presence of copies that makes the problem disappear. But the original problem also wasn't there with copies I think. Here's what I had in mind without copies: Program 1 initializes buffer and supplies buffer to driver (e.g. len = read(buf, 2048)). driver does get user pages and gives it to device, which writes to first 2K but tells driver only first 1K is modified. Now len == 1K, so Program 1 assumes data after first 1K is unmodified. Program 1 removes some private data from first 1K but assumes following 1K is unmodified and copies whole buffer to program 2, leaking info in the second 1K. Why copy whole 2K you ask? Maybe exact length is also private info we don't want to leak. -- MST

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Apr 07, 2015 at 10:49:54AM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
>> >> We said what it was for, and noted why. We didn't place any requirements
>> >> on it, nor clearly spell out the implications of its use.
>> >>
>> >> This clarification comes particularly from noticing that QEMU didn't
>> >> set len correctly, and philosophising over the correct value when
>> >> an error has occurred.
>> >
>> > I'm having second thoughts about this philosophising here :)
>>
>> That's what philosophising is all about, I think!
>>
>> > Imagine this driver with untrusted buffers used by multiple
>> > applications. Assume application 1 uses the device.
>> > We get len value from device but with your change applied, we don't
>> > really know whether any bytes > len have been modified.
>> > Passing them to application 2 will thus leak some info from application
>> > 1 - thus driver must zero out the rest of buffer before reuse.
>>
>> I don't see it. Program1 and Program2 read from /dev/fubar.
>> Internally, /dev/fubar reuses a 4k buffer.
>>
>> virtio says 1k was written to the buffer, so 1k is copied to Program1.
>> Buffer is reposted, and virtio says 2k was written to the buffer, so 2k
>> is copied to Program2.
>>
>> If Program2 has some other way of accessing the left over buffer, that's
>> a problem. But I think that will always require special driver
>> handling.
>>
>> Cheers,
>> Rusty.
>
> I think it's the presence of copies that makes the problem
> disappear. But the original problem also wasn't there
> with copies I think.
>
> Here's what I had in mind without copies:
>
> Program 1 initializes buffer and supplies buffer to driver
> (e.g. len = read(buf, 2048)). driver does get user pages and gives it to
> device, which writes to first 2K but tells driver only first 1K is
> modified. Now len == 1K, so Program 1 assumes data after first 1K
> is unmodified. Program 1 removes some private data from first 1K but
> assumes following 1K is unmodified and copies whole buffer to program 2,
> leaking info in the second 1K. Why copy whole 2K you ask?
> Maybe exact length is also private info we don't want to leak.

That's quite a convoluted scenario. But yes, the driver would have to
zero the rest, and that's made clear by the proposed change:

+The driver MUST NOT make assumptions about data in device-writable buffers
+beyond the first \field{len} bytes, and SHOULD ignore this data.

Far worse is if the device returns 2k when it's only written 1k. Then
the (more straightforward) case of Program 1 reading 2k, and the driver
allocating an unzeroed buffer and returning 2k of data.

Cheers,
Rusty.

"Michael S. Tsirkin" <mst@redhat.com> writes: > On Tue, Apr 07, 2015 at 10:49:54AM +0930, Rusty Russell wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: >> >> We said what it was for, and noted why. We didn't place any requirements >> >> on it, nor clearly spell out the implications of its use. >> >> >> >> This clarification comes particularly from noticing that QEMU didn't >> >> set len correctly, and philosophising over the correct value when >> >> an error has occurred. >> > >> > I'm having second thoughts about this philosophising here :) >> >> That's what philosophising is all about, I think! >> >> > Imagine this driver with untrusted buffers used by multiple >> > applications. Assume application 1 uses the device. >> > We get len value from device but with your change applied, we don't >> > really know whether any bytes > len have been modified. >> > Passing them to application 2 will thus leak some info from application >> > 1 - thus driver must zero out the rest of buffer before reuse. >> >> I don't see it. Program1 and Program2 read from /dev/fubar. >> Internally, /dev/fubar reuses a 4k buffer. >> >> virtio says 1k was written to the buffer, so 1k is copied to Program1. >> Buffer is reposted, and virtio says 2k was written to the buffer, so 2k >> is copied to Program2. >> >> If Program2 has some other way of accessing the left over buffer, that's >> a problem. But I think that will always require special driver >> handling. >> >> Cheers, >> Rusty. > > I think it's the presence of copies that makes the problem > disappear. But the original problem also wasn't there > with copies I think. > > Here's what I had in mind without copies: > > Program 1 initializes buffer and supplies buffer to driver > (e.g. len = read(buf, 2048)). driver does get user pages and gives it to > device, which writes to first 2K but tells driver only first 1K is > modified. Now len == 1K, so Program 1 assumes data after first 1K > is unmodified. Program 1 removes some private data from first 1K but > assumes following 1K is unmodified and copies whole buffer to program 2, > leaking info in the second 1K. Why copy whole 2K you ask? > Maybe exact length is also private info we don't want to leak. That's quite a convoluted scenario. But yes, the driver would have to zero the rest, and that's made clear by the proposed change: +The driver MUST NOT make assumptions about data in device-writable buffers +beyond the first field{len} bytes, and SHOULD ignore this data. Far worse is if the device returns 2k when it's only written 1k. Then the (more straightforward) case of Program 1 reading 2k, and the driver allocating an unzeroed buffer and returning 2k of data. Cheers, Rusty.

On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> We said what it was for, and noted why. We didn't place any requirements
> on it, nor clearly spell out the implications of its use.
>
> This clarification comes particularly from noticing that QEMU didn't
> set len correctly, and philosophising over the correct value when
> an error has occurred.
>
> (Wording precision feedback from Michael and Cornelia - Thanks!)
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> diff --git a/content.tex b/content.tex
> index 6ba079d..2ff8c65 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 could 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} is
> +permitted to be an underestimate: 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 this data.
> +
> \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

We know legacy devices don't follow this, so we also need some text in
the legacy sections to document the differences.
What can be said?
- some legacy devices included the write buffer length
in len value.
- on error, some legacy devices included full
request size in len value

Good summary? Now, for recommendations:
When using the legacy interface, transitional drivers
SHOULD ....
what should be done?

>
> ---------------------------------------------------------------------
> To unsubscribe from this mail list, you must leave the OASIS TC that
> generates this mail. Follow this link to all your TCs in OASIS at:
> https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php

On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: > We said what it was for, and noted why. We didn't place any requirements > on it, nor clearly spell out the implications of its use. > > This clarification comes particularly from noticing that QEMU didn't > set len correctly, and philosophising over the correct value when > an error has occurred. > > (Wording precision feedback from Michael and Cornelia - Thanks!) > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > diff --git a/content.tex b/content.tex > index 6ba079d..2ff8c65 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. > + > +egin{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 could leak the > +contents of freed memory from other processes to the application. > +end{note} > > egin{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. > + > +egin{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} is > +permitted to be an underestimate: 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 this data. > + > 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 We know legacy devices don't follow this, so we also need some text in the legacy sections to document the differences. What can be said? - some legacy devices included the write buffer length in len value. - on error, some legacy devices included full request size in len value Good summary? Now, for recommendations: When using the legacy interface, transitional drivers SHOULD .... what should be done? > > --------------------------------------------------------------------- > To unsubscribe from this mail list, you must leave the OASIS TC that > generates this mail. Follow this link to all your TCs in OASIS at: > https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
>> +\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} is
>> +permitted to be an underestimate: 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 this data.
>> +
>> \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
>
> We know legacy devices don't follow this, so we also need some text in
> the legacy sections to document the differences.

Indeed, but it was specific to block devices.

It may be worth noting that older block devices would set erroneously
set a length corresponding to the entire buffer length, and thus legacy
drivers SHOULD rely on the status byte and ignore used.len?

Cheers,
Rusty.

"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: >> +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. >> + >> +egin{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} is >> +permitted to be an underestimate: 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 this data. >> + >> 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 > > We know legacy devices don't follow this, so we also need some text in > the legacy sections to document the differences. Indeed, but it was specific to block devices. It may be worth noting that older block devices would set erroneously set a length corresponding to the entire buffer length, and thus legacy drivers SHOULD rely on the status byte and ignore used.len? Cheers, Rusty.

On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> >> +\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} is
> >> +permitted to be an underestimate: 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 this data.
> >> +
> >> \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
> >
> > We know legacy devices don't follow this, so we also need some text in
> > the legacy sections to document the differences.
>
> Indeed, but it was specific to block devices.
>
> It may be worth noting that older block devices would set erroneously
> set a length corresponding to the entire buffer length, and thus legacy
> drivers SHOULD rely on the status byte and ignore used.len?
>
> Cheers,
> Rusty.

QEMU before 1.3 also set used len in tx descriptor to packet length.

--
MST

On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: > >> +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. > >> + > >> +egin{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} is > >> +permitted to be an underestimate: 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 this data. > >> + > >> 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 > > > > We know legacy devices don't follow this, so we also need some text in > > the legacy sections to document the differences. > > Indeed, but it was specific to block devices. > > It may be worth noting that older block devices would set erroneously > set a length corresponding to the entire buffer length, and thus legacy > drivers SHOULD rely on the status byte and ignore used.len? > > Cheers, > Rusty. QEMU before 1.3 also set used len in tx descriptor to packet length. -- MST

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
>> >> +\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} is
>> >> +permitted to be an underestimate: 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 this data.
>> >> +
>> >> \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
>> >
>> > We know legacy devices don't follow this, so we also need some text in
>> > the legacy sections to document the differences.
>>
>> Indeed, but it was specific to block devices.
>>
>> It may be worth noting that older block devices would set erroneously
>> set a length corresponding to the entire buffer length, and thus legacy
>> drivers SHOULD rely on the status byte and ignore used.len?
>>
>> Cheers,
>> Rusty.
>
> QEMU before 1.3 also set used len in tx descriptor to packet length.

Yech. How's this on top then? (Untested, uprade seems to have broken
tex here):

diff --git a/conformance.tex b/conformance.tex
index 619859b..fae899a 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements:
\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
+\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
\item \ref{drivernormative:General Initialization And Device Operation / Device Initialization}
\item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx}
\item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device}
@@ -143,6 +144,7 @@ A device MUST conform to the following normative statements:
\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
+\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
\item \ref{devicenormative:Reserved Feature Bits}
\end{itemize}
@@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits}
\item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
\item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
\item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
+\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
\item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
\item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
\item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
diff --git a/content.tex b/content.tex
index 8e8765d..a23148d 100644
--- a/content.tex
+++ b/content.tex
@@ -643,6 +643,19 @@ that uninitialized memory has been overwritten when it has not.
The driver MUST NOT make assumptions about data in device-writable buffers
beyond the first \field{len} bytes, and SHOULD ignore this data.

+\drivernormative{\subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
+
+When using the legacy interface, a driver SHOULD treat a \field{len}
+which exceeds the total length of device-writable buffers as if it
+were equal to that length.
+
+\begin{note}
+Some legacy device implementations incorrectly set \field{len} to the
+total descriptor length, not the written length; in particular on
+network device transmit packets (QEMU prior to version 1.3) and block
+device writes (QEMU including version 2.2).
+\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

"Michael S. Tsirkin" <mst@redhat.com> writes: > On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: >> >> +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. >> >> + >> >> +egin{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} is >> >> +permitted to be an underestimate: 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 this data. >> >> + >> >> 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 >> > >> > We know legacy devices don't follow this, so we also need some text in >> > the legacy sections to document the differences. >> >> Indeed, but it was specific to block devices. >> >> It may be worth noting that older block devices would set erroneously >> set a length corresponding to the entire buffer length, and thus legacy >> drivers SHOULD rely on the status byte and ignore used.len? >> >> Cheers, >> Rusty. > > QEMU before 1.3 also set used len in tx descriptor to packet length. Yech. How's this on top then? (Untested, uprade seems to have broken tex here): diff --git a/conformance.tex b/conformance.tex index 619859b..fae899a 100644 --- a/conformance.tex +++ b/conformance.tex @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements: item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} +item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} item
ef{drivernormative:General Initialization And Device Operation / Device Initialization} item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx} item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device} @@ -143,6 +144,7 @@ A device MUST conform to the following normative statements: item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table} item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} +item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} item
ef{devicenormative:Reserved Feature Bits} end{itemize} @@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits} item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout} item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness} item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing} +item Section
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} item Section
ef{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization} item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery} item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} diff --git a/content.tex b/content.tex index 8e8765d..a23148d 100644 --- a/content.tex +++ b/content.tex @@ -643,6 +643,19 @@ that uninitialized memory has been overwritten when it has not. The driver MUST NOT make assumptions about data in device-writable buffers beyond the first field{len} bytes, and SHOULD ignore this data. +drivernormative{subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} + +When using the legacy interface, a driver SHOULD treat a field{len} +which exceeds the total length of device-writable buffers as if it +were equal to that length. + +egin{note} +Some legacy device implementations incorrectly set field{len} to the +total descriptor length, not the written length; in particular on +network device transmit packets (QEMU prior to version 1.3) and block +device writes (QEMU including version 2.2). +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

On Fri, Apr 10, 2015 at 12:49:56PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> >> >> +\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} is
> >> >> +permitted to be an underestimate: 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 this data.
> >> >> +
> >> >> \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
> >> >
> >> > We know legacy devices don't follow this, so we also need some text in
> >> > the legacy sections to document the differences.
> >>
> >> Indeed, but it was specific to block devices.
> >>
> >> It may be worth noting that older block devices would set erroneously
> >> set a length corresponding to the entire buffer length, and thus legacy
> >> drivers SHOULD rely on the status byte and ignore used.len?
> >>
> >> Cheers,
> >> Rusty.
> >
> > QEMU before 1.3 also set used len in tx descriptor to packet length.
>
> Yech. How's this on top then? (Untested, uprade seems to have broken
> tex here):
>
> diff --git a/conformance.tex b/conformance.tex
> index 619859b..fae899a 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements:
> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
> +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> \item \ref{drivernormative:General Initialization And Device Operation / Device Initialization}
> \item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx}
> \item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device}
> @@ -143,6 +144,7 @@ A device MUST conform to the following normative statements:
> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
> \item \ref{devicenormative:Reserved Feature Bits}
> \end{itemize}
> @@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits}
> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
> +\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
> \item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
> \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
> \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
> diff --git a/content.tex b/content.tex
> index 8e8765d..a23148d 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -643,6 +643,19 @@ that uninitialized memory has been overwritten when it has not.
> The driver MUST NOT make assumptions about data in device-writable buffers
> beyond the first \field{len} bytes, and SHOULD ignore this data.
>
> +\drivernormative{\subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
> +
> +When using the legacy interface, a driver SHOULD treat a \field{len}
> +which exceeds the total length of device-writable buffers as if it
> +were equal to that length.
> +
> +\begin{note}
> +Some legacy device implementations incorrectly set \field{len} to the
> +total descriptor length, not the written length; in particular on
> +network device transmit packets (QEMU prior to version 1.3) and block
> +device writes (QEMU including version 2.2).
> +\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

Didn't you want to mention read errors too?

On Fri, Apr 10, 2015 at 12:49:56PM +0930, Rusty Russell wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: > >> >> +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. > >> >> + > >> >> +egin{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} is > >> >> +permitted to be an underestimate: 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 this data. > >> >> + > >> >> 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 > >> > > >> > We know legacy devices don't follow this, so we also need some text in > >> > the legacy sections to document the differences. > >> > >> Indeed, but it was specific to block devices. > >> > >> It may be worth noting that older block devices would set erroneously > >> set a length corresponding to the entire buffer length, and thus legacy > >> drivers SHOULD rely on the status byte and ignore used.len? > >> > >> Cheers, > >> Rusty. > > > > QEMU before 1.3 also set used len in tx descriptor to packet length. > > Yech. How's this on top then? (Untested, uprade seems to have broken > tex here): > > diff --git a/conformance.tex b/conformance.tex > index 619859b..fae899a 100644 > --- a/conformance.tex > +++ b/conformance.tex > @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements: > item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} > item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} > item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} > +item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} > item
ef{drivernormative:General Initialization And Device Operation / Device Initialization} > item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx} > item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device} > @@ -143,6 +144,7 @@ A device MUST conform to the following normative statements: > item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table} > item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} > item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} > +item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} > item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} > item
ef{devicenormative:Reserved Feature Bits} > end{itemize} > @@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits} > item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout} > item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness} > item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing} > +item Section
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} > item Section
ef{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization} > item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery} > item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} > diff --git a/content.tex b/content.tex > index 8e8765d..a23148d 100644 > --- a/content.tex > +++ b/content.tex > @@ -643,6 +643,19 @@ that uninitialized memory has been overwritten when it has not. > The driver MUST NOT make assumptions about data in device-writable buffers > beyond the first field{len} bytes, and SHOULD ignore this data. > > +drivernormative{subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} > + > +When using the legacy interface, a driver SHOULD treat a field{len} > +which exceeds the total length of device-writable buffers as if it > +were equal to that length. > + > +egin{note} > +Some legacy device implementations incorrectly set field{len} to the > +total descriptor length, not the written length; in particular on > +network device transmit packets (QEMU prior to version 1.3) and block > +device writes (QEMU including version 2.2). > +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 Didn't you want to mention read errors too?

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Apr 10, 2015 at 12:49:56PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
>> >> >> +\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} is
>> >> >> +permitted to be an underestimate: 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 this data.
>> >> >> +
>> >> >> \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
>> >> >
>> >> > We know legacy devices don't follow this, so we also need some text in
>> >> > the legacy sections to document the differences.
>> >>
>> >> Indeed, but it was specific to block devices.
>> >>
>> >> It may be worth noting that older block devices would set erroneously
>> >> set a length corresponding to the entire buffer length, and thus legacy
>> >> drivers SHOULD rely on the status byte and ignore used.len?
>> >>
>> >> Cheers,
>> >> Rusty.
>> >
>> > QEMU before 1.3 also set used len in tx descriptor to packet length.
>>
>> Yech. How's this on top then? (Untested, uprade seems to have broken
>> tex here):
>>
>> diff --git a/conformance.tex b/conformance.tex
>> index 619859b..fae899a 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements:
>> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
>> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
>> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
>> +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
>> \item \ref{drivernormative:General Initialization And Device Operation / Device Initialization}
>> \item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx}
>> \item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device}
>> @@ -143,6 +144,7 @@ A device MUST conform to the following normative statements:
>> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
>> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
>> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
>> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
>> \item \ref{devicenormative:Reserved Feature Bits}
>> \end{itemize}
>> @@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits}
>> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
>> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
>> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
>> +\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
>> \item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
>> \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
>> \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
>> diff --git a/content.tex b/content.tex
>> index 8e8765d..a23148d 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -643,6 +643,19 @@ that uninitialized memory has been overwritten when it has not.
>> The driver MUST NOT make assumptions about data in device-writable buffers
>> beyond the first \field{len} bytes, and SHOULD ignore this data.
>>
>> +\drivernormative{\subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
>> +
>> +When using the legacy interface, a driver SHOULD treat a \field{len}
>> +which exceeds the total length of device-writable buffers as if it
>> +were equal to that length.
>> +
>> +\begin{note}
>> +Some legacy device implementations incorrectly set \field{len} to the
>> +total descriptor length, not the written length; in particular on
>> +network device transmit packets (QEMU prior to version 1.3) and block
>> +device writes (QEMU including version 2.2).
>> +\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
>
> Didn't you want to mention read errors too?

Already mentioned in previous patch. Here's the full proposed diff, for
simpler review:

diff --git a/conformance.tex b/conformance.tex
index 619859b..fae899a 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements:
\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
+\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
\item \ref{drivernormative:General Initialization And Device Operation / Device Initialization}
\item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx}
\item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device}
@@ -143,6 +144,7 @@ A device MUST conform to the following normative statements:
\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
+\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
\item \ref{devicenormative:Reserved Feature Bits}
\end{itemize}
@@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits}
\item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
\item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
\item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
+\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
\item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
\item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
\item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
diff --git a/content.tex b/content.tex
index 6ba079d..a23148d 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 particularly 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 could 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,41 @@ the constant as VRING_USED_F_NO_NOTIFY, but the layout and value were
identical.
\end{note}

+\devicenormative{\subsubsection}{The Virtqueue Used Ring}{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} is
+permitted to be an underestimate: that's preferable to the driver believing
+that uninitialized memory has been overwritten when it has not.
+\end{note}
+
+\drivernormative{\subsubsection}{The Virtqueue Used Ring}{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 this data.
+
+\drivernormative{\subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
+
+When using the legacy interface, a driver SHOULD treat a \field{len}
+which exceeds the total length of device-writable buffers as if it
+were equal to that length.
+
+\begin{note}
+Some legacy device implementations incorrectly set \field{len} to the
+total descriptor length, not the written length; in particular on
+network device transmit packets (QEMU prior to version 1.3) and block
+device writes (QEMU including version 2.2).
+\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

"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Apr 10, 2015 at 12:49:56PM +0930, Rusty Russell wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> > On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote: >> >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: >> >> >> +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. >> >> >> + >> >> >> +egin{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} is >> >> >> +permitted to be an underestimate: 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 this data. >> >> >> + >> >> >> 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 >> >> > >> >> > We know legacy devices don't follow this, so we also need some text in >> >> > the legacy sections to document the differences. >> >> >> >> Indeed, but it was specific to block devices. >> >> >> >> It may be worth noting that older block devices would set erroneously >> >> set a length corresponding to the entire buffer length, and thus legacy >> >> drivers SHOULD rely on the status byte and ignore used.len? >> >> >> >> Cheers, >> >> Rusty. >> > >> > QEMU before 1.3 also set used len in tx descriptor to packet length. >> >> Yech. How's this on top then? (Untested, uprade seems to have broken >> tex here): >> >> diff --git a/conformance.tex b/conformance.tex >> index 619859b..fae899a 100644 >> --- a/conformance.tex >> +++ b/conformance.tex >> @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements: >> item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} >> item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} >> item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} >> +item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} >> item
ef{drivernormative:General Initialization And Device Operation / Device Initialization} >> item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx} >> item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device} >> @@ -143,6 +144,7 @@ A device MUST conform to the following normative statements: >> item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table} >> item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} >> item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} >> +item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} >> item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} >> item
ef{devicenormative:Reserved Feature Bits} >> end{itemize} >> @@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits} >> item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout} >> item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness} >> item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing} >> +item Section
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} >> item Section
ef{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization} >> item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery} >> item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} >> diff --git a/content.tex b/content.tex >> index 8e8765d..a23148d 100644 >> --- a/content.tex >> +++ b/content.tex >> @@ -643,6 +643,19 @@ that uninitialized memory has been overwritten when it has not. >> The driver MUST NOT make assumptions about data in device-writable buffers >> beyond the first field{len} bytes, and SHOULD ignore this data. >> >> +drivernormative{subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} >> + >> +When using the legacy interface, a driver SHOULD treat a field{len} >> +which exceeds the total length of device-writable buffers as if it >> +were equal to that length. >> + >> +egin{note} >> +Some legacy device implementations incorrectly set field{len} to the >> +total descriptor length, not the written length; in particular on >> +network device transmit packets (QEMU prior to version 1.3) and block >> +device writes (QEMU including version 2.2). >> +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 > > Didn't you want to mention read errors too? Already mentioned in previous patch. Here's the full proposed diff, for simpler review: diff --git a/conformance.tex b/conformance.tex index 619859b..fae899a 100644 --- a/conformance.tex +++ b/conformance.tex @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements: item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} +item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} item
ef{drivernormative:General Initialization And Device Operation / Device Initialization} item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx} item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device} @@ -143,6 +144,7 @@ A device MUST conform to the following normative statements: item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table} item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} +item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} item
ef{devicenormative:Reserved Feature Bits} end{itemize} @@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits} item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout} item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness} item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing} +item Section
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} item Section
ef{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization} item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery} item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} diff --git a/content.tex b/content.tex index 6ba079d..a23148d 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. + +egin{note} +field{len} is particularly 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 could leak the +contents of freed memory from other processes to the application. +end{note} egin{note} The legacy hyperref[intro:Virtio PCI Draft]{[Virtio PCI Draft]} @@ -612,6 +621,41 @@ the constant as VRING_USED_F_NO_NOTIFY, but the layout and value were identical. end{note} +devicenormative{subsubsection}{The Virtqueue Used Ring}{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. + +egin{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} is +permitted to be an underestimate: that's preferable to the driver believing +that uninitialized memory has been overwritten when it has not. +end{note} + +drivernormative{subsubsection}{The Virtqueue Used Ring}{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 this data. + +drivernormative{subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} + +When using the legacy interface, a driver SHOULD treat a field{len} +which exceeds the total length of device-writable buffers as if it +were equal to that length. + +egin{note} +Some legacy device implementations incorrectly set field{len} to the +total descriptor length, not the written length; in particular on +network device transmit packets (QEMU prior to version 1.3) and block +device writes (QEMU including version 2.2). +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

On Tue, Apr 14, 2015 at 10:59:06AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Fri, Apr 10, 2015 at 12:49:56PM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote:
> >> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> >> >> >> +\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} is
> >> >> >> +permitted to be an underestimate: 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 this data.
> >> >> >> +
> >> >> >> \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
> >> >> >
> >> >> > We know legacy devices don't follow this, so we also need some text in
> >> >> > the legacy sections to document the differences.
> >> >>
> >> >> Indeed, but it was specific to block devices.
> >> >>
> >> >> It may be worth noting that older block devices would set erroneously
> >> >> set a length corresponding to the entire buffer length, and thus legacy
> >> >> drivers SHOULD rely on the status byte and ignore used.len?
> >> >>
> >> >> Cheers,
> >> >> Rusty.
> >> >
> >> > QEMU before 1.3 also set used len in tx descriptor to packet length.
> >>
> >> Yech. How's this on top then? (Untested, uprade seems to have broken
> >> tex here):
> >>
> >> diff --git a/conformance.tex b/conformance.tex
> >> index 619859b..fae899a 100644
> >> --- a/conformance.tex
> >> +++ b/conformance.tex
> >> @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements:
> >> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> >> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
> >> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
> >> +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> >> \item \ref{drivernormative:General Initialization And Device Operation / Device Initialization}
> >> \item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx}
> >> \item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device}
> >> @@ -143,6 +144,7 @@ A device MUST conform to the following normative statements:
> >> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
> >> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> >> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
> >> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> >> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
> >> \item \ref{devicenormative:Reserved Feature Bits}
> >> \end{itemize}
> >> @@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits}
> >> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
> >> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
> >> \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
> >> +\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
> >> \item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
> >> \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
> >> \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
> >> diff --git a/content.tex b/content.tex
> >> index 8e8765d..a23148d 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -643,6 +643,19 @@ that uninitialized memory has been overwritten when it has not.
> >> The driver MUST NOT make assumptions about data in device-writable buffers
> >> beyond the first \field{len} bytes, and SHOULD ignore this data.
> >>
> >> +\drivernormative{\subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
> >> +
> >> +When using the legacy interface, a driver SHOULD treat a \field{len}
> >> +which exceeds the total length of device-writable buffers as if it
> >> +were equal to that length.
> >> +
> >> +\begin{note}
> >> +Some legacy device implementations incorrectly set \field{len} to the
> >> +total descriptor length, not the written length; in particular on
> >> +network device transmit packets (QEMU prior to version 1.3) and block
> >> +device writes (QEMU including version 2.2).
> >> +\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
> >
> > Didn't you want to mention read errors too?
>
> Already mentioned in previous patch.

In the non-legacy case, yes.
But I mean mention that legacy block (and scsi? Paolo do you remember)
devices would overestimate len in case of errors.

--
MST

On Tue, Apr 14, 2015 at 10:59:06AM +0930, Rusty Russell wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > On Fri, Apr 10, 2015 at 12:49:56PM +0930, Rusty Russell wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote: > >> >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote: > >> >> >> +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. > >> >> >> + > >> >> >> +egin{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} is > >> >> >> +permitted to be an underestimate: 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 this data. > >> >> >> + > >> >> >> 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 > >> >> > > >> >> > We know legacy devices don't follow this, so we also need some text in > >> >> > the legacy sections to document the differences. > >> >> > >> >> Indeed, but it was specific to block devices. > >> >> > >> >> It may be worth noting that older block devices would set erroneously > >> >> set a length corresponding to the entire buffer length, and thus legacy > >> >> drivers SHOULD rely on the status byte and ignore used.len? > >> >> > >> >> Cheers, > >> >> Rusty. > >> > > >> > QEMU before 1.3 also set used len in tx descriptor to packet length. > >> > >> Yech. How's this on top then? (Untested, uprade seems to have broken > >> tex here): > >> > >> diff --git a/conformance.tex b/conformance.tex > >> index 619859b..fae899a 100644 > >> --- a/conformance.tex > >> +++ b/conformance.tex > >> @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements: > >> item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} > >> item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} > >> item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} > >> +item
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} > >> item
ef{drivernormative:General Initialization And Device Operation / Device Initialization} > >> item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx} > >> item
ef{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device} > >> @@ -143,6 +144,7 @@ A device MUST conform to the following normative statements: > >> item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table} > >> item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} > >> item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression} > >> +item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} > >> item
ef{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression} > >> item
ef{devicenormative:Reserved Feature Bits} > >> end{itemize} > >> @@ -264,6 +266,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits} > >> item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout} > >> item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness} > >> item Section
ef{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing} > >> +item Section
ef{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} > >> item Section
ef{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization} > >> item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery} > >> item Section
ef{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} > >> diff --git a/content.tex b/content.tex > >> index 8e8765d..a23148d 100644 > >> --- a/content.tex > >> +++ b/content.tex > >> @@ -643,6 +643,19 @@ that uninitialized memory has been overwritten when it has not. > >> The driver MUST NOT make assumptions about data in device-writable buffers > >> beyond the first field{len} bytes, and SHOULD ignore this data. > >> > >> +drivernormative{subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field} > >> + > >> +When using the legacy interface, a driver SHOULD treat a field{len} > >> +which exceeds the total length of device-writable buffers as if it > >> +were equal to that length. > >> + > >> +egin{note} > >> +Some legacy device implementations incorrectly set field{len} to the > >> +total descriptor length, not the written length; in particular on > >> +network device transmit packets (QEMU prior to version 1.3) and block > >> +device writes (QEMU including version 2.2). > >> +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 > > > > Didn't you want to mention read errors too? > > Already mentioned in previous patch. In the non-legacy case, yes. But I mean mention that legacy block (and scsi? Paolo do you remember) devices would overestimate len in case of errors. -- MST