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

[Laszlo Ersek]

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 <address@hidden>
> 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: address@hidden
> Cc: address@hidden
> Cc: Laszlo Ersek <address@hidden>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  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 <address@hidden>