[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu.
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu. |
Date: |
Mon, 16 Mar 2015 06:03:24 +0100 |
On Mon, Mar 16, 2015 at 01:44:22PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> > On Fri, Mar 13, 2015 at 11:47:18AM +1030, Rusty Russell wrote:
> >> Here's my proposed spec patch, which spells this out:
> >>
> >> diff --git a/content.tex b/content.tex
> >> index 6ba079d..b6345a8 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -600,10 +600,19 @@ them: it is only written to by the device, and read
> >> by the driver.
> >> Each entry in the ring is a pair: \field{id} indicates the head entry of
> >> the
> >> descriptor chain describing the buffer (this matches an entry
> >> placed in the available ring by the guest earlier), and \field{len} the
> >> total
> >> -of bytes written into the buffer. The latter is extremely useful
> >> +of bytes written into the buffer.
> >> +
> >> +\begin{note}
> >> +\field{len} is extremely useful
> >
> > just "useful" maybe?
>
> OK.
>
> >> for drivers using untrusted buffers: if you do not know exactly
> >
> > replace "you" with "driver" here?
>
> Yep.
>
> >> -how much has been written by the device, you usually have to zero
> >> -the buffer to ensure no data leakage occurs.
> >> +how much has been written by the device, a driver would have to zero
> >> +the buffer in advance to ensure no data leakage occurs.
> >> +
> >> +For example, a network driver
> >
> > any driver really, right?
>
> Well, the block device has an explicit status byte, and an fixed length.
>
> But there's a subtler detail I was considering when I designed this.
>
> Imagine a Xen-style "driver domain" which is actually your device; it's
> *untrusted*. This is possible if the (trusted) host does that actual
> data transfer, *and* reports the length; and such a mechanism is
> generic, so the host doesn't need to whether this is a block, net, or
> other device.
>
> (Imagine the device-guest has R/O mapping of the avail ring and
> descriptor table. Ignoring indirect descriptors you only need a "copy
> this data to/from this avail entry" helper to make this work).
>
> > How about something like this:
> >
> > +The device MUST write at least \field{len} bytes to descriptor,
> > +beginning at the first device-writable buffer,
> > +prior to updating the used index field.
> > +The device MAY write more than \field{len} bytes to descriptor.
> > +The driver MUST NOT make assumptions about data in the buffer pointed to
> > +by the descriptor with WRITE flag
> > +beyond the first \field{len} bytes: the data
> > +might be unchanged by the device, or it might be
> > +overwritten by the device.
> > +The driver SHOULD ignore data beyond the first \field{len} bytes.
>
> I like these, as long as we note that this MAY is to allow error cases,
> otherwise people might think they should just set len to zero.
>
> Here it is, using the device-writable terminology, and explicitly
> requiring that the device must set len (otherwise the requirements
> about the device obeying len makes it look like it's set by the driver):
>
> diff --git a/content.tex b/content.tex
> index 6ba079d..2c946a5 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -600,10 +600,19 @@ them: it is only written to by the device, and read by
> the driver.
> Each entry in the ring is a pair: \field{id} indicates the head entry of the
> descriptor chain describing the buffer (this matches an entry
> placed in the available ring by the guest earlier), and \field{len} the total
> -of bytes written into the buffer. The latter is extremely useful
> -for drivers using untrusted buffers: if you do not know exactly
> -how much has been written by the device, you usually have to zero
> -the buffer to ensure no data leakage occurs.
> +of bytes written into the buffer.
> +
> +\begin{note}
> +\field{len} is useful
> +for drivers using untrusted buffers: if a driver does not know exactly
> +how much has been written by the device, the driver would have to zero
> +the buffer in advance to ensure no data leakage occurs.
> +
> +For example, a network driver may hand a received buffer directly to
> +an unprivileged userspace application. If the network device has not
> +overwritten the bytes which were in that buffer, this may leak the
> +contents of freed memory from other processes to the application.
> +\end{note}
>
> \begin{note}
> The legacy \hyperref[intro:Virtio PCI Draft]{[Virtio PCI Draft]}
> @@ -612,6 +621,28 @@ the constant as VRING_USED_F_NO_NOTIFY, but the layout
> and value were
> identical.
> \end{note}
>
> +\devicenormative{\subsubsection}{Virtqueue Notification Suppression}{Basic
> Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> +
> +The device MUST set \field{len} prior to updating the used \field{idx}.
> +
> +The device MUST write at least \field{len} bytes to descriptor,
> +beginning at the first device-writable buffer,
> +prior to updating the used \field{idx}.
> +
> +The device MAY write more than \field{len} bytes to descriptor.
> +
> +\begin{note}
> +There are potential error cases where a device might not know what
> +parts of the buffers have been written. This is why \field{len} may
> +be an underestimate, but that's preferable to the driver believing
> +that uninitialized memory has been overwritten when it has not.
> +\end{note}
> +
> +\drivernormative{\subsubsection}{Virtqueue Notification Suppression}{Basic
> Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> +
> +The driver MUST NOT make assumptions about data in device-writable buffers
> +beyond the first \field{len} bytes, and SHOULD ignore it.
it -> this data.
Otherwise on first reading I thought "it" refers to len field.
> +
> \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
Sounds good, let's move discussion to virtio/virtio-dev now?
I think it's 1.1 material - agree?
--
MST
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., (continued)
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Michael S. Tsirkin, 2015/03/11
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Fam Zheng, 2015/03/11
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Michael S. Tsirkin, 2015/03/11
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Rusty Russell, 2015/03/11
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Michael S. Tsirkin, 2015/03/11
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Rusty Russell, 2015/03/11
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Michael S. Tsirkin, 2015/03/12
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Rusty Russell, 2015/03/12
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Michael S. Tsirkin, 2015/03/13
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Rusty Russell, 2015/03/15
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu.,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Cornelia Huck, 2015/03/16
- Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Rusty Russell, 2015/03/19
Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu., Michael S. Tsirkin, 2015/03/18