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

[Laszlo Ersek <lersek@redhat.com>]

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