Re: [Qemu-devel] [PATCH v2 for-2.3] virtio-blk: correctly dirty guest memory

[Paolo Bonzini <pbonzini@redhat.com>]

On 02/04/2015 20:46, Michael S. Tsirkin wrote:
> Oh, true in fact.  It might be a good idea to add something like this to
> the commit log:
> 
> 	Additionally, virtio spec requires that device writes at least
> 	len bytes to descriptor - so that driver can rely on
> 	bytes 0..len-1 being initialized by device. Specifically, it says
> 	len can be used as an optimization "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".
> 
> 	We violated this rule in two cases: on write - len should be 0,
> 	request size was mistakenly used

Should be 1 due to the status byte.

> - and on read error - we don't
> 	know whether the whole request size was written, so again len
> 	should be set to 0.

Oh no wait... my patch does not handle the read error case.

The len argument to virtqueue_push is being overloaded with two meanings:

1) a value that is >= the actual count, used to set the dirty bitmap

2) a value that should be <= the actual count, used as mentioned in your
English text above.

This is a problem for read errors, because the status byte is at the end
of the input buffers.  So (1) requires that you set len = size+1, while
(2) requires that you set len = 0.

My patch only deals with (1), which is a correctness problem for
migration, as Wen debugged.  It is a 2.3 regression.

I don't think (2) is fixable without changing the virtqueue API, and it
is not a regression.

Paolo