[Qemu-devel] CVE-2011-2212: has it been actually fixed?

[Qemu-devel] CVE-2011-2212: has it been actually fixed?

[Michael Tokarev]

I come across a patch in ububtu qemu-kvm package, this:

From: Nelson Elhage <nelhage@ksplice.com>
Date: Thu, 19 May 2011 13:23:17 -0400
Subject: [PATCH] virtqueue: Sanity-check the length of indirect descriptors.

We were previously allowing arbitrarily-long descriptors, which could lead to a
buffer overflow in the qemu-kvm process.

Index: qemu-kvm-1.1~rc+dfsg/hw/virtio.c
===================================================================
--- qemu-kvm-1.1~rc+dfsg.orig/hw/virtio.c       2012-06-01 01:19:22.000000000 +0000
+++ qemu-kvm-1.1~rc+dfsg/hw/virtio.c    2012-06-12 19:31:02.336250076 +0000
@@ -370,6 +370,11 @@
             max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
             num_bufs = i = 0;
             desc_pa = vring_desc_addr(desc_pa, i);
+
+            if (max > VIRTQUEUE_MAX_SIZE) {
+                error_report("Too-large indirect descriptor");
+                exit(1);
+            }
         }

         do {
@@ -443,6 +448,11 @@
         max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
         desc_pa = vring_desc_addr(desc_pa, i);
         i = 0;
+
+        if (max > VIRTQUEUE_MAX_SIZE) {
+            error_report("Too-large indirect descriptor");
+            exit(1);
+        }
     }

     /* Collect all the descriptors */


And I wonder if it is still needed.  The mentioned CVE-2011-2212
has been fixed before 0.15, by the following:


commit c8eac1cfa1e9104a658b4614ada758861b8d823a
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Mon Jun 20 13:42:27 2011 +0300

    virtio: fix indirect descriptor buffer overflow

    We were previously allowing arbitrarily-long indirect descriptors, which
    could lead to a buffer overflow in qemu-kvm process.

    CVE-2011-2212

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

diff --git a/hw/virtio.c b/hw/virtio.c
index cc47a06..a8f4940 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -449,9 +449,17 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
         struct iovec *sg;

         if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
+            if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
+                error_report("Too many write descriptors in indirect table");
+                exit(1);
+            }
             elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i);
             sg = &elem->in_sg[elem->in_num++];
         } else {
+            if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
+                error_report("Too many read descriptors in indirect table");
+                exit(1);
+            }
             elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i);
             sg = &elem->out_sg[elem->out_num++];
         }


But this one - apparently - fixes a different codepath, no?

Thanks,

/mjt

Re: [Qemu-devel] CVE-2011-2212: has it been actually fixed?

[Anthony Liguori]

On 07/07/2012 08:37 AM, Michael Tokarev wrote:
> I come across a patch in ububtu qemu-kvm package, this:
>
> From: Nelson Elhage<nelhage@ksplice.com>
> Date: Thu, 19 May 2011 13:23:17 -0400
> Subject: [PATCH] virtqueue: Sanity-check the length of indirect descriptors.
>
> We were previously allowing arbitrarily-long descriptors, which could lead to a
> buffer overflow in the qemu-kvm process.

I don't have the original thread handy, but while the CVE was still embargoed, 
we made some changes to Nelson's original patch which is what led to Michael's 
patch.

We had a test case for the bug and confirmed that Michael's patch fixed that 
test case.

Regards,

Anthony Liguori

>
> Index: qemu-kvm-1.1~rc+dfsg/hw/virtio.c
> ===================================================================
> --- qemu-kvm-1.1~rc+dfsg.orig/hw/virtio.c       2012-06-01 01:19:22.000000000 +0000
> +++ qemu-kvm-1.1~rc+dfsg/hw/virtio.c    2012-06-12 19:31:02.336250076 +0000
> @@ -370,6 +370,11 @@
>               max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
>               num_bufs = i = 0;
>               desc_pa = vring_desc_addr(desc_pa, i);
> +
> +            if (max>  VIRTQUEUE_MAX_SIZE) {
> +                error_report("Too-large indirect descriptor");
> +                exit(1);
> +            }
>           }
>
>           do {
> @@ -443,6 +448,11 @@
>           max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
>           desc_pa = vring_desc_addr(desc_pa, i);
>           i = 0;
> +
> +        if (max>  VIRTQUEUE_MAX_SIZE) {
> +            error_report("Too-large indirect descriptor");
> +            exit(1);
> +        }
>       }
>
>       /* Collect all the descriptors */
>
>
> And I wonder if it is still needed.  The mentioned CVE-2011-2212
> has been fixed before 0.15, by the following:
>
>
> commit c8eac1cfa1e9104a658b4614ada758861b8d823a
> Author: Michael S. Tsirkin<mst@redhat.com>
> Date:   Mon Jun 20 13:42:27 2011 +0300
>
>      virtio: fix indirect descriptor buffer overflow
>
>      We were previously allowing arbitrarily-long indirect descriptors, which
>      could lead to a buffer overflow in qemu-kvm process.
>
>      CVE-2011-2212
>
>      Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>
> diff --git a/hw/virtio.c b/hw/virtio.c
> index cc47a06..a8f4940 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -449,9 +449,17 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>           struct iovec *sg;
>
>           if (vring_desc_flags(desc_pa, i)&  VRING_DESC_F_WRITE) {
> +            if (elem->in_num>= ARRAY_SIZE(elem->in_sg)) {
> +                error_report("Too many write descriptors in indirect table");
> +                exit(1);
> +            }
>               elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i);
>               sg =&elem->in_sg[elem->in_num++];
>           } else {
> +            if (elem->out_num>= ARRAY_SIZE(elem->out_sg)) {
> +                error_report("Too many read descriptors in indirect table");
> +                exit(1);
> +            }
>               elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i);
>               sg =&elem->out_sg[elem->out_num++];
>           }
>
>
> But this one - apparently - fixes a different codepath, no?
>
> Thanks,
>
> /mjt
>