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@xxxxxxxxxx
> Cc: secalert@xxxxxxxxxx
> Cc: Laszlo Ersek <lersek@xxxxxxxxxx>
> Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> ---
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel