[Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Michael S. Tsirkin]

We leak cpu mappings when 1st s/g is not exactly the
header. As we don't set ANY_LAYOUT, we can at this point
simply assert the correct length.

This will have to be fixed once ANY_LAYOUT is set.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Untested: posting for early feedback.

 hw/block/virtio-blk.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b19b102..1404b3f 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -381,6 +381,12 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
         exit(1);
     }
 
+    /* We don't advertize ANY_LAYOUT, so first s/g is exactly the header. */
+    if (iov[0].iov_len != sizeof(req->out)) {
+        error_report("virtio-blk request outhdr too long");
+        exit(1);
+    }
+
     iov_discard_front(&iov, &out_num, sizeof(req->out));
 
     if (in_num < 1 ||
-- 
MST

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Stefan Hajnoczi]

On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> We leak cpu mappings when 1st s/g is not exactly the
> header. As we don't set ANY_LAYOUT, we can at this point
> simply assert the correct length.
>
> This will have to be fixed once ANY_LAYOUT is set.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Untested: posting for early feedback.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Michael S. Tsirkin]

On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote:
> On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > We leak cpu mappings when 1st s/g is not exactly the
> > header. As we don't set ANY_LAYOUT, we can at this point
> > simply assert the correct length.
> >
> > This will have to be fixed once ANY_LAYOUT is set.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Untested: posting for early feedback.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Stefan, you are going to merge this for 2.2?
Peter, if Stefan is merging this one, we can
take Jason's patch for -net and that should be
enough for 2.2.

-- 
MST

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Fam Zheng]

On Thu, 11/27 23:13, Michael S. Tsirkin wrote:
> On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote:
> > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > We leak cpu mappings when 1st s/g is not exactly the
> > > header. As we don't set ANY_LAYOUT, we can at this point
> > > simply assert the correct length.
> > >
> > > This will have to be fixed once ANY_LAYOUT is set.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >
> > > Untested: posting for early feedback.
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Stefan, you are going to merge this for 2.2?
> Peter, if Stefan is merging this one, we can
> take Jason's patch for -net and that should be
> enough for 2.2.

My test bot saw a failure of "make check":

qemu-system-x86_64: virtio-blk request outhdr too long
Broken pipe
GTester: last random seed: R02S44c5a09d74c0603f0091ed20d1395121
qemu-system-x86_64: virtio-blk request outhdr too long
Broken pipe
GTester: last random seed: R02Sfdf270c8e1ed75c1f2047e3f0fb89b88
qemu-system-x86_64: virtio-blk request outhdr too long
Broken pipe
GTester: last random seed: R02Sd292c540929b963ed9d7d155f3db5452
qemu-system-x86_64: virtio-blk request outhdr too long
Broken pipe
GTester: last random seed: R02Sfc775a34a844c39f376aa478eda7573f
[vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio extension. Task offloads will be emulated.
make: *** [check-qtest-x86_64] Error 1

Fam

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Jason Wang]

On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng <famz@redhat.com> wrote:
> On Thu, 11/27 23:13, Michael S. Tsirkin wrote:
>>  On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote:
>>  > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin 
>> <mst@redhat.com> wrote:
>>  > > We leak cpu mappings when 1st s/g is not exactly the
>>  > > header. As we don't set ANY_LAYOUT, we can at this point
>>  > > simply assert the correct length.
>>  > >
>>  > > This will have to be fixed once ANY_LAYOUT is set.
>>  > >
>>  > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>  > > ---
>>  > >
>>  > > Untested: posting for early feedback.
>>  > 
>>  > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>  
>>  Stefan, you are going to merge this for 2.2?
>>  Peter, if Stefan is merging this one, we can
>>  take Jason's patch for -net and that should be
>>  enough for 2.2.
> 
> My test bot saw a failure of "make check":
> 
> qemu-system-x86_64: virtio-blk request outhdr too long
> Broken pipe
> GTester: last random seed: R02S44c5a09d74c0603f0091ed20d1395121
> qemu-system-x86_64: virtio-blk request outhdr too long
> Broken pipe
> GTester: last random seed: R02Sfdf270c8e1ed75c1f2047e3f0fb89b88
> qemu-system-x86_64: virtio-blk request outhdr too long
> Broken pipe
> GTester: last random seed: R02Sd292c540929b963ed9d7d155f3db5452
> qemu-system-x86_64: virtio-blk request outhdr too long
> Broken pipe
> GTester: last random seed: R02Sfc775a34a844c39f376aa478eda7573f
> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio 
> extension. Task offloads will be emulated.
> make: *** [check-qtest-x86_64] Error 1
> 
> Fam

This probably because of the test itself.

But anyway all kinds of guests (especially Windows drivers) need 
to be tested.

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Stefan Hajnoczi]

On Fri, Nov 28, 2014 at 7:05 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng <famz@redhat.com> wrote:
>>
>> On Thu, 11/27 23:13, Michael S. Tsirkin wrote:
>>>
>>>  On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote:
>>>  > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin <mst@redhat.com>
>>> wrote:
>>>  > > We leak cpu mappings when 1st s/g is not exactly the
>>>  > > header. As we don't set ANY_LAYOUT, we can at this point
>>>  > > simply assert the correct length.
>>>  > >
>>>  > > This will have to be fixed once ANY_LAYOUT is set.
>>>  > >
>>>  > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>  > > ---
>>>  > >
>>>  > > Untested: posting for early feedback.
>>>  >  > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>   Stefan, you are going to merge this for 2.2?
>>>  Peter, if Stefan is merging this one, we can
>>>  take Jason's patch for -net and that should be
>>>  enough for 2.2.
>>
>>
>> My test bot saw a failure of "make check":
>>
>> qemu-system-x86_64: virtio-blk request outhdr too long
>> Broken pipe
>> GTester: last random seed: R02S44c5a09d74c0603f0091ed20d1395121
>> qemu-system-x86_64: virtio-blk request outhdr too long
>> Broken pipe
>> GTester: last random seed: R02Sfdf270c8e1ed75c1f2047e3f0fb89b88
>> qemu-system-x86_64: virtio-blk request outhdr too long
>> Broken pipe
>> GTester: last random seed: R02Sd292c540929b963ed9d7d155f3db5452
>> qemu-system-x86_64: virtio-blk request outhdr too long
>> Broken pipe
>> GTester: last random seed: R02Sfc775a34a844c39f376aa478eda7573f
>> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio extension.
>> Task offloads will be emulated.
>> make: *** [check-qtest-x86_64] Error 1
>>
>> Fam
>
>
> This probably because of the test itself.
>
> But anyway all kinds of guests (especially Windows drivers) need to be
> tested.

Right, the test case explicitly tests different descriptor layouts,
even though virtio-blk-pci does not set the ANY_LAYOUT feature bit.

Either the test case needs to check ANY_LAYOUT before using the
2-descriptor layout or it needs to expect QEMU to refuse (in this case
exit(1), which is not very graceful).

The quick fix is to skip the 2-descriptor layout tests and re-enable
them once virtio-blk actually supports ANY_LAYOUT.  Any objections?

Stefan

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Marc Marí]

El Fri, 28 Nov 2014 11:43:59 +0000
Stefan Hajnoczi <stefanha@gmail.com> escribió:
> On Fri, Nov 28, 2014 at 7:05 AM, Jason Wang <jasowang@redhat.com>
> wrote:
> >
> >
> > On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng <famz@redhat.com> wrote:
> >>
> >> On Thu, 11/27 23:13, Michael S. Tsirkin wrote:
> >>>
> >>>  On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote:
> >>>  > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin
> >>>  > <mst@redhat.com>
> >>> wrote:
> >>>  > > We leak cpu mappings when 1st s/g is not exactly the
> >>>  > > header. As we don't set ANY_LAYOUT, we can at this point
> >>>  > > simply assert the correct length.
> >>>  > >
> >>>  > > This will have to be fixed once ANY_LAYOUT is set.
> >>>  > >
> >>>  > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>  > > ---
> >>>  > >
> >>>  > > Untested: posting for early feedback.
> >>>  >  > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>>   Stefan, you are going to merge this for 2.2?
> >>>  Peter, if Stefan is merging this one, we can
> >>>  take Jason's patch for -net and that should be
> >>>  enough for 2.2.
> >>
> >>
> >> My test bot saw a failure of "make check":
> >>
> >> qemu-system-x86_64: virtio-blk request outhdr too long
> >> Broken pipe
> >> GTester: last random seed: R02S44c5a09d74c0603f0091ed20d1395121
> >> qemu-system-x86_64: virtio-blk request outhdr too long
> >> Broken pipe
> >> GTester: last random seed: R02Sfdf270c8e1ed75c1f2047e3f0fb89b88
> >> qemu-system-x86_64: virtio-blk request outhdr too long
> >> Broken pipe
> >> GTester: last random seed: R02Sd292c540929b963ed9d7d155f3db5452
> >> qemu-system-x86_64: virtio-blk request outhdr too long
> >> Broken pipe
> >> GTester: last random seed: R02Sfc775a34a844c39f376aa478eda7573f
> >> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
> >> extension. Task offloads will be emulated.
> >> make: *** [check-qtest-x86_64] Error 1
> >>
> >> Fam
> >
> >
> > This probably because of the test itself.
> >
> > But anyway all kinds of guests (especially Windows drivers) need to
> > be tested.
> 
> Right, the test case explicitly tests different descriptor layouts,
> even though virtio-blk-pci does not set the ANY_LAYOUT feature bit.
> 
> Either the test case needs to check ANY_LAYOUT before using the
> 2-descriptor layout or it needs to expect QEMU to refuse (in this case
> exit(1), which is not very graceful).
> 
> The quick fix is to skip the 2-descriptor layout tests and re-enable
> them once virtio-blk actually supports ANY_LAYOUT.  Any objections?
> 
> Stefan

To be compliant with the specs, the test should check for ANY_LAYOUT
feature before testing with 2 descriptor layout. I'll add it with the
other changes pending for the test.

Marc

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Peter Maydell]

On 28 November 2014 at 11:43, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> Right, the test case explicitly tests different descriptor layouts,
> even though virtio-blk-pci does not set the ANY_LAYOUT feature bit.
>
> Either the test case needs to check ANY_LAYOUT before using the
> 2-descriptor layout or it needs to expect QEMU to refuse (in this case
> exit(1), which is not very graceful).
>
> The quick fix is to skip the 2-descriptor layout tests and re-enable
> them once virtio-blk actually supports ANY_LAYOUT.  Any objections?

So what do we want to do with this for 2.2? We have I think
two choices:
 (1) say that this isn't causing problems in practice, and defer all
 this to 2.3
 (2) add something like this patch plus fix the 'make check' tests
 (but turning "maybe something misbehaves" into "qemu definitely
 blows up and exits" doesn't seem like a great improvement to me)

I started looking at virtio-blk initially because I wasn't sure
if we should fix the virtio-net issue in the core virtio code.
But since we've decided not to do that, whether virtio-blk's
problems are release-blockers or not is something that we can
decide on their own merits.

My current thought is that we don't need to address this for 2.2;
is there something I'm missing that means we shouldn't defer to 2.3?

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Michael S. Tsirkin]

On Fri, Nov 28, 2014 at 04:14:35PM +0000, Peter Maydell wrote:
> On 28 November 2014 at 11:43, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > Right, the test case explicitly tests different descriptor layouts,
> > even though virtio-blk-pci does not set the ANY_LAYOUT feature bit.
> >
> > Either the test case needs to check ANY_LAYOUT before using the
> > 2-descriptor layout or it needs to expect QEMU to refuse (in this case
> > exit(1), which is not very graceful).
> >
> > The quick fix is to skip the 2-descriptor layout tests and re-enable
> > them once virtio-blk actually supports ANY_LAYOUT.  Any objections?
> 
> So what do we want to do with this for 2.2? We have I think
> two choices:
>  (1) say that this isn't causing problems in practice, and defer all
>  this to 2.3
>  (2) add something like this patch plus fix the 'make check' tests
>  (but turning "maybe something misbehaves" into "qemu definitely
>  blows up and exits" doesn't seem like a great improvement to me)
> 
> I started looking at virtio-blk initially because I wasn't sure
> if we should fix the virtio-net issue in the core virtio code.
> But since we've decided not to do that, whether virtio-blk's
> problems are release-blockers or not is something that we can
> decide on their own merits.
> 
> My current thought is that we don't need to address this for 2.2;
> is there something I'm missing that means we shouldn't defer to 2.3?
> 
> thanks
> -- PMM

The result of this is host mapping leak.
What effect does this have? Can this DOS host?
If not, I agree.

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Peter Maydell]

On 30 November 2014 at 16:43, Michael S. Tsirkin <mst@redhat.com> wrote:
> The result of this is host mapping leak.
> What effect does this have? Can this DOS host?

I don't think we can DOS the host here.

If Xen, we crash (but you can't use virtio-blk with Xen anyway)
Otherwise, if you managed to get address_space_map() to hand you
the bounce-buffer (by asking for dma to something other than RAM)
then we'll either hit an assertion or just end up never allowing
dma to/from non-RAM ever again for this guest.
The usual case would be that this was dma to/from ram, in
which case it's harmless if the virtio-backend never wrote to
the memory, and will fail to update dirty bitmaps for migration
etc if the backend did write.

In any case I think that none of these outcomes are worse
than the "exit(1)" the current patch proposes.

-- PMM

Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header

[Michael S. Tsirkin]

On Mon, Dec 01, 2014 at 12:07:07PM +0000, Peter Maydell wrote:
> On 30 November 2014 at 16:43, Michael S. Tsirkin <mst@redhat.com> wrote:
> > The result of this is host mapping leak.
> > What effect does this have? Can this DOS host?
> 
> I don't think we can DOS the host here.
> 
> If Xen, we crash (but you can't use virtio-blk with Xen anyway)
> Otherwise, if you managed to get address_space_map() to hand you
> the bounce-buffer (by asking for dma to something other than RAM)
> then we'll either hit an assertion or just end up never allowing
> dma to/from non-RAM ever again for this guest.
> The usual case would be that this was dma to/from ram, in
> which case it's harmless if the virtio-backend never wrote to
> the memory, and will fail to update dirty bitmaps for migration
> etc if the backend did write.
> 
> In any case I think that none of these outcomes are worse
> than the "exit(1)" the current patch proposes.
> 
> -- PMM

Fair enough.
Pls disregard the patch then, and we'll fix it properly
for 2.3 when we set ANY_LAYOUT.