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