diff --git a/SOURCES/0039-worker-validate-correctly-surfaces.patch b/SOURCES/0039-worker-validate-correctly-surfaces.patch new file mode 100644 index 0000000..f7a89d8 --- /dev/null +++ b/SOURCES/0039-worker-validate-correctly-surfaces.patch @@ -0,0 +1,136 @@ +From bea51d967731a2be7741fcbeef799b9fd925343a Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Wed, 9 Sep 2015 12:42:09 +0100 +Subject: [PATCH 1/2] worker: validate correctly surfaces + +Do not just give warning and continue to use an invalid index into +an array. + +Resolves: CVE-2015-5260 + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_worker.c | 33 ++++++++++++++++++--------------- + 1 file changed, 18 insertions(+), 15 deletions(-) + +diff --git a/server/red_worker.c b/server/red_worker.c +index 93e3398..c62dbcb 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -1054,6 +1054,7 @@ typedef struct BitmapData { + SpiceRect lossy_rect; + } BitmapData; + ++static inline int validate_surface(RedWorker *worker, uint32_t surface_id); + static void red_draw_qxl_drawable(RedWorker *worker, Drawable *drawable); + static void red_current_flush(RedWorker *worker, int surface_id); + #ifdef DRAW_ALL +@@ -1267,11 +1268,6 @@ static inline int is_primary_surface(RedWorker *worker, uint32_t surface_id) + return FALSE; + } + +-static inline void __validate_surface(RedWorker *worker, uint32_t surface_id) +-{ +- spice_warn_if(surface_id >= worker->n_surfaces); +-} +- + static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable) + { + DrawContext *context; +@@ -1280,7 +1276,7 @@ static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable) + /* surface_id must be validated before calling into + * validate_drawable_bbox + */ +- __validate_surface(worker, surface_id); ++ VALIDATE_SURFACE_RETVAL(worker, surface_id, FALSE); + context = &worker->surfaces[surface_id].context; + + if (drawable->bbox.top < 0) +@@ -1301,7 +1297,10 @@ static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable) + + static inline int validate_surface(RedWorker *worker, uint32_t surface_id) + { +- spice_warn_if(surface_id >= worker->n_surfaces); ++ if SPICE_UNLIKELY(surface_id >= worker->n_surfaces) { ++ spice_warning("invalid surface_id %u", surface_id); ++ return 0; ++ } + if (!worker->surfaces[surface_id].context.canvas) { + spice_warning("canvas address is %p for %d (and is NULL)\n", + &(worker->surfaces[surface_id].context.canvas), surface_id); +@@ -4304,12 +4303,14 @@ static inline void red_create_surface(RedWorker *worker, uint32_t surface_id,uin + static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface, + uint32_t group_id, int loadvm) + { +- int surface_id; ++ uint32_t surface_id; + RedSurface *red_surface; + uint8_t *data; + + surface_id = surface->surface_id; +- __validate_surface(worker, surface_id); ++ if SPICE_UNLIKELY(surface_id >= worker->n_surfaces) { ++ goto exit; ++ } + + red_surface = &worker->surfaces[surface_id]; + +@@ -4345,6 +4346,7 @@ static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface + default: + spice_error("unknown surface command"); + }; ++exit: + red_put_surface_cmd(surface); + free(surface); + } +@@ -11091,7 +11093,7 @@ void handle_dev_update(void *opaque, void *payload) + { + RedWorker *worker = opaque; + RedWorkerMessageUpdate *msg = payload; +- SpiceRect *rect = spice_new0(SpiceRect, 1); ++ SpiceRect *rect; + RedSurface *surface; + uint32_t surface_id = msg->surface_id; + const QXLRect *qxl_area = msg->qxl_area; +@@ -11099,17 +11101,16 @@ void handle_dev_update(void *opaque, void *payload) + QXLRect *qxl_dirty_rects = msg->qxl_dirty_rects; + uint32_t clear_dirty_region = msg->clear_dirty_region; + ++ VALIDATE_SURFACE_RET(worker, surface_id); ++ ++ rect = spice_new0(SpiceRect, 1); + surface = &worker->surfaces[surface_id]; + red_get_rect_ptr(rect, qxl_area); + flush_display_commands(worker); + + spice_assert(worker->running); + +- if (validate_surface(worker, surface_id)) { +- red_update_area(worker, rect, surface_id); +- } else { +- rendering_incorrect(__func__); +- } ++ red_update_area(worker, rect, surface_id); + free(rect); + + surface_dirty_region_to_rects(surface, qxl_dirty_rects, num_dirty_rects, +@@ -11148,6 +11149,7 @@ void handle_dev_del_memslot(void *opaque, void *payload) + * surface_id == 0, maybe move the assert upward and merge the two functions? */ + static inline void destroy_surface_wait(RedWorker *worker, int surface_id) + { ++ VALIDATE_SURFACE_RET(worker, surface_id); + if (!worker->surfaces[surface_id].context.canvas) { + return; + } +@@ -11412,6 +11414,7 @@ void handle_dev_create_primary_surface(void *opaque, void *payload) + + static void dev_destroy_primary_surface(RedWorker *worker, uint32_t surface_id) + { ++ VALIDATE_SURFACE_RET(worker, surface_id); + spice_warn_if(surface_id != 0); + + spice_debug(NULL); +-- +2.4.3 + diff --git a/SOURCES/0040-worker-avoid-double-free-or-double-create-of-surface.patch b/SOURCES/0040-worker-avoid-double-free-or-double-create-of-surface.patch new file mode 100644 index 0000000..2ef2398 --- /dev/null +++ b/SOURCES/0040-worker-avoid-double-free-or-double-create-of-surface.patch @@ -0,0 +1,46 @@ +From 269e9d112639ab6c54645de217c46ef75617d780 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Wed, 9 Sep 2015 12:45:06 +0100 +Subject: [PATCH 2/2] worker: avoid double free or double create of surfaces + +A driver can overwrite surface state creating a surface with the same +id of a previous one. +Also can try to destroy surfaces that are not created. +Both requests cause invalid internal states that could lead to crashes +or memory corruptions. + +Signed-off-by: Frediano Ziglio +--- + server/red_worker.c | 9 ++++++++- + 1 file changed, 8 insertions(+), 1 deletion(-) + +diff --git a/server/red_worker.c b/server/red_worker.c +index c62dbcb..a7eaab9 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -4320,6 +4320,10 @@ static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface + int32_t stride = surface->u.surface_create.stride; + int reloaded_surface = loadvm || (surface->flags & QXL_SURF_FLAG_KEEP_DATA); + ++ if (red_surface->refs) { ++ spice_warning("avoiding creating a surface twice"); ++ break; ++ } + data = surface->u.surface_create.data; + if (stride < 0) { + data -= (int32_t)(stride * (height - 1)); +@@ -4333,7 +4337,10 @@ static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface + break; + } + case QXL_SURFACE_CMD_DESTROY: +- spice_warn_if(!red_surface->context.canvas); ++ if (!red_surface->refs) { ++ spice_warning("avoiding destroying a surface twice"); ++ break; ++ } + set_surface_release_info(worker, surface_id, 0, surface->release_info, group_id); + red_handle_depends_on_target_surface(worker, surface_id); + /* note that red_handle_depends_on_target_surface must be called before red_current_clear. +-- +2.4.3 + diff --git a/SOURCES/0041-Define-a-constant-to-limit-data-from-guest.patch b/SOURCES/0041-Define-a-constant-to-limit-data-from-guest.patch new file mode 100644 index 0000000..d569778 --- /dev/null +++ b/SOURCES/0041-Define-a-constant-to-limit-data-from-guest.patch @@ -0,0 +1,42 @@ +From 247209c1f1c6a41d9fe0532ae17f19ae1cdcc2f7 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 11:58:11 +0100 +Subject: [PATCH 41/57] Define a constant to limit data from guest. + +This limit will prevent guest trying to do nasty things and DoS to host. + +Signed-off-by: Frediano Ziglio +--- + server/red_parse_qxl.c | 11 +++++++++++ + 1 file changed, 11 insertions(+) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 6c0b065..4449f2c 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -21,11 +21,22 @@ + + #include + #include ++#include + #include "common/lz_common.h" + #include "red_common.h" + #include "red_memslots.h" + #include "red_parse_qxl.h" + ++/* Max size in bytes for any data field used in a QXL command. ++ * This will for example be useful to prevent the guest from saturating the ++ * host memory if it tries to send overlapping chunks. ++ * This value should be big enough for all requests but limited ++ * to 32 bits. Even better if it fits on 31 bits to detect integer overflows. ++ */ ++#define MAX_DATA_CHUNK 0x7ffffffflu ++ ++G_STATIC_ASSERT(MAX_DATA_CHUNK <= G_MAXINT32); ++ + #if 0 + static void hexdump_qxl(RedMemSlotInfo *slots, int group_id, + QXLPHYSICAL addr, uint8_t bytes) +-- +2.4.3 + diff --git a/SOURCES/0042-Fix-some-integer-overflow-causing-large-memory-alloc.patch b/SOURCES/0042-Fix-some-integer-overflow-causing-large-memory-alloc.patch new file mode 100644 index 0000000..ff5e6af --- /dev/null +++ b/SOURCES/0042-Fix-some-integer-overflow-causing-large-memory-alloc.patch @@ -0,0 +1,69 @@ +From b9ee3c381ef823b6a4f246e0df3112efdd349b6a Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Thu, 17 Sep 2015 15:00:22 +0100 +Subject: [PATCH 42/57] Fix some integer overflow causing large memory + allocations + +Prevent integer overflow when computing image sizes. +Image index computations are done using 32 bit so this can cause easily +security issues. MAX_DATA_CHUNK is larger than the virtual +card limit, so this is not going to cause change in behaviours. +Comparing size calculation results with MAX_DATA_CHUNK will allow us to +catch overflows. +Prevent guest from allocating large amount of memory. + +Signed-off-by: Frediano Ziglio +--- + server/red_parse_qxl.c | 15 +++++++++++---- + 1 file changed, 11 insertions(+), 4 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 4449f2c..ceb2759 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -384,7 +384,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id, + QXLImage *qxl; + SpiceImage *red = NULL; + SpicePalette *rp = NULL; +- size_t bitmap_size, size; ++ uint64_t bitmap_size, size; + uint8_t qxl_flags; + int error; + +@@ -460,7 +460,10 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id, + red->u.bitmap.palette = rp; + red->u.bitmap.palette_id = rp->unique; + } +- bitmap_size = red->u.bitmap.y * abs(red->u.bitmap.stride); ++ bitmap_size = (uint64_t) red->u.bitmap.y * red->u.bitmap.stride; ++ if (bitmap_size > MAX_DATA_CHUNK) { ++ goto error; ++ } + if (qxl_flags & QXL_BITMAP_DIRECT) { + red->u.bitmap.data = red_get_image_data_flat(slots, group_id, + qxl->bitmap.data, +@@ -1220,7 +1223,7 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id, + RedSurfaceCmd *red, QXLPHYSICAL addr) + { + QXLSurfaceCmd *qxl; +- size_t size; ++ uint64_t size; + int error; + + qxl = (QXLSurfaceCmd *)get_virt(slots, addr, sizeof(*qxl), group_id, +@@ -1240,7 +1243,11 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id, + red->u.surface_create.width = qxl->u.surface_create.width; + red->u.surface_create.height = qxl->u.surface_create.height; + red->u.surface_create.stride = qxl->u.surface_create.stride; +- size = red->u.surface_create.height * abs(red->u.surface_create.stride); ++ /* the multiplication can overflow, also abs(-2^31) may return a negative value */ ++ size = (uint64_t) red->u.surface_create.height * abs(red->u.surface_create.stride); ++ if (size > MAX_DATA_CHUNK || red->u.surface_create.stride == G_MININT32) { ++ return 1; ++ } + red->u.surface_create.data = + (uint8_t*)get_virt(slots, qxl->u.surface_create.data, size, group_id, &error); + if (error) { +-- +2.4.3 + diff --git a/SOURCES/0043-Check-properly-surface-to-be-created.patch b/SOURCES/0043-Check-properly-surface-to-be-created.patch new file mode 100644 index 0000000..05c24d9 --- /dev/null +++ b/SOURCES/0043-Check-properly-surface-to-be-created.patch @@ -0,0 +1,78 @@ +From b15d9c6d94bf426d61aaa4631ed55271c0d12b14 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 16:02:59 +0100 +Subject: [PATCH 43/57] Check properly surface to be created + +Check format is valid. +Check stride is at least the size of required bytes for a row. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 35 ++++++++++++++++++++++++++++++++++- + 1 file changed, 34 insertions(+), 1 deletion(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index ceb2759..a7ca71d 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -1219,12 +1219,30 @@ void red_put_message(RedMessage *red) + /* nothing yet */ + } + ++static unsigned int surface_format_to_bpp(uint32_t format) ++{ ++ switch (format) { ++ case SPICE_SURFACE_FMT_1_A: ++ return 1; ++ case SPICE_SURFACE_FMT_8_A: ++ return 8; ++ case SPICE_SURFACE_FMT_16_555: ++ case SPICE_SURFACE_FMT_16_565: ++ return 16; ++ case SPICE_SURFACE_FMT_32_xRGB: ++ case SPICE_SURFACE_FMT_32_ARGB: ++ return 32; ++ } ++ return 0; ++} ++ + int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id, + RedSurfaceCmd *red, QXLPHYSICAL addr) + { + QXLSurfaceCmd *qxl; + uint64_t size; + int error; ++ unsigned int bpp; + + qxl = (QXLSurfaceCmd *)get_virt(slots, addr, sizeof(*qxl), group_id, + &error); +@@ -1243,9 +1261,24 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id, + red->u.surface_create.width = qxl->u.surface_create.width; + red->u.surface_create.height = qxl->u.surface_create.height; + red->u.surface_create.stride = qxl->u.surface_create.stride; ++ bpp = surface_format_to_bpp(red->u.surface_create.format); ++ ++ /* check if format is valid */ ++ if (!bpp) { ++ return 1; ++ } ++ ++ /* check stride is larger than required bytes */ ++ size = ((uint64_t) red->u.surface_create.width * bpp + 7u) / 8u; ++ /* the uint32_t conversion is here to avoid problems with -2^31 value */ ++ if (red->u.surface_create.stride == G_MININT32 ++ || size > (uint32_t) abs(red->u.surface_create.stride)) { ++ return 1; ++ } ++ + /* the multiplication can overflow, also abs(-2^31) may return a negative value */ + size = (uint64_t) red->u.surface_create.height * abs(red->u.surface_create.stride); +- if (size > MAX_DATA_CHUNK || red->u.surface_create.stride == G_MININT32) { ++ if (size > MAX_DATA_CHUNK) { + return 1; + } + red->u.surface_create.data = +-- +2.4.3 + diff --git a/SOURCES/0044-Fix-buffer-reading-overflow.patch b/SOURCES/0044-Fix-buffer-reading-overflow.patch new file mode 100644 index 0000000..bc0240d --- /dev/null +++ b/SOURCES/0044-Fix-buffer-reading-overflow.patch @@ -0,0 +1,38 @@ +From 18087073df84885642d9b0b1efd0e86e18409bbe Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:00:37 +0100 +Subject: [PATCH 44/57] Fix buffer reading overflow + +Not security risk as just for read. +However, this could be used to attempt integer overflows in the +following lines. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 9 ++++++++- + 1 file changed, 8 insertions(+), 1 deletion(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index a7ca71d..01cba0f 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -361,7 +361,14 @@ static const int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] = {0, 1, 1, 4, 4, 8, 16, 24, + + static int bitmap_consistent(SpiceBitmap *bitmap) + { +- int bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format]; ++ int bpp; ++ ++ if (bitmap->format >= SPICE_N_ELEMENTS(MAP_BITMAP_FMT_TO_BITS_PER_PIXEL)) { ++ spice_warning("wrong format specified for image\n"); ++ return FALSE; ++ } ++ ++ bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format]; + + if (bitmap->stride < ((bitmap->x * bpp + 7) / 8)) { + spice_error("image stride too small for width: %d < ((%d * %d + 7) / 8) (%s=%d)\n", +-- +2.4.3 + diff --git a/SOURCES/0045-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch b/SOURCES/0045-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch new file mode 100644 index 0000000..822ab6d --- /dev/null +++ b/SOURCES/0045-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch @@ -0,0 +1,48 @@ +From 7baa8c39757b46a834e20198e4b18e9f1752e20e Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 13:09:35 +0100 +Subject: [PATCH 45/57] Prevent 32 bit integer overflow in bitmap_consistent + +The overflow may lead to buffer overflow as the row size computed from +width (bitmap->x) can be bigger than the size in bytes (bitmap->stride). +This can make spice-server accept the invalid sizes. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 9 +++++---- + 1 file changed, 5 insertions(+), 4 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 01cba0f..3385f52 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -357,11 +357,12 @@ static const char *bitmap_format_to_string(int format) + return "unknown"; + } + +-static const int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] = {0, 1, 1, 4, 4, 8, 16, 24, 32, 32, 8}; ++static const unsigned int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] = ++ {0, 1, 1, 4, 4, 8, 16, 24, 32, 32, 8}; + + static int bitmap_consistent(SpiceBitmap *bitmap) + { +- int bpp; ++ unsigned int bpp; + + if (bitmap->format >= SPICE_N_ELEMENTS(MAP_BITMAP_FMT_TO_BITS_PER_PIXEL)) { + spice_warning("wrong format specified for image\n"); +@@ -370,8 +371,8 @@ static int bitmap_consistent(SpiceBitmap *bitmap) + + bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format]; + +- if (bitmap->stride < ((bitmap->x * bpp + 7) / 8)) { +- spice_error("image stride too small for width: %d < ((%d * %d + 7) / 8) (%s=%d)\n", ++ if (bitmap->stride < (((uint64_t) bitmap->x * bpp + 7u) / 8u)) { ++ spice_warning("image stride too small for width: %d < ((%d * %d + 7) / 8) (%s=%d)\n", + bitmap->stride, bitmap->x, bpp, + bitmap_format_to_string(bitmap->format), + bitmap->format); +-- +2.4.3 + diff --git a/SOURCES/0046-Fix-race-condition-on-red_get_clip_rects.patch b/SOURCES/0046-Fix-race-condition-on-red_get_clip_rects.patch new file mode 100644 index 0000000..279aeca --- /dev/null +++ b/SOURCES/0046-Fix-race-condition-on-red_get_clip_rects.patch @@ -0,0 +1,42 @@ +From 078a903d55f44aedd22b4fa8dd86e4b03b82c01c Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:01:51 +0100 +Subject: [PATCH 46/57] Fix race condition on red_get_clip_rects + +Do not read multiple time an array size that can be changed. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 8 +++++--- + 1 file changed, 5 insertions(+), 3 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 3385f52..affd3a2 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -273,6 +273,7 @@ static SpiceClipRects *red_get_clip_rects(RedMemSlotInfo *slots, int group_id, + size_t size; + int i; + int error; ++ uint32_t num_rects; + + qxl = (QXLClipRects *)get_virt(slots, addr, sizeof(*qxl), group_id, &error); + if (error) { +@@ -284,9 +285,10 @@ static SpiceClipRects *red_get_clip_rects(RedMemSlotInfo *slots, int group_id, + data = red_linearize_chunk(&chunks, size, &free_data); + red_put_data_chunks(&chunks); + +- spice_assert(qxl->num_rects * sizeof(QXLRect) == size); +- red = spice_malloc(sizeof(*red) + qxl->num_rects * sizeof(SpiceRect)); +- red->num_rects = qxl->num_rects; ++ num_rects = qxl->num_rects; ++ spice_assert(num_rects * sizeof(QXLRect) == size); ++ red = spice_malloc(sizeof(*red) + num_rects * sizeof(SpiceRect)); ++ red->num_rects = num_rects; + + start = (QXLRect*)data; + for (i = 0; i < red->num_rects; i++) { +-- +2.4.3 + diff --git a/SOURCES/0047-Fix-race-in-red_get_image.patch b/SOURCES/0047-Fix-race-in-red_get_image.patch new file mode 100644 index 0000000..abaf5b3 --- /dev/null +++ b/SOURCES/0047-Fix-race-in-red_get_image.patch @@ -0,0 +1,77 @@ +From d2fc5cee16c10e53d81c6251e6929da54270a6f4 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:04:10 +0100 +Subject: [PATCH 47/57] Fix race in red_get_image + +Do not read multiple times data from guest as this could be changed +by other vcpu threads. +This causes races and security problems if these data are used for +buffer allocation or checks. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 18 ++++++++++-------- + 1 file changed, 10 insertions(+), 8 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index affd3a2..84ea526 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -397,6 +397,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id, + uint64_t bitmap_size, size; + uint8_t qxl_flags; + int error; ++ QXLPHYSICAL palette; + + if (addr == 0) { + return NULL; +@@ -422,12 +423,16 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id, + switch (red->descriptor.type) { + case SPICE_IMAGE_TYPE_BITMAP: + red->u.bitmap.format = qxl->bitmap.format; +- if (!bitmap_fmt_is_rgb(qxl->bitmap.format) && !qxl->bitmap.palette && !is_mask) { ++ red->u.bitmap.x = qxl->bitmap.x; ++ red->u.bitmap.y = qxl->bitmap.y; ++ red->u.bitmap.stride = qxl->bitmap.stride; ++ palette = qxl->bitmap.palette; ++ if (!bitmap_fmt_is_rgb(red->u.bitmap.format) && !palette && !is_mask) { + spice_warning("guest error: missing palette on bitmap format=%d\n", + red->u.bitmap.format); + goto error; + } +- if (qxl->bitmap.x == 0 || qxl->bitmap.y == 0) { ++ if (red->u.bitmap.x == 0 || red->u.bitmap.y == 0) { + spice_warning("guest error: zero area bitmap\n"); + goto error; + } +@@ -435,23 +440,20 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id, + if (qxl_flags & QXL_BITMAP_TOP_DOWN) { + red->u.bitmap.flags = SPICE_BITMAP_FLAGS_TOP_DOWN; + } +- red->u.bitmap.x = qxl->bitmap.x; +- red->u.bitmap.y = qxl->bitmap.y; +- red->u.bitmap.stride = qxl->bitmap.stride; + if (!bitmap_consistent(&red->u.bitmap)) { + goto error; + } +- if (qxl->bitmap.palette) { ++ if (palette) { + QXLPalette *qp; + int i, num_ents; +- qp = (QXLPalette *)get_virt(slots, qxl->bitmap.palette, ++ qp = (QXLPalette *)get_virt(slots, palette, + sizeof(*qp), group_id, &error); + if (error) { + goto error; + } + num_ents = qp->num_ents; + if (!validate_virt(slots, (intptr_t)qp->ents, +- get_memslot_id(slots, qxl->bitmap.palette), ++ get_memslot_id(slots, palette), + num_ents * sizeof(qp->ents[0]), group_id)) { + goto error; + } +-- +2.4.3 + diff --git a/SOURCES/0048-Fix-race-condition-in-red_get_string.patch b/SOURCES/0048-Fix-race-condition-in-red_get_string.patch new file mode 100644 index 0000000..8bc6bb5 --- /dev/null +++ b/SOURCES/0048-Fix-race-condition-in-red_get_string.patch @@ -0,0 +1,62 @@ +From 932e27e50032c1c7032be3616217a2ab0586fe78 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:05:20 +0100 +Subject: [PATCH 48/57] Fix race condition in red_get_string + +Do not read multiple time an array size that can be changed. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 15 +++++++++------ + 1 file changed, 9 insertions(+), 6 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 84ea526..2d4636e 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -809,6 +809,7 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id, + size_t chunk_size, qxl_size, red_size, glyph_size; + int glyphs, bpp = 0, i; + int error; ++ uint16_t qxl_flags, qxl_length; + + qxl = (QXLString *)get_virt(slots, addr, sizeof(*qxl), group_id, &error); + if (error) { +@@ -825,13 +826,15 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id, + red_put_data_chunks(&chunks); + + qxl_size = qxl->data_size; ++ qxl_flags = qxl->flags; ++ qxl_length = qxl->length; + spice_assert(chunk_size == qxl_size); + +- if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A1) { ++ if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A1) { + bpp = 1; +- } else if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A4) { ++ } else if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A4) { + bpp = 4; +- } else if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A8) { ++ } else if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A8) { + bpp = 8; + } + spice_assert(bpp != 0); +@@ -848,11 +851,11 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id, + start = (QXLRasterGlyph*)(&start->data[glyph_size]); + } + spice_assert(start <= end); +- spice_assert(glyphs == qxl->length); ++ spice_assert(glyphs == qxl_length); + + red = spice_malloc(red_size); +- red->length = qxl->length; +- red->flags = qxl->flags; ++ red->length = qxl_length; ++ red->flags = qxl_flags; + + start = (QXLRasterGlyph*)data; + end = (QXLRasterGlyph*)(data + chunk_size); +-- +2.4.3 + diff --git a/SOURCES/0049-Fix-integer-overflow-computing-glyph_size-in-red_get.patch b/SOURCES/0049-Fix-integer-overflow-computing-glyph_size-in-red_get.patch new file mode 100644 index 0000000..9bf8b82 --- /dev/null +++ b/SOURCES/0049-Fix-integer-overflow-computing-glyph_size-in-red_get.patch @@ -0,0 +1,63 @@ +From e28c08d63490a2fb6b8cc07bf968eb16243e9c63 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:13:24 +0100 +Subject: [PATCH 49/57] Fix integer overflow computing glyph_size in + red_get_string + +If bpp is int the formula can lead to weird overflows. width and height +are uint16_t so the formula is: + + size_t = u16 * (u16 * int + const_int) / const_int; + +so it became + + size_t = (int) u16 * ((int) u16 * int + const_int) / const_int; + +However the (int) u16 * (int) u16 can then became negative to overflow. +Under 64 bit architectures size_t is 64 and int usually 32 so converting +this negative 32 bit number to a unsigned 64 bit lead to a very big +number as the signed is extended and then converted to unsigned. +Using unsigned arithmetic prevent extending the sign. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 8 +++++--- + 1 file changed, 5 insertions(+), 3 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 2d4636e..c4b82be 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -807,7 +807,9 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id, + uint8_t *data; + bool free_data; + size_t chunk_size, qxl_size, red_size, glyph_size; +- int glyphs, bpp = 0, i; ++ int glyphs, i; ++ /* use unsigned to prevent integer overflow in multiplication below */ ++ unsigned int bpp = 0; + int error; + uint16_t qxl_flags, qxl_length; + +@@ -846,7 +848,7 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id, + while (start < end) { + spice_assert((QXLRasterGlyph*)(&start->data[0]) <= end); + glyphs++; +- glyph_size = start->height * ((start->width * bpp + 7) / 8); ++ glyph_size = start->height * ((start->width * bpp + 7u) / 8u); + red_size += sizeof(SpiceRasterGlyph *) + SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4); + start = (QXLRasterGlyph*)(&start->data[glyph_size]); + } +@@ -867,7 +869,7 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id, + glyph->height = start->height; + red_get_point_ptr(&glyph->render_pos, &start->render_pos); + red_get_point_ptr(&glyph->glyph_origin, &start->glyph_origin); +- glyph_size = glyph->height * ((glyph->width * bpp + 7) / 8); ++ glyph_size = glyph->height * ((glyph->width * bpp + 7u) / 8u); + spice_assert((QXLRasterGlyph*)(&start->data[glyph_size]) <= end); + memcpy(glyph->data, start->data, glyph_size); + start = (QXLRasterGlyph*)(&start->data[glyph_size]); +-- +2.4.3 + diff --git a/SOURCES/0050-Fix-race-condition-in-red_get_data_chunks_ptr.patch b/SOURCES/0050-Fix-race-condition-in-red_get_data_chunks_ptr.patch new file mode 100644 index 0000000..71b0a38 --- /dev/null +++ b/SOURCES/0050-Fix-race-condition-in-red_get_data_chunks_ptr.patch @@ -0,0 +1,66 @@ +From 20979131dee0f81972ca159e5f754d42890e75e6 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 12:12:19 +0100 +Subject: [PATCH 50/57] Fix race condition in red_get_data_chunks_ptr + +Do not read multiple times data from guest as this can be changed by +other guest vcpus. This causes races and security problems if these +data are used for buffer allocation or checks. + +Actually, the 'data' member can't change during read as it is just a +pointer to a fixed array contained in qxl. However, this change will +make it clear that there can be no race condition. + +Signed-off-by: Frediano Ziglio +--- + server/red_parse_qxl.c | 17 ++++++++++------- + 1 file changed, 10 insertions(+), 7 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index c4b82be..7cc20e6 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -102,30 +102,33 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id, + RedDataChunk *red_prev; + size_t data_size = 0; + int error; ++ QXLPHYSICAL next_chunk; + + red->data_size = qxl->data_size; + data_size += red->data_size; +- if (!validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id)) { ++ red->data = qxl->data; ++ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) { ++ red->data = NULL; + return 0; + } +- red->data = qxl->data; + red->prev_chunk = NULL; + +- while (qxl->next_chunk) { ++ while ((next_chunk = qxl->next_chunk) != 0) { + red_prev = red; + red = spice_new(RedDataChunk, 1); +- memslot_id = get_memslot_id(slots, qxl->next_chunk); +- qxl = (QXLDataChunk *)get_virt(slots, qxl->next_chunk, sizeof(*qxl), group_id, ++ memslot_id = get_memslot_id(slots, next_chunk); ++ qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id, + &error); + if (error) { + return 0; + } + red->data_size = qxl->data_size; + data_size += red->data_size; +- if (!validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id)) { ++ red->data = qxl->data; ++ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) { ++ red->data = NULL; + return 0; + } +- red->data = qxl->data; + red->prev_chunk = red_prev; + red_prev->next_chunk = red; + } +-- +2.4.3 + diff --git a/SOURCES/0051-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch b/SOURCES/0051-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch new file mode 100644 index 0000000..331c3a2 --- /dev/null +++ b/SOURCES/0051-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch @@ -0,0 +1,75 @@ +From 173645bea63ad31ba0d6d4c58a06272aa488fb92 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 12:14:55 +0100 +Subject: [PATCH 51/57] Prevent memory leak if red_get_data_chunks_ptr fails + +Free linked list if client tries to do nasty things + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 31 ++++++++++++++++++++----------- + 1 file changed, 20 insertions(+), 11 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 7cc20e6..fe3ae78 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -107,34 +107,43 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id, + red->data_size = qxl->data_size; + data_size += red->data_size; + red->data = qxl->data; ++ red->prev_chunk = red->next_chunk = NULL; + if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) { + red->data = NULL; + return 0; + } +- red->prev_chunk = NULL; + + while ((next_chunk = qxl->next_chunk) != 0) { + red_prev = red; +- red = spice_new(RedDataChunk, 1); ++ red = spice_new0(RedDataChunk, 1); ++ red->prev_chunk = red_prev; ++ red_prev->next_chunk = red; ++ + memslot_id = get_memslot_id(slots, next_chunk); + qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id, + &error); +- if (error) { +- return 0; +- } ++ if (error) ++ goto error; + red->data_size = qxl->data_size; + data_size += red->data_size; + red->data = qxl->data; +- if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) { +- red->data = NULL; +- return 0; +- } +- red->prev_chunk = red_prev; +- red_prev->next_chunk = red; ++ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) ++ goto error; + } + + red->next_chunk = NULL; + return data_size; ++ ++error: ++ while (red->prev_chunk) { ++ red_prev = red->prev_chunk; ++ free(red); ++ red = red_prev; ++ } ++ red->data_size = 0; ++ red->next_chunk = NULL; ++ red->data = NULL; ++ return 0; + } + + static size_t red_get_data_chunks(RedMemSlotInfo *slots, int group_id, +-- +2.4.3 + diff --git a/SOURCES/0052-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch b/SOURCES/0052-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch new file mode 100644 index 0000000..eb26991 --- /dev/null +++ b/SOURCES/0052-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch @@ -0,0 +1,102 @@ +From ffbe1b2e3bff5fec022030836a9319d57c6f94c5 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 12:28:54 +0100 +Subject: [PATCH 52/57] Prevent DoS from guest trying to allocate too much data + on host for chunks + +Limit number of chunks to a given amount to avoid guest trying to +allocate too much memory. Using circular or nested chunks lists +guest could try to allocate huge amounts of memory. +Considering the list can be infinite and guest can change data this +also prevents strange security attacks from guest. + +Signed-off-by: Frediano Ziglio +--- + server/red_parse_qxl.c | 49 +++++++++++++++++++++++++++++++++++++++++-------- + 1 file changed, 41 insertions(+), 8 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index fe3ae78..f183248 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -37,6 +37,13 @@ + + G_STATIC_ASSERT(MAX_DATA_CHUNK <= G_MAXINT32); + ++/* Limit number of chunks. ++ * The guest can attempt to make host allocate too much memory ++ * just with a large number of small chunks. ++ * Prevent that the chunk list take more memory than the data itself. ++ */ ++#define MAX_CHUNKS (MAX_DATA_CHUNK/1024u) ++ + #if 0 + static void hexdump_qxl(RedMemSlotInfo *slots, int group_id, + QXLPHYSICAL addr, uint8_t bytes) +@@ -100,9 +107,11 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id, + RedDataChunk *red, QXLDataChunk *qxl) + { + RedDataChunk *red_prev; +- size_t data_size = 0; ++ uint64_t data_size = 0; ++ uint32_t chunk_data_size; + int error; + QXLPHYSICAL next_chunk; ++ unsigned num_chunks = 0; + + red->data_size = qxl->data_size; + data_size += red->data_size; +@@ -114,19 +123,43 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id, + } + + while ((next_chunk = qxl->next_chunk) != 0) { ++ /* somebody is trying to use too much memory using a lot of chunks. ++ * Or made a circular list of chunks ++ */ ++ if (++num_chunks >= MAX_CHUNKS) { ++ spice_warning("data split in too many chunks, avoiding DoS\n"); ++ goto error; ++ } ++ ++ memslot_id = get_memslot_id(slots, next_chunk); ++ qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), ++ group_id, &error); ++ if (error) ++ goto error; ++ ++ /* do not waste space for empty chunks. ++ * This could be just a driver issue or an attempt ++ * to allocate too much memory or a circular list. ++ * All above cases are handled by the check for number ++ * of chunks. ++ */ ++ chunk_data_size = qxl->data_size; ++ if (chunk_data_size == 0) ++ continue; ++ + red_prev = red; + red = spice_new0(RedDataChunk, 1); ++ red->data_size = chunk_data_size; + red->prev_chunk = red_prev; ++ red->data = qxl->data; + red_prev->next_chunk = red; + +- memslot_id = get_memslot_id(slots, next_chunk); +- qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id, +- &error); +- if (error) ++ data_size += chunk_data_size; ++ /* this can happen if client is sending nested chunks */ ++ if (data_size > MAX_DATA_CHUNK) { ++ spice_warning("too much data inside chunks, avoiding DoS\n"); + goto error; +- red->data_size = qxl->data_size; +- data_size += red->data_size; +- red->data = qxl->data; ++ } + if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) + goto error; + } +-- +2.4.3 + diff --git a/SOURCES/0053-Fix-some-possible-overflows-in-red_get_string-for-32.patch b/SOURCES/0053-Fix-some-possible-overflows-in-red_get_string-for-32.patch new file mode 100644 index 0000000..6875045 --- /dev/null +++ b/SOURCES/0053-Fix-some-possible-overflows-in-red_get_string-for-32.patch @@ -0,0 +1,41 @@ +From 8f60fc6ec611aa6ad6fa31f3dfc8027462dbb442 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 13:06:03 +0100 +Subject: [PATCH 53/57] Fix some possible overflows in red_get_string for 32 + bit + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 8 +++++++- + 1 file changed, 7 insertions(+), 1 deletion(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index f183248..668ce10 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -895,6 +895,11 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id, + glyphs++; + glyph_size = start->height * ((start->width * bpp + 7u) / 8u); + red_size += sizeof(SpiceRasterGlyph *) + SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4); ++ /* do the test correctly, we know end - start->data[0] cannot ++ * overflow, don't use start->data[glyph_size] to test for ++ * buffer overflow as this on 32 bit can cause overflow ++ * on the pointer arithmetic */ ++ spice_assert(glyph_size <= (char*) end - (char*) &start->data[0]); + start = (QXLRasterGlyph*)(&start->data[glyph_size]); + } + spice_assert(start <= end); +@@ -915,7 +920,8 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id, + red_get_point_ptr(&glyph->render_pos, &start->render_pos); + red_get_point_ptr(&glyph->glyph_origin, &start->glyph_origin); + glyph_size = glyph->height * ((glyph->width * bpp + 7u) / 8u); +- spice_assert((QXLRasterGlyph*)(&start->data[glyph_size]) <= end); ++ /* see above for similar test */ ++ spice_assert(glyph_size <= (char*) end - (char*) &start->data[0]); + memcpy(glyph->data, start->data, glyph_size); + start = (QXLRasterGlyph*)(&start->data[glyph_size]); + glyph = (SpiceRasterGlyph*) +-- +2.4.3 + diff --git a/SOURCES/0054-Make-sure-we-can-read-QXLPathSeg-structures.patch b/SOURCES/0054-Make-sure-we-can-read-QXLPathSeg-structures.patch new file mode 100644 index 0000000..f363968 --- /dev/null +++ b/SOURCES/0054-Make-sure-we-can-read-QXLPathSeg-structures.patch @@ -0,0 +1,40 @@ +From 289301f33c7da81fcb034448d96e8c276b4fc06a Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 15 Sep 2015 16:25:17 +0100 +Subject: [PATCH 54/57] Make sure we can read QXLPathSeg structures + +start pointer points to a QXLPathSeg structure. +Before reading from the structure, make sure the structure is contained +in the memory range checked. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 668ce10..4663bfd 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -256,7 +256,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id, + + start = (QXLPathSeg*)data; + end = (QXLPathSeg*)(data + size); +- while (start < end) { ++ while (start+1 < end) { + n_segments++; + count = start->count; + segment_size = sizeof(SpicePathSeg) + count * sizeof(SpicePointFix); +@@ -272,7 +272,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id, + seg = (SpicePathSeg*)&red->segments[n_segments]; + n_segments = 0; + mem_size2 = sizeof(*red); +- while (start < end) { ++ while (start+1 < end) { + red->segments[n_segments++] = seg; + count = start->count; + +-- +2.4.3 + diff --git a/SOURCES/0055-Avoid-race-condition-copying-segments-in-red_get_pat.patch b/SOURCES/0055-Avoid-race-condition-copying-segments-in-red_get_pat.patch new file mode 100644 index 0000000..51c989c --- /dev/null +++ b/SOURCES/0055-Avoid-race-condition-copying-segments-in-red_get_pat.patch @@ -0,0 +1,31 @@ +From 14f53eef04c38a3c537a1a1012c2f7101a298194 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 15 Sep 2015 16:38:23 +0100 +Subject: [PATCH 55/57] Avoid race condition copying segments in red_get_path + +The guest can attempt to increase the number of segments while +spice-server is reading them. +Make sure we don't copy more then the allocated segments. + +Signed-off-by: Frediano Ziglio +Acked-by: Christophe Fergeau +--- + server/red_parse_qxl.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 4663bfd..c1df8e8 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -272,7 +272,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id, + seg = (SpicePathSeg*)&red->segments[n_segments]; + n_segments = 0; + mem_size2 = sizeof(*red); +- while (start+1 < end) { ++ while (start+1 < end && n_segments < red->num_segments) { + red->segments[n_segments++] = seg; + count = start->count; + +-- +2.4.3 + diff --git a/SOURCES/0056-Prevent-data_size-to-be-set-independently-from-data.patch b/SOURCES/0056-Prevent-data_size-to-be-set-independently-from-data.patch new file mode 100644 index 0000000..a78ea9a --- /dev/null +++ b/SOURCES/0056-Prevent-data_size-to-be-set-independently-from-data.patch @@ -0,0 +1,29 @@ +From c2cdd1daf8edceec8adbb456dca656efe3648eec Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Thu, 17 Sep 2015 14:28:36 +0100 +Subject: [PATCH 56/57] Prevent data_size to be set independently from data + +There was not check for data_size field so one could set data to +a small set of data and data_size much bigger than size of data +leading to buffer overflow. + +Signed-off-by: Frediano Ziglio +--- + server/red_parse_qxl.c | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index c1df8e8..8e3dd55 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -1391,6 +1391,7 @@ static int red_get_cursor(RedMemSlotInfo *slots, int group_id, + size = red_get_data_chunks_ptr(slots, group_id, + get_memslot_id(slots, addr), + &chunks, &qxl->chunk); ++ red->data_size = MIN(red->data_size, size); + data = red_linearize_chunk(&chunks, size, &free_data); + red_put_data_chunks(&chunks); + if (free_data) { +-- +2.4.3 + diff --git a/SOURCES/0057-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch b/SOURCES/0057-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch new file mode 100644 index 0000000..b7bf58f --- /dev/null +++ b/SOURCES/0057-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch @@ -0,0 +1,34 @@ +From 5580cac5502cd518adad0a3682fd53aeeaf86a86 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Thu, 17 Sep 2015 15:01:05 +0100 +Subject: [PATCH 57/57] Prevent leak if size from red_get_data_chunks don't + match in red_get_image + +Signed-off-by: Frediano Ziglio +--- + server/red_parse_qxl.c | 2 ++ + 1 file changed, 2 insertions(+) + +diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c +index 8e3dd55..bd0c408 100644 +--- a/server/red_parse_qxl.c ++++ b/server/red_parse_qxl.c +@@ -530,6 +530,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id, + &chunks, qxl->bitmap.data); + spice_assert(size == bitmap_size); + if (size != bitmap_size) { ++ red_put_data_chunks(&chunks); + goto error; + } + red->u.bitmap.data = red_get_image_data_chunked(slots, group_id, +@@ -550,6 +551,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id, + &chunks, (QXLDataChunk *)qxl->quic.data); + spice_assert(size == red->u.quic.data_size); + if (size != red->u.quic.data_size) { ++ red_put_data_chunks(&chunks); + goto error; + } + red->u.quic.data = red_get_image_data_chunked(slots, group_id, +-- +2.4.3 + diff --git a/SPECS/spice.spec b/SPECS/spice.spec index 1fa2e80..f24555c 100644 --- a/SPECS/spice.spec +++ b/SPECS/spice.spec @@ -1,6 +1,6 @@ Name: spice Version: 0.12.4 -Release: 9%{?dist}.1 +Release: 9%{?dist}.3 Summary: Implements the SPICE protocol Group: User Interface/Desktops License: LGPLv2+ @@ -44,6 +44,25 @@ Patch35: 0035-migration-Don-t-assert-if-MIGRATE_DATA-comes-before-.patch Patch36: 0036-server-fix-crash-when-restarting-VM-with-old-client.patch Patch37: 0037-Use-TLS-version-1.0-or-better.patch Patch38: 0038-Avoid-race-conditions-reading-monitor-configs-from-g.patch +Patch39: 0039-worker-validate-correctly-surfaces.patch +Patch40: 0040-worker-avoid-double-free-or-double-create-of-surface.patch +Patch41: 0041-Define-a-constant-to-limit-data-from-guest.patch +Patch42: 0042-Fix-some-integer-overflow-causing-large-memory-alloc.patch +Patch43: 0043-Check-properly-surface-to-be-created.patch +Patch44: 0044-Fix-buffer-reading-overflow.patch +Patch45: 0045-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch +Patch46: 0046-Fix-race-condition-on-red_get_clip_rects.patch +Patch47: 0047-Fix-race-in-red_get_image.patch +Patch48: 0048-Fix-race-condition-in-red_get_string.patch +Patch49: 0049-Fix-integer-overflow-computing-glyph_size-in-red_get.patch +Patch50: 0050-Fix-race-condition-in-red_get_data_chunks_ptr.patch +Patch51: 0051-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch +Patch52: 0052-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch +Patch53: 0053-Fix-some-possible-overflows-in-red_get_string-for-32.patch +Patch54: 0054-Make-sure-we-can-read-QXLPathSeg-structures.patch +Patch55: 0055-Avoid-race-condition-copying-segments-in-red_get_pat.patch +Patch56: 0056-Prevent-data_size-to-be-set-independently-from-data.patch +Patch57: 0057-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch # https://bugzilla.redhat.com/show_bug.cgi?id=613529 %if 0%{?rhel} @@ -137,6 +156,25 @@ using spice-server, you will need to install spice-server-devel. %patch36 -p1 %patch37 -p1 %patch38 -p1 +%patch39 -p1 +%patch40 -p1 +%patch41 -p1 +%patch42 -p1 +%patch43 -p1 +%patch44 -p1 +%patch45 -p1 +%patch46 -p1 +%patch47 -p1 +%patch48 -p1 +%patch49 -p1 +%patch50 -p1 +%patch51 -p1 +%patch52 -p1 +%patch53 -p1 +%patch54 -p1 +%patch55 -p1 +%patch56 -p1 +%patch57 -p1 %build @@ -167,6 +205,14 @@ mkdir -p %{buildroot}%{_libexecdir} %changelog +* Wed Sep 23 2015 Frediano Ziglio 0.12.4-9.3 +- CVE-2015-5260 CVE-2015-5261 fixed various security flaws + Resolves: rhbz#1262771 + +* Tue Sep 15 2015 Frediano Ziglio 0.12.4-9.2 +- Validate surface_id + Resolves: rhbz#1262771 + * Tue Jul 21 2015 Christophe Fergeau 0.12.4-9.1 - Avoid race conditions reading monitor configs from guest. This race could trigger memory corruption host-side