[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH] used ring: define the meaning and requirements of the len field.
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.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]