[Spice-devel,CVE-2014-3615,v2,3/3] spice: make sure we don't overflow ssd->buf

Submitted by Gerd Hoffmann on Sept. 4, 2014, 7:04 a.m.

Details

Message ID 1409814273-11463-4-git-send-email-kraxel@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Gerd Hoffmann Sept. 4, 2014, 7:04 a.m.
Related spice-only bug.  We have a fixed 16 MB buffer here, being
presented to the spice-server as qxl video memory in case spice is
used with a non-qxl card.  It's also used with qxl in vga mode.

When using display resolutions requiring more than 16 MB of memory we
are going to overflow that buffer.  In theory the guest can write,
indirectly via spice-server.  The spice-server clears the memory after
setting a new video mode though, triggering a segfault in the overflow
case, so qemu crashes before the guest has a chance to do something
evil.

Fix that by switching to dynamic allocation for the buffer.

CVE-2014-3615

Cc: qemu-stable@nongnu.org
Cc: secalert@redhat.com
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 66e2578..57a8fd0 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -334,6 +334,7 @@  void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
 void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
 {
     QXLDevSurfaceCreate surface;
+    uint64_t surface_size;
 
     memset(&surface, 0, sizeof(surface));
 
@@ -347,9 +348,18 @@  void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
     surface.mouse_mode = true;
     surface.flags      = 0;
     surface.type       = 0;
-    surface.mem        = (uintptr_t)ssd->buf;
     surface.group_id   = MEMSLOT_GROUP_HOST;
 
+    surface_size = surface.width * surface.height * 4;
+    if (ssd->bufsize < surface_size) {
+        ssd->bufsize = surface_size;
+        fprintf(stderr, "%s: %" PRId64 " (%dx%d)\n", __func__,
+                surface_size, surface.width, surface.height);
+        g_free(ssd->buf);
+        ssd->buf = g_malloc(ssd->bufsize);
+    }
+    surface.mem = (uintptr_t)ssd->buf;
+
     qemu_spice_create_primary_surface(ssd, 0, &surface, QXL_SYNC);
 }
 
@@ -369,8 +379,6 @@  void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd)
     if (ssd->num_surfaces == 0) {
         ssd->num_surfaces = 1024;
     }
-    ssd->bufsize = (16 * 1024 * 1024);
-    ssd->buf = g_malloc(ssd->bufsize);
 }
 
 /* display listener callbacks */
@@ -495,7 +503,7 @@  static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
     info->num_memslots = NUM_MEMSLOTS;
     info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
     info->internal_groupslot_id = 0;
-    info->qxl_ram_size = ssd->bufsize;
+    info->qxl_ram_size = 16 * 1024 * 1024;
     info->n_surfaces = ssd->num_surfaces;
 }
 

Comments

On 09/04/14 09:04, Gerd Hoffmann wrote:
> Related spice-only bug.  We have a fixed 16 MB buffer here, being
> presented to the spice-server as qxl video memory in case spice is
> used with a non-qxl card.  It's also used with qxl in vga mode.
> 
> When using display resolutions requiring more than 16 MB of memory we
> are going to overflow that buffer.  In theory the guest can write,
> indirectly via spice-server.  The spice-server clears the memory after
> setting a new video mode though, triggering a segfault in the overflow
> case, so qemu crashes before the guest has a chance to do something
> evil.
> 
> Fix that by switching to dynamic allocation for the buffer.
> 
> CVE-2014-3615
> 
> Cc: qemu-stable@nongnu.org
> Cc: secalert@redhat.com
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/spice-display.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 66e2578..57a8fd0 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -334,6 +334,7 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
>  void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>  {
>      QXLDevSurfaceCreate surface;
> +    uint64_t surface_size;
>  
>      memset(&surface, 0, sizeof(surface));
>  
> @@ -347,9 +348,18 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>      surface.mouse_mode = true;
>      surface.flags      = 0;
>      surface.type       = 0;
> -    surface.mem        = (uintptr_t)ssd->buf;
>      surface.group_id   = MEMSLOT_GROUP_HOST;
>  
> +    surface_size = surface.width * surface.height * 4;

(1) surface.width and surface.height are uint32_t fields
[spice-server/server/spice.h]:

struct QXLDevSurfaceCreate {
    uint32_t width;
    uint32_t height;

uint32_t equals "unsigned int" in our case, hence the multiplication
will be carried out in "unsigned int" -- 32-bits. Given that the
dimensions here are under guest control, I suggest to write it as

    surface_size = (uint64_t)surface.width * surface.height * 4;

(2) However, I have a concern even that way.

The above multiplication (due to the *4) can overflow in uint64_t as well.

I can see that "surface.width" and "surface.height" come from
pixman_image_get_width() and pixman_image_get_height(), which seem to
return "int" values. Hence,

    (uint64_t)0x7FFF_FFFF * 0x7FFF_FFFF * 4 == 0xFFFF_FFFC_0000_0004

which is OK. But, what if pixman returns a negative value? Can we create
an image that big?

From qemu_spice_create_one_update():

    int bw, bh;

    bw       = rect->right - rect->left;
    bh       = rect->bottom - rect->top;

    dest = pixman_image_create_bits(PIXMAN_x8r8g8b8, bw, bh,
                                    (void *)update->bitmap, bw * 4);

where

typedef struct SPICE_ATTR_PACKED QXLRect {
    int32_t top;
    int32_t left;
    int32_t bottom;
    int32_t right;
} QXLRect;

....

I can't track this back far enough. I'd feel safer if you checked that
the multiplication can't overflow even in uint64_t.

(3) In addition:

> +    if (ssd->bufsize < surface_size) {
> +        ssd->bufsize = surface_size;

struct SimpleSpiceDisplay {
[...]
    int bufsize;

Meaning, even if the multiplication fits okay in an uint64_t, the above
assignment can overflow ssd->bufsize, because that's just an int.

> +        fprintf(stderr, "%s: %" PRId64 " (%dx%d)\n", __func__,
> +                surface_size, surface.width, surface.height);

(4) surface_size is uint64_t, it should be printed with PRIu64, not
PRId64. Similarly, surface.width and surface.height are uint32_t, they
should be printed with either %u or PRIu32, not %d.

> +        g_free(ssd->buf);
> +        ssd->buf = g_malloc(ssd->bufsize);

Then, g_malloc() takes a "gsize", which means "unsigned long":

https://developer.gnome.org/glib/2.28/glib-Memory-Allocation.html#g-malloc
https://developer.gnome.org/glib/2.28/glib-Basic-Types.html#gsize

which has the range of uint32_t in an ILP32 (i686) build. Therefore even
changing the type of ssd->bufsize to uint64_t wouldn't help.

(5) Instead, you really need to make sure that the very first
multiplication fits in a signed int:

    int width, height;

    width = surface_width(ssd->ds);
    height = surface_height(ssd->ds);

    if (width <= 0 || height <= 0 ||
        width > INT_MAX / 4 ||
        height > INT_MAX / (width * 4)) {
        /* won't ever fit */
        abort();
    }

    if (ssd->bufsize < width * height * 4) {
        ssd->bufsize = width * height * 4;
        /* do the rest of the realloc */
    }

and do everything else after, even the population of "surface"'s fields.

> +    }
> +    surface.mem = (uintptr_t)ssd->buf;
> +
>      qemu_spice_create_primary_surface(ssd, 0, &surface, QXL_SYNC);
>  }
>  
> @@ -369,8 +379,6 @@ void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd)
>      if (ssd->num_surfaces == 0) {
>          ssd->num_surfaces = 1024;
>      }
> -    ssd->bufsize = (16 * 1024 * 1024);
> -    ssd->buf = g_malloc(ssd->bufsize);
>  }

I'm okay with this as long as you guarantee that the dimensions the
guest specifies will be strictly greater than zero. Otherwise, the
product could be zero, and I quite dislike g_malloc(0) calls -- "If
n_bytes is 0 it returns NULL", which could be problematic elsewhere.

The code I proposed above makes sure that both dimensions are positive.

>  
>  /* display listener callbacks */
> @@ -495,7 +503,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>      info->num_memslots = NUM_MEMSLOTS;
>      info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
>      info->internal_groupslot_id = 0;
> -    info->qxl_ram_size = ssd->bufsize;
> +    info->qxl_ram_size = 16 * 1024 * 1024;
>      info->n_surfaces = ssd->num_surfaces;
>  }

Is it safe to detach these two from each other? You could actually leave
the initial 16MB alloc in place.

Thanks
Laszlo
Hi,

> I can't track this back far enough. I'd feel safer if you checked that
> the multiplication can't overflow even in uint64_t.

Effectively it comes from the emulated graphics hardware (anything in
hw/display/*).  The gfx emulation must make sure that the framebuffer
fits into the video memory, which in turn pretty much implies that we
can't overflow uint64_t.  I think even uint32_t can't overflow with the
gfx hardware we are emulating today.

> (5) Instead, you really need to make sure that the very first
> multiplication fits in a signed int:

Makes sense.  I think it is easier to just multiply in 64bit, then check
the result is small enougth (new patch attached).

> >  /* display listener callbacks */
> > @@ -495,7 +503,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
> >      info->num_memslots = NUM_MEMSLOTS;
> >      info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
> >      info->internal_groupslot_id = 0;
> > -    info->qxl_ram_size = ssd->bufsize;
> > +    info->qxl_ram_size = 16 * 1024 * 1024;
> >      info->n_surfaces = ssd->num_surfaces;
> >  }

spice-server doesn't do anything with it, other than passing to
spice-client.  Not fully sure what spice-client uses this for, maybe as
some kind of hint for resource management.  Maybe not at all.

It surely doesn't matter at all for ssd->buf size.

cheers,
  Gerd
On 09/05/14 10:58, Gerd Hoffmann wrote:
>   Hi,
> 
>> I can't track this back far enough. I'd feel safer if you checked that
>> the multiplication can't overflow even in uint64_t.
> 
> Effectively it comes from the emulated graphics hardware (anything in
> hw/display/*).  The gfx emulation must make sure that the framebuffer
> fits into the video memory, which in turn pretty much implies that we
> can't overflow uint64_t.  I think even uint32_t can't overflow with the
> gfx hardware we are emulating today.
> 
>> (5) Instead, you really need to make sure that the very first
>> multiplication fits in a signed int:
> 
> Makes sense.  I think it is easier to just multiply in 64bit, then check
> the result is small enougth (new patch attached).

Okay, if you can guarantee that the product fits in uint64_t, then such
a check would suffice.

New patch has not been attached though :)

> 
>>>  /* display listener callbacks */
>>> @@ -495,7 +503,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>>>      info->num_memslots = NUM_MEMSLOTS;
>>>      info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
>>>      info->internal_groupslot_id = 0;
>>> -    info->qxl_ram_size = ssd->bufsize;
>>> +    info->qxl_ram_size = 16 * 1024 * 1024;
>>>      info->n_surfaces = ssd->num_surfaces;
>>>  }
> 
> spice-server doesn't do anything with it, other than passing to
> spice-client.  Not fully sure what spice-client uses this for, maybe as
> some kind of hint for resource management.  Maybe not at all.
> 
> It surely doesn't matter at all for ssd->buf size.

Okay, I'll trust you on this.

Thanks
Laszlo