Re: [PATCH] used ring: define the meaning and requirements of the len field.

[Rusty Russell <rusty@au1.ibm.com>]

"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.