[Qemu-devel] [PATCH] virtio: check vring descriptor buffer length

[Qemu-devel] [PATCH] virtio: check vring descriptor buffer length

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

virtio back end uses set of buffers to facilitate I/O operations.
An infinite loop unfolds in virtqueue_pop() if a buffer was
of zero size. Add check to avoid it.

Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/virtio/virtio.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 30ede3d..8de896c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -457,6 +457,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
     unsigned num_sg = *p_num_sg;
     assert(num_sg <= max_num_sg);
 
+    if (!sz) {
+        error_report("virtio: zero sized buffers are not allowed");
+        exit(1);
+    }
+
     while (sz) {
         hwaddr len = sz;
 
-- 
2.5.5

Re: [Qemu-devel] [PATCH] virtio: check vring descriptor buffer length

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

On 07/27/2016 09:37 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> virtio back end uses set of buffers to facilitate I/O operations.
> An infinite loop unfolds in virtqueue_pop() if a buffer was
> of zero size. Add check to avoid it.
> 
> Reported-by: Li Qiang <liqiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/virtio/virtio.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 30ede3d..8de896c 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -457,6 +457,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
>      unsigned num_sg = *p_num_sg;
>      assert(num_sg <= max_num_sg);
>  
> +    if (!sz) {
> +        error_report("virtio: zero sized buffers are not allowed");
> +        exit(1);
> +    }

This lets the guest forcefully exit qemu.  Isn't it better to just make
the guest error degrade the virtio device into a broken state (the guest
can no longer use it, but qemu doesn't exit)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] virtio: check vring descriptor buffer length

[Michael S. Tsirkin]

On Wed, Jul 27, 2016 at 10:13:04AM -0600, Eric Blake wrote:
> On 07/27/2016 09:37 AM, P J P wrote:
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> > 
> > virtio back end uses set of buffers to facilitate I/O operations.
> > An infinite loop unfolds in virtqueue_pop() if a buffer was
> > of zero size. Add check to avoid it.
> > 
> > Reported-by: Li Qiang <liqiang6-s@360.cn>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> >  hw/virtio/virtio.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 30ede3d..8de896c 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -457,6 +457,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
> >      unsigned num_sg = *p_num_sg;
> >      assert(num_sg <= max_num_sg);
> >  
> > +    if (!sz) {
> > +        error_report("virtio: zero sized buffers are not allowed");
> > +        exit(1);
> > +    }
> 
> This lets the guest forcefully exit qemu.  Isn't it better to just make
> the guest error degrade the virtio device into a broken state (the guest
> can no longer use it, but qemu doesn't exit)?
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

ATM that's how other guest bugs are handled so I think it's fine.
With a kernel driver it's easy for guest to forcefully exit qemu anyway.
Hey just call halt().
We do want to improve this over time, and in particular supporting dpdk
within guest will need that, but I don't think we should block this fix
because of that.

-- 
MST

Re: [Qemu-devel] [PATCH] virtio: check vring descriptor buffer length

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Wed, Jul 27, 2016 at 09:07:56PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> virtio back end uses set of buffers to facilitate I/O operations.
> An infinite loop unfolds in virtqueue_pop() if a buffer was
> of zero size. Add check to avoid it.
> 
> Reported-by: Li Qiang <liqiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/virtio/virtio.c | 5 +++++
>  1 file changed, 5 insertions(+)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]