From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXNBf-0007Mk-3o for qemu-devel@nongnu.org; Mon, 16 Mar 2015 01:03:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXNBY-0004Lg-6k for qemu-devel@nongnu.org; Mon, 16 Mar 2015 01:03:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXNBX-0004LZ-Vj for qemu-devel@nongnu.org; Mon, 16 Mar 2015 01:03:28 -0400 Date: Mon, 16 Mar 2015 06:03:24 +0100 From: "Michael S. Tsirkin" Message-ID: <20150316060034-mutt-send-email-mst@redhat.com> References: <20150311061935.GA13037@redhat.com> <20150311064747.GF1437@ad.nay.redhat.com> <20150311074938-mutt-send-email-mst@redhat.com> <87pp8fyddj.fsf@rustcorp.com.au> <20150311133242-mutt-send-email-mst@redhat.com> <87bnjzxbz0.fsf@rustcorp.com.au> <20150312073038-mutt-send-email-mst@redhat.com> <87bnjxwva9.fsf@rustcorp.com.au> <20150313143624-mutt-send-email-mst@redhat.com> <87bnjtmy5t.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87bnjtmy5t.fsf@rustcorp.com.au> Subject: Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Rusty Russell Cc: Fam Zheng , QEMU Developers , stefanha@redhat.com On Mon, Mar 16, 2015 at 01:44:22PM +1030, Rusty Russell wrote: > "Michael S. Tsirkin" 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