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

Submitted by Gerd Hoffmann on Sept. 5, 2014, 9:33 a.m.

Details

Message ID 1409909600.20018.11.camel@nilsson.home.kraxel.org
State New
Headers show

Not browsing as part of any series.

Commit Message

Gerd Hoffmann Sept. 5, 2014, 9:33 a.m.
On Fr, 2014-09-05 at 11:06 +0200, Laszlo Ersek wrote:
> > 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 :)

Oops.  Here we go.

cheers,
  Gerd

Patch hide | download patch | download mbox

From 33c5c3d1736fd577fc1279a1f3c50d2414e98fe3 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 3 Sep 2014 15:50:08 +0200
Subject: [PATCH] spice: make sure we don't overflow ssd->buf

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 | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 66e2578..def7b52 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -334,11 +334,23 @@  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));
 
-    dprint(1, "%s/%d: %dx%d\n", __func__, ssd->qxl.id,
-           surface_width(ssd->ds), surface_height(ssd->ds));
+    surface_size = (uint64_t) surface_width(ssd->ds) *
+        surface_height(ssd->ds) * 4;
+    assert(surface_size > 0);
+    assert(surface_size < INT_MAX);
+    if (ssd->bufsize < surface_size) {
+        ssd->bufsize = surface_size;
+        g_free(ssd->buf);
+        ssd->buf = g_malloc(ssd->bufsize);
+    }
+
+    dprint(1, "%s/%d: %ux%u (size %" PRIu64 "/%d)\n", __func__, ssd->qxl.id,
+           surface_width(ssd->ds), surface_height(ssd->ds),
+           surface_size, ssd->bufsize);
 
     surface.format     = SPICE_SURFACE_FMT_32_xRGB;
     surface.width      = surface_width(ssd->ds);
@@ -369,8 +381,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 +505,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;
 }
 
-- 
1.8.3.1


Comments

On 09/05/14 11:33, Gerd Hoffmann wrote:
> On Fr, 2014-09-05 at 11:06 +0200, Laszlo Ersek wrote:
>>> > > 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 :)
> Oops.  Here we go.
> 
> cheers,
>   Gerd
> 
> 
> 0001-spice-make-sure-we-don-t-overflow-ssd-buf.patch
> 
> 
> From 33c5c3d1736fd577fc1279a1f3c50d2414e98fe3 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Wed, 3 Sep 2014 15:50:08 +0200
> Subject: [PATCH] spice: make sure we don't overflow ssd->buf
> 
> 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 | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 66e2578..def7b52 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -334,11 +334,23 @@ 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));
>  
> -    dprint(1, "%s/%d: %dx%d\n", __func__, ssd->qxl.id,
> -           surface_width(ssd->ds), surface_height(ssd->ds));
> +    surface_size = (uint64_t) surface_width(ssd->ds) *
> +        surface_height(ssd->ds) * 4;
> +    assert(surface_size > 0);
> +    assert(surface_size < INT_MAX);
> +    if (ssd->bufsize < surface_size) {
> +        ssd->bufsize = surface_size;
> +        g_free(ssd->buf);
> +        ssd->buf = g_malloc(ssd->bufsize);
> +    }
> +
> +    dprint(1, "%s/%d: %ux%u (size %" PRIu64 "/%d)\n", __func__, ssd->qxl.id,
> +           surface_width(ssd->ds), surface_height(ssd->ds),
> +           surface_size, ssd->bufsize);
>  
>      surface.format     = SPICE_SURFACE_FMT_32_xRGB;
>      surface.width      = surface_width(ssd->ds);
> @@ -369,8 +381,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 +505,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;
>  }
>  
> -- 1.8.3.1
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>