diff --git a/SOURCES/0038-Add-const-to-test_capability-first-argument.patch b/SOURCES/0038-Add-const-to-test_capability-first-argument.patch new file mode 100644 index 0000000..7fd9526 --- /dev/null +++ b/SOURCES/0038-Add-const-to-test_capability-first-argument.patch @@ -0,0 +1,40 @@ +From 17fd07ff2fd287fff6cc866a308097e75cb968f5 Mon Sep 17 00:00:00 2001 +From: Christophe Fergeau +Date: Wed, 19 Mar 2014 18:17:32 +0100 +Subject: [PATCH 1/3] Add const to test_capability first argument + +We don't modify the capabilities content, so it can be marked as const. +--- + server/red_channel.c | 2 +- + server/red_channel.h | 2 +- + 2 files changed, 2 insertions(+), 2 deletions(-) + +diff --git a/server/red_channel.c b/server/red_channel.c +index 5df8f14..449e628 100644 +--- a/server/red_channel.c ++++ b/server/red_channel.c +@@ -1159,7 +1159,7 @@ void red_channel_register_client_cbs(RedChannel *channel, ClientCbs *client_cbs) + } + } + +-int test_capabilty(uint32_t *caps, int num_caps, uint32_t cap) ++int test_capabilty(const uint32_t *caps, int num_caps, uint32_t cap) + { + uint32_t index = cap / 32; + if (num_caps < index + 1) { +diff --git a/server/red_channel.h b/server/red_channel.h +index 9e54dce..58109d5 100644 +--- a/server/red_channel.h ++++ b/server/red_channel.h +@@ -223,7 +223,7 @@ typedef struct RedChannelCapabilities { + uint32_t *caps; + } RedChannelCapabilities; + +-int test_capabilty(uint32_t *caps, int num_caps, uint32_t cap); ++int test_capabilty(const uint32_t *caps, int num_caps, uint32_t cap); + + typedef struct RedChannelClientLatencyMonitor { + int state; +-- +2.4.4 + diff --git a/SOURCES/0038-Avoid-race-conditions-reading-monitor-configs-from-g.patch b/SOURCES/0038-Avoid-race-conditions-reading-monitor-configs-from-g.patch deleted file mode 100644 index c909575..0000000 --- a/SOURCES/0038-Avoid-race-conditions-reading-monitor-configs-from-g.patch +++ /dev/null @@ -1,117 +0,0 @@ -From 524eef10c6c6c2f3f30be28c56b8f96adc7901f0 Mon Sep 17 00:00:00 2001 -From: Frediano Ziglio -Date: Tue, 9 Jun 2015 08:50:46 +0100 -Subject: [PATCH] Avoid race conditions reading monitor configs from guest - -For security reasons do not assume guest do not change structures it -pass to Qemu. -Guest could change count field while Qemu is copying QXLMonitorsConfig -structure leading to heap corruption. -This patch avoid it reading count only once. - -Signed-off-by: Frediano Ziglio ---- - server/red_worker.c | 46 ++++++++++++++++++++++++++++++++-------------- - 1 file changed, 32 insertions(+), 14 deletions(-) - -diff --git a/server/red_worker.c b/server/red_worker.c -index 9e6a6ad..955cac2 100644 ---- a/server/red_worker.c -+++ b/server/red_worker.c -@@ -11270,7 +11270,8 @@ static inline void red_monitors_config_item_add(DisplayChannelClient *dcc) - } - - static void worker_update_monitors_config(RedWorker *worker, -- QXLMonitorsConfig *dev_monitors_config) -+ QXLMonitorsConfig *dev_monitors_config, -+ uint16_t count, uint16_t max_allowed) - { - int heads_size; - MonitorsConfig *monitors_config; -@@ -11279,22 +11280,22 @@ static void worker_update_monitors_config(RedWorker *worker, - monitors_config_decref(worker->monitors_config); - - spice_debug("monitors config %d(%d)", -- dev_monitors_config->count, -- dev_monitors_config->max_allowed); -- for (i = 0; i < dev_monitors_config->count; i++) { -+ count, -+ max_allowed); -+ for (i = 0; i < count; i++) { - spice_debug("+%d+%d %dx%d", - dev_monitors_config->heads[i].x, - dev_monitors_config->heads[i].y, - dev_monitors_config->heads[i].width, - dev_monitors_config->heads[i].height); - } -- heads_size = dev_monitors_config->count * sizeof(QXLHead); -+ heads_size = count * sizeof(QXLHead); - worker->monitors_config = monitors_config = - spice_malloc(sizeof(*monitors_config) + heads_size); - monitors_config->refs = 1; - monitors_config->worker = worker; -- monitors_config->count = dev_monitors_config->count; -- monitors_config->max_allowed = dev_monitors_config->max_allowed; -+ monitors_config->count = count; -+ monitors_config->max_allowed = max_allowed; - memcpy(monitors_config->heads, dev_monitors_config->heads, heads_size); - } - -@@ -11678,33 +11679,50 @@ void handle_dev_display_migrate(void *opaque, void *payload) - red_migrate_display(worker, rcc); - } - -+static inline uint32_t qxl_monitors_config_size(uint32_t heads) -+{ -+ return sizeof(QXLMonitorsConfig) + sizeof(QXLHead) * heads; -+} -+ - static void handle_dev_monitors_config_async(void *opaque, void *payload) - { - RedWorkerMessageMonitorsConfigAsync *msg = payload; - RedWorker *worker = opaque; -- int min_size = sizeof(QXLMonitorsConfig) + sizeof(QXLHead); - int error; -+ uint16_t count, max_allowed; - QXLMonitorsConfig *dev_monitors_config = - (QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config, -- min_size, msg->group_id, &error); -+ qxl_monitors_config_size(1), -+ msg->group_id, &error); - - if (error) { - /* TODO: raise guest bug (requires added QXL interface) */ - return; - } - worker->driver_cap_monitors_config = 1; -- if (dev_monitors_config->count == 0) { -+ count = dev_monitors_config->count; -+ max_allowed = dev_monitors_config->max_allowed; -+ if (count == 0) { - spice_warning("ignoring an empty monitors config message from driver"); - return; - } -- if (dev_monitors_config->count > dev_monitors_config->max_allowed) { -+ if (count > max_allowed) { - spice_warning("ignoring malformed monitors_config from driver, " - "count > max_allowed %d > %d", -- dev_monitors_config->count, -- dev_monitors_config->max_allowed); -+ count, -+ max_allowed); - return; - } -- worker_update_monitors_config(worker, dev_monitors_config); -+ /* get pointer again to check virtual size */ -+ dev_monitors_config = -+ (QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config, -+ qxl_monitors_config_size(count), -+ msg->group_id, &error); -+ if (error) { -+ /* TODO: raise guest bug (requires added QXL interface) */ -+ return; -+ } -+ worker_update_monitors_config(worker, dev_monitors_config, count, max_allowed); - red_worker_push_monitors_config(worker); - } - diff --git a/SOURCES/0039-Introduce-red_link_info_test_capability.patch b/SOURCES/0039-Introduce-red_link_info_test_capability.patch new file mode 100644 index 0000000..8b17861 --- /dev/null +++ b/SOURCES/0039-Introduce-red_link_info_test_capability.patch @@ -0,0 +1,60 @@ +From 0b0ca2c972041b785145c558dfd0e8372869f696 Mon Sep 17 00:00:00 2001 +From: Christophe Fergeau +Date: Wed, 5 Mar 2014 11:59:37 +0100 +Subject: [PATCH 2/3] Introduce red_link_info_test_capability() + +This just hides a bit of pointer arithmetic away from reds_send_link_ack. +This helper will be used in the next commits. +--- + server/reds.c | 21 ++++++++++++++++++--- + 1 file changed, 18 insertions(+), 3 deletions(-) + +diff --git a/server/reds.c b/server/reds.c +index ccba67c..c1edf67 100644 +--- a/server/reds.c ++++ b/server/reds.c +@@ -1446,6 +1446,22 @@ static void reds_channel_init_auth_caps(RedLinkInfo *link, RedChannel *channel) + red_channel_set_common_cap(channel, SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION); + } + ++ ++static const uint32_t *red_link_info_get_caps(const RedLinkInfo *link) ++{ ++ const uint8_t *caps_start = (const uint8_t *)link->link_mess; ++ ++ return (const uint32_t *)(caps_start + link->link_mess->caps_offset); ++} ++ ++static bool red_link_info_test_capability(const RedLinkInfo *link, uint32_t cap) ++{ ++ const uint32_t *caps = red_link_info_get_caps(link); ++ ++ return test_capabilty(caps, link->link_mess->num_common_caps, cap); ++} ++ ++ + static int reds_send_link_ack(RedLinkInfo *link) + { + SpiceLinkHeader header; +@@ -2701,7 +2717,6 @@ static void reds_handle_read_link_done(void *opaque) + SpiceLinkMess *link_mess = link->link_mess; + AsyncRead *obj = &link->asyc_read; + uint32_t num_caps = link_mess->num_common_caps + link_mess->num_channel_caps; +- uint32_t *caps = (uint32_t *)((uint8_t *)link_mess + link_mess->caps_offset); + int auth_selection; + + if (num_caps && (num_caps * sizeof(uint32_t) + link_mess->caps_offset > +@@ -2712,8 +2727,8 @@ static void reds_handle_read_link_done(void *opaque) + return; + } + +- auth_selection = test_capabilty(caps, link_mess->num_common_caps, +- SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION); ++ auth_selection = red_link_info_test_capability(link, ++ SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION); + + if (!reds_security_check(link)) { + if (link->stream->ssl) { +-- +2.4.4 + diff --git a/SOURCES/0039-worker-validate-correctly-surfaces.patch b/SOURCES/0039-worker-validate-correctly-surfaces.patch deleted file mode 100644 index f7a89d8..0000000 --- a/SOURCES/0039-worker-validate-correctly-surfaces.patch +++ /dev/null @@ -1,136 +0,0 @@ -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-Don-t-set-SpiceLinkReply-pub_key-if-client-advertise.patch b/SOURCES/0040-Don-t-set-SpiceLinkReply-pub_key-if-client-advertise.patch new file mode 100644 index 0000000..47993b3 --- /dev/null +++ b/SOURCES/0040-Don-t-set-SpiceLinkReply-pub_key-if-client-advertise.patch @@ -0,0 +1,111 @@ +From 3a3ec08f25e2b53e9de256fcb3a4f951c4b1e871 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= +Date: Thu, 9 Jul 2015 01:04:31 +0200 +Subject: [PATCH 3/3] Don't set SpiceLinkReply::pub_key if client advertises + SASL cap + +If the client advertises the SASL cap, it means it guarantees it will be +able to use SASL if the server supports, and that it does not need a valid +SpiceLinkReply::pub_key field when using SASL. + +When the client cap is set, we thus don't need to create a RSA public key +if SASL is enabled server side. + +The reason for needing client guarantees about not looking at the pub_key +field is that its presence and size is hardcoded in the protocol, but in +some hardened setups (using fips mode), generating a RSA 1024 bit key as +expected is forbidden and fails. With this new capability, the server +knows the client will be able to handle SASL if needed, and can skip +the generation of the key altogether. This means that on the setups +described above, SASL authentication has to be used. +--- + server/reds.c | 56 +++++++++++++++++++++++++++++++++++++++----------------- + 1 file changed, 39 insertions(+), 17 deletions(-) + +diff --git a/server/reds.c b/server/reds.c +index c1edf67..9521416 100644 +--- a/server/reds.c ++++ b/server/reds.c +@@ -1469,7 +1469,7 @@ static int reds_send_link_ack(RedLinkInfo *link) + RedChannel *channel; + RedChannelCapabilities *channel_caps; + BUF_MEM *bmBuf; +- BIO *bio; ++ BIO *bio = NULL; + int ret = FALSE; + + header.magic = SPICE_MAGIC; +@@ -1494,24 +1494,45 @@ static int reds_send_link_ack(RedLinkInfo *link) + ack.num_channel_caps = channel_caps->num_caps; + header.size += (ack.num_common_caps + ack.num_channel_caps) * sizeof(uint32_t); + ack.caps_offset = sizeof(SpiceLinkReply); ++ if (!sasl_enabled ++ || !red_link_info_test_capability(link, SPICE_COMMON_CAP_AUTH_SASL)) { ++ if (!(link->tiTicketing.rsa = RSA_new())) { ++ spice_warning("RSA new failed"); ++ return FALSE; ++ } + +- if (!(link->tiTicketing.rsa = RSA_new())) { +- spice_warning("RSA nes failed"); +- return FALSE; +- } +- +- if (!(bio = BIO_new(BIO_s_mem()))) { +- spice_warning("BIO new failed"); +- return FALSE; +- } ++ if (!(bio = BIO_new(BIO_s_mem()))) { ++ spice_warning("BIO new failed"); ++ return FALSE; ++ } + +- RSA_generate_key_ex(link->tiTicketing.rsa, SPICE_TICKET_KEY_PAIR_LENGTH, link->tiTicketing.bn, +- NULL); +- link->tiTicketing.rsa_size = RSA_size(link->tiTicketing.rsa); ++ if (RSA_generate_key_ex(link->tiTicketing.rsa, ++ SPICE_TICKET_KEY_PAIR_LENGTH, ++ link->tiTicketing.bn, ++ NULL) != 1) { ++ spice_warning("Failed to generate %d bits RSA key: %s", ++ SPICE_TICKET_KEY_PAIR_LENGTH, ++ ERR_error_string(ERR_get_error(), NULL)); ++ goto end; ++ } ++ link->tiTicketing.rsa_size = RSA_size(link->tiTicketing.rsa); + +- i2d_RSA_PUBKEY_bio(bio, link->tiTicketing.rsa); +- BIO_get_mem_ptr(bio, &bmBuf); +- memcpy(ack.pub_key, bmBuf->data, sizeof(ack.pub_key)); ++ i2d_RSA_PUBKEY_bio(bio, link->tiTicketing.rsa); ++ BIO_get_mem_ptr(bio, &bmBuf); ++ memcpy(ack.pub_key, bmBuf->data, sizeof(ack.pub_key)); ++ } else { ++ /* if the client sets the AUTH_SASL cap, it indicates that it ++ * supports SASL, and will use it if the server supports SASL as ++ * well. Moreover, a client setting the AUTH_SASL cap also ++ * indicates that it will not try using the RSA-related content ++ * in the SpiceLinkReply message, so we don't need to initialize ++ * it. Reason to avoid this is to fix auth in fips mode where ++ * the generation of a 1024 bit RSA key as we are trying to do ++ * will fail. ++ */ ++ spice_warning("not initialising RSA key"); ++ memset(ack.pub_key, '\0', sizeof(ack.pub_key)); ++ } + + if (!sync_write(link->stream, &header, sizeof(header))) + goto end; +@@ -1525,7 +1546,8 @@ static int reds_send_link_ack(RedLinkInfo *link) + ret = TRUE; + + end: +- BIO_free(bio); ++ if (bio != NULL) ++ BIO_free(bio); + return ret; + } + +-- +2.4.4 + 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 deleted file mode 100644 index 2ef2398..0000000 --- a/SOURCES/0040-worker-avoid-double-free-or-double-create-of-surface.patch +++ /dev/null @@ -1,46 +0,0 @@ -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 deleted file mode 100644 index d569778..0000000 --- a/SOURCES/0041-Define-a-constant-to-limit-data-from-guest.patch +++ /dev/null @@ -1,42 +0,0 @@ -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/0041-server-don-t-assert-on-invalid-client-message.patch b/SOURCES/0041-server-don-t-assert-on-invalid-client-message.patch new file mode 100644 index 0000000..a3750ca --- /dev/null +++ b/SOURCES/0041-server-don-t-assert-on-invalid-client-message.patch @@ -0,0 +1,33 @@ +From cf19a886b8ce41ce7c5159a2898f9587318a32ad Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= +Date: Wed, 25 Jun 2014 14:36:03 +0200 +Subject: [PATCH] server: don't assert on invalid client message + +Some users have been reaching this error: +snd_receive: ASSERT n failed + +A misbehaving client could easily hit that condition by sending too big +messages. Instead of assert(), replace with a warning. When a message +too big to fit is received, it will simply disconnect the channel. + +https://bugzilla.redhat.com/show_bug.cgi?id=962187 +--- + server/snd_worker.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/server/snd_worker.c b/server/snd_worker.c +index ebddfcd..c451031 100644 +--- a/server/snd_worker.c ++++ b/server/snd_worker.c +@@ -421,7 +421,7 @@ static void snd_receive(void* data) + for (;;) { + ssize_t n; + n = channel->recive_data.end - channel->recive_data.now; +- spice_assert(n); ++ spice_warn_if(n <= 0); + n = reds_stream_read(channel->stream, channel->recive_data.now, n); + if (n <= 0) { + if (n == 0) { +-- +2.4.4 + diff --git a/SOURCES/0042-Don-t-truncate-large-now-values-in-_spice_timer_set.patch b/SOURCES/0042-Don-t-truncate-large-now-values-in-_spice_timer_set.patch new file mode 100644 index 0000000..792da4f --- /dev/null +++ b/SOURCES/0042-Don-t-truncate-large-now-values-in-_spice_timer_set.patch @@ -0,0 +1,80 @@ +From d6f73e30020e0e2cbe6ee48d5f1bf38e0587ba83 Mon Sep 17 00:00:00 2001 +From: David Gibson +Date: Mon, 10 Mar 2014 11:55:47 +0100 +Subject: [PATCH] Don't truncate large 'now' values in _spice_timer_set + +static void _spice_timer_set(SpiceTimer *timer, uint32_t ms, uint32_t now) + +The _spice_timer_set() function takes a 32-bit integer for the "now" value. +The now value passed in however, can exceed 2^32 (it's in ms and derived +from CLOCK_MONOTONIC, which will wrap around a 32-bit integer in around 46 +days). + +If the now value passed in exceeds 2^32, this will mean timers are inserted +into the active list with expiry values before the current time, they will +immediately trigger, and (if they don't make themselves inactive) be +reinserted still before the current time. + +This leads to an infinite loop in spice_timer_queue_cb(). + +https://bugzilla.redhat.com/show_bug.cgi?id=1072700 +--- + server/spice_timer_queue.c | 13 +++++++------ + 1 file changed, 7 insertions(+), 6 deletions(-) + +diff --git a/server/spice_timer_queue.c b/server/spice_timer_queue.c +index 8f6e9c8..71de84a 100644 +--- a/server/spice_timer_queue.c ++++ b/server/spice_timer_queue.c +@@ -147,7 +147,7 @@ SpiceTimer *spice_timer_queue_add(SpiceTimerFunc func, void *opaque) + return timer; + } + +-static void _spice_timer_set(SpiceTimer *timer, uint32_t ms, uint32_t now) ++static void _spice_timer_set(SpiceTimer *timer, uint32_t ms, uint64_t now) + { + RingItem *next_item; + SpiceTimerQueue *queue; +@@ -183,7 +183,8 @@ void spice_timer_set(SpiceTimer *timer, uint32_t ms) + spice_assert(pthread_equal(timer->queue->thread, pthread_self()) != 0); + + clock_gettime(CLOCK_MONOTONIC, &now); +- _spice_timer_set(timer, ms, now.tv_sec * 1000 + (now.tv_nsec / 1000 / 1000)); ++ _spice_timer_set(timer, ms, ++ (uint64_t)now.tv_sec * 1000 + (now.tv_nsec / 1000 / 1000)); + } + + void spice_timer_cancel(SpiceTimer *timer) +@@ -217,7 +218,7 @@ void spice_timer_remove(SpiceTimer *timer) + unsigned int spice_timer_queue_get_timeout_ms(void) + { + struct timespec now; +- int now_ms; ++ int64_t now_ms; + RingItem *head; + SpiceTimer *head_timer; + SpiceTimerQueue *queue = spice_timer_queue_find_with_lock(); +@@ -232,9 +233,9 @@ unsigned int spice_timer_queue_get_timeout_ms(void) + head_timer = SPICE_CONTAINEROF(head, SpiceTimer, active_link); + + clock_gettime(CLOCK_MONOTONIC, &now); +- now_ms = (now.tv_sec * 1000) - (now.tv_nsec / 1000 / 1000); ++ now_ms = ((int64_t)now.tv_sec * 1000) - (now.tv_nsec / 1000 / 1000); + +- return MAX(0, ((int)head_timer->expiry_time - now_ms)); ++ return MAX(0, ((int64_t)head_timer->expiry_time - now_ms)); + } + + +@@ -252,7 +253,7 @@ void spice_timer_queue_cb(void) + } + + clock_gettime(CLOCK_MONOTONIC, &now); +- now_ms = (now.tv_sec * 1000) + (now.tv_nsec / 1000 / 1000); ++ now_ms = ((uint64_t)now.tv_sec * 1000) + (now.tv_nsec / 1000 / 1000); + + while ((head = ring_get_head(&queue->active_timers))) { + SpiceTimer *timer = SPICE_CONTAINEROF(head, SpiceTimer, active_link); +-- +2.4.4 + 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 deleted file mode 100644 index ff5e6af..0000000 --- a/SOURCES/0042-Fix-some-integer-overflow-causing-large-memory-alloc.patch +++ /dev/null @@ -1,69 +0,0 @@ -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-Avoid-race-conditions-reading-monitor-configs-from-g.patch b/SOURCES/0043-Avoid-race-conditions-reading-monitor-configs-from-g.patch new file mode 100644 index 0000000..c26de66 --- /dev/null +++ b/SOURCES/0043-Avoid-race-conditions-reading-monitor-configs-from-g.patch @@ -0,0 +1,120 @@ +From 4249d114abe6ace0553b6cfd1464e220d1e5acb1 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 9 Jun 2015 08:50:46 +0100 +Subject: [PATCH] Avoid race conditions reading monitor configs from guest + +For security reasons do not assume guest do not change structures it +pass to Qemu. +Guest could change count field while Qemu is copying QXLMonitorsConfig +structure leading to heap corruption. +This patch avoid it reading count only once. + +Signed-off-by: Frediano Ziglio +--- + server/red_worker.c | 46 ++++++++++++++++++++++++++++++++-------------- + 1 file changed, 32 insertions(+), 14 deletions(-) + +diff --git a/server/red_worker.c b/server/red_worker.c +index 9e6a6ad..955cac2 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -11270,7 +11270,8 @@ static inline void red_monitors_config_item_add(DisplayChannelClient *dcc) + } + + static void worker_update_monitors_config(RedWorker *worker, +- QXLMonitorsConfig *dev_monitors_config) ++ QXLMonitorsConfig *dev_monitors_config, ++ uint16_t count, uint16_t max_allowed) + { + int heads_size; + MonitorsConfig *monitors_config; +@@ -11279,22 +11280,22 @@ static void worker_update_monitors_config(RedWorker *worker, + monitors_config_decref(worker->monitors_config); + + spice_debug("monitors config %d(%d)", +- dev_monitors_config->count, +- dev_monitors_config->max_allowed); +- for (i = 0; i < dev_monitors_config->count; i++) { ++ count, ++ max_allowed); ++ for (i = 0; i < count; i++) { + spice_debug("+%d+%d %dx%d", + dev_monitors_config->heads[i].x, + dev_monitors_config->heads[i].y, + dev_monitors_config->heads[i].width, + dev_monitors_config->heads[i].height); + } +- heads_size = dev_monitors_config->count * sizeof(QXLHead); ++ heads_size = count * sizeof(QXLHead); + worker->monitors_config = monitors_config = + spice_malloc(sizeof(*monitors_config) + heads_size); + monitors_config->refs = 1; + monitors_config->worker = worker; +- monitors_config->count = dev_monitors_config->count; +- monitors_config->max_allowed = dev_monitors_config->max_allowed; ++ monitors_config->count = count; ++ monitors_config->max_allowed = max_allowed; + memcpy(monitors_config->heads, dev_monitors_config->heads, heads_size); + } + +@@ -11678,33 +11679,50 @@ void handle_dev_display_migrate(void *opaque, void *payload) + red_migrate_display(worker, rcc); + } + ++static inline uint32_t qxl_monitors_config_size(uint32_t heads) ++{ ++ return sizeof(QXLMonitorsConfig) + sizeof(QXLHead) * heads; ++} ++ + static void handle_dev_monitors_config_async(void *opaque, void *payload) + { + RedWorkerMessageMonitorsConfigAsync *msg = payload; + RedWorker *worker = opaque; +- int min_size = sizeof(QXLMonitorsConfig) + sizeof(QXLHead); + int error; ++ uint16_t count, max_allowed; + QXLMonitorsConfig *dev_monitors_config = + (QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config, +- min_size, msg->group_id, &error); ++ qxl_monitors_config_size(1), ++ msg->group_id, &error); + + if (error) { + /* TODO: raise guest bug (requires added QXL interface) */ + return; + } + worker->driver_cap_monitors_config = 1; +- if (dev_monitors_config->count == 0) { ++ count = dev_monitors_config->count; ++ max_allowed = dev_monitors_config->max_allowed; ++ if (count == 0) { + spice_warning("ignoring an empty monitors config message from driver"); + return; + } +- if (dev_monitors_config->count > dev_monitors_config->max_allowed) { ++ if (count > max_allowed) { + spice_warning("ignoring malformed monitors_config from driver, " + "count > max_allowed %d > %d", +- dev_monitors_config->count, +- dev_monitors_config->max_allowed); ++ count, ++ max_allowed); ++ return; ++ } ++ /* get pointer again to check virtual size */ ++ dev_monitors_config = ++ (QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config, ++ qxl_monitors_config_size(count), ++ msg->group_id, &error); ++ if (error) { ++ /* TODO: raise guest bug (requires added QXL interface) */ + return; + } +- worker_update_monitors_config(worker, dev_monitors_config); ++ worker_update_monitors_config(worker, dev_monitors_config, count, max_allowed); + red_worker_push_monitors_config(worker); + } + +-- +2.4.4 + diff --git a/SOURCES/0043-Check-properly-surface-to-be-created.patch b/SOURCES/0043-Check-properly-surface-to-be-created.patch deleted file mode 100644 index 05c24d9..0000000 --- a/SOURCES/0043-Check-properly-surface-to-be-created.patch +++ /dev/null @@ -1,78 +0,0 @@ -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 deleted file mode 100644 index bc0240d..0000000 --- a/SOURCES/0044-Fix-buffer-reading-overflow.patch +++ /dev/null @@ -1,38 +0,0 @@ -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/0044-Lock-the-pixmap-image-cache-for-the-entire-fill_bits.patch b/SOURCES/0044-Lock-the-pixmap-image-cache-for-the-entire-fill_bits.patch new file mode 100644 index 0000000..bcc9e80 --- /dev/null +++ b/SOURCES/0044-Lock-the-pixmap-image-cache-for-the-entire-fill_bits.patch @@ -0,0 +1,223 @@ +From 6f7e2fca1030e38f3a82f09b30a4839923f17c66 Mon Sep 17 00:00:00 2001 +From: Sandy Stutsman +Date: Fri, 26 Jun 2015 11:59:13 -0400 +Subject: [PATCH] Lock the pixmap image cache for the entire fill_bits call + +Locking the individual calls that access the pixmap cache in fill_bits is +not adequately thread safe. Often a windows guest with multiple monitors +will be sending the same image via different threads. Both threads can +be in fill_bits at the same time making changes to the cache for the same +image. This can result in images being deleted before all the client +channels are finished with them or with the same image being send multiple +times. Here's what can happen with out the lock in fill_bits + +On the server in red_worker.c:fill_bits + Thread 1 calls pixmap_cache_hit for Image A and finds it isn't in cache + Thread 2 calls pixmap_cache_hit for Image A and finds it isn't in cache + + Thread 1 adds Image 1 to pixmap_cache (1x) + Thread 2 adds Image 1 to pixmap_cache (2x) + +On the client + Channel 1 adds Image A to image_cache (1x) + Channel 2 replaces Image A in image_cache (1x) + +On server + Thread 1 sends Image A rendering commands + Thread N removes Image A from pixmap_cache (image remains - 1x) + Thread 2 sends Image A rendering commands + +On client + Channe1 renders from Image A + Channel N removes Image a from image_cache (image is completely removed) + Channel2 render command hangs waiting for Image A +--- + server/red_client_shared_cache.h | 24 ++++++++++++------------ + server/red_worker.c | 23 +++++++++++++++-------- + 2 files changed, 27 insertions(+), 20 deletions(-) + +diff --git a/server/red_client_shared_cache.h b/server/red_client_shared_cache.h +index 821ee18..7feb28e 100644 +--- a/server/red_client_shared_cache.h ++++ b/server/red_client_shared_cache.h +@@ -36,13 +36,12 @@ + + #define CHANNEL_FROM_RCC(rcc) SPICE_CONTAINEROF((rcc)->channel, CHANNEL, common.base); + +-static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc) ++static int FUNC_NAME(unlocked_hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc) + { + NewCacheItem *item; + uint64_t serial; + + serial = red_channel_client_get_message_serial(&dcc->common.base); +- pthread_mutex_lock(&cache->lock); + item = cache->hash_table[CACHE_HASH_KEY(id)]; + + while (item) { +@@ -57,15 +56,22 @@ static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelC + } + item = item->next; + } +- pthread_mutex_unlock(&cache->lock); + + return !!item; + } + +-static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy) ++static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc) + { +- NewCacheItem *item; ++ int hit; + pthread_mutex_lock(&cache->lock); ++ hit = FUNC_NAME(unlocked_hit)(cache,id,lossy, dcc); ++ pthread_mutex_unlock(&cache->lock); ++ return hit; ++} ++ ++static int FUNC_NAME(unlocked_set_lossy)(CACHE *cache, uint64_t id, int lossy) ++{ ++ NewCacheItem *item; + + item = cache->hash_table[CACHE_HASH_KEY(id)]; + +@@ -76,11 +82,10 @@ static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy) + } + item = item->next; + } +- pthread_mutex_unlock(&cache->lock); + return !!item; + } + +-static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, DisplayChannelClient *dcc) ++static int FUNC_NAME(unlocked_add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, DisplayChannelClient *dcc) + { + NewCacheItem *item; + uint64_t serial; +@@ -91,15 +96,12 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D + item = spice_new(NewCacheItem, 1); + serial = red_channel_client_get_message_serial(&dcc->common.base); + +- pthread_mutex_lock(&cache->lock); +- + if (cache->generation != dcc->CACH_GENERATION) { + if (!dcc->pending_pixmaps_sync) { + red_channel_client_pipe_add_type( + &dcc->common.base, PIPE_ITEM_TYPE_PIXMAP_SYNC); + dcc->pending_pixmaps_sync = TRUE; + } +- pthread_mutex_unlock(&cache->lock); + free(item); + return FALSE; + } +@@ -112,7 +114,6 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D + if (!(tail = (NewCacheItem *)ring_get_tail(&cache->lru)) || + tail->sync[dcc->common.id] == serial) { + cache->available += size; +- pthread_mutex_unlock(&cache->lock); + free(item); + return FALSE; + } +@@ -144,7 +145,6 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D + memset(item->sync, 0, sizeof(item->sync)); + item->sync[dcc->common.id] = serial; + cache->sync[dcc->common.id] = serial; +- pthread_mutex_unlock(&cache->lock); + return TRUE; + } + +diff --git a/server/red_worker.c b/server/red_worker.c +index 955cac2..93e3398 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -6750,9 +6750,9 @@ static inline void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc, + if ((image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) { + spice_assert(image->descriptor.width * image->descriptor.height > 0); + if (!(io_image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME)) { +- if (pixmap_cache_add(dcc->pixmap_cache, image->descriptor.id, +- image->descriptor.width * image->descriptor.height, is_lossy, +- dcc)) { ++ if (pixmap_cache_unlocked_add(dcc->pixmap_cache, image->descriptor.id, ++ image->descriptor.width * image->descriptor.height, is_lossy, ++ dcc)) { + io_image->descriptor.flags |= SPICE_IMAGE_FLAGS_CACHE_ME; + dcc->send_data.pixmap_cache_items[dcc->send_data.num_pixmap_cache_items++] = + image->descriptor.id; +@@ -6797,11 +6797,12 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m, + if (simage->descriptor.flags & SPICE_IMAGE_FLAGS_HIGH_BITS_SET) { + image.descriptor.flags = SPICE_IMAGE_FLAGS_HIGH_BITS_SET; + } ++ pthread_mutex_lock(&dcc->pixmap_cache->lock); + + if ((simage->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) { + int lossy_cache_item; +- if (pixmap_cache_hit(dcc->pixmap_cache, image.descriptor.id, +- &lossy_cache_item, dcc)) { ++ if (pixmap_cache_unlocked_hit(dcc->pixmap_cache, image.descriptor.id, ++ &lossy_cache_item, dcc)) { + dcc->send_data.pixmap_cache_items[dcc->send_data.num_pixmap_cache_items++] = + image.descriptor.id; + if (can_lossy || !lossy_cache_item) { +@@ -6818,10 +6819,11 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m, + spice_assert(bitmap_palette_out == NULL); + spice_assert(lzplt_palette_out == NULL); + stat_inc_counter(display_channel->cache_hits_counter, 1); ++ pthread_mutex_unlock(&dcc->pixmap_cache->lock); + return FILL_BITS_TYPE_CACHE; + } else { +- pixmap_cache_set_lossy(dcc->pixmap_cache, simage->descriptor.id, +- FALSE); ++ pixmap_cache_unlocked_set_lossy(dcc->pixmap_cache, simage->descriptor.id, ++ FALSE); + image.descriptor.flags |= SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME; + } + } +@@ -6835,6 +6837,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m, + surface_id = simage->u.surface.surface_id; + if (!validate_surface(worker, surface_id)) { + rendering_incorrect("SPICE_IMAGE_TYPE_SURFACE"); ++ pthread_mutex_unlock(&dcc->pixmap_cache->lock); + return FILL_BITS_TYPE_SURFACE; + } + +@@ -6849,6 +6852,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m, + &bitmap_palette_out, &lzplt_palette_out); + spice_assert(bitmap_palette_out == NULL); + spice_assert(lzplt_palette_out == NULL); ++ pthread_mutex_unlock(&dcc->pixmap_cache->lock); + return FILL_BITS_TYPE_SURFACE; + } + case SPICE_IMAGE_TYPE_BITMAP: { +@@ -6879,6 +6883,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m, + } + + spice_marshaller_add_ref_chunks(m, bitmap->data); ++ pthread_mutex_unlock(&dcc->pixmap_cache->lock); + return FILL_BITS_TYPE_BITMAP; + } else { + red_display_add_image_to_pixmap_cache(rcc, simage, &image, +@@ -6896,6 +6901,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m, + } + + spice_assert(!comp_send_data.is_lossy || can_lossy); ++ pthread_mutex_unlock(&dcc->pixmap_cache->lock); + return (comp_send_data.is_lossy ? FILL_BITS_TYPE_COMPRESS_LOSSY : + FILL_BITS_TYPE_COMPRESS_LOSSLESS); + } +@@ -6909,11 +6915,12 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m, + spice_assert(bitmap_palette_out == NULL); + spice_assert(lzplt_palette_out == NULL); + spice_marshaller_add_ref_chunks(m, image.u.quic.data); ++ pthread_mutex_unlock(&dcc->pixmap_cache->lock); + return FILL_BITS_TYPE_COMPRESS_LOSSLESS; + default: + spice_error("invalid image type %u", image.descriptor.type); + } +- ++ pthread_mutex_unlock(&dcc->pixmap_cache->lock); + return 0; + } + +-- +2.4.4 + 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 deleted file mode 100644 index 822ab6d..0000000 --- a/SOURCES/0045-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch +++ /dev/null @@ -1,48 +0,0 @@ -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/0045-reds-Assure-we-don-t-have-stale-statistic-files-befo.patch b/SOURCES/0045-reds-Assure-we-don-t-have-stale-statistic-files-befo.patch new file mode 100644 index 0000000..951a92e --- /dev/null +++ b/SOURCES/0045-reds-Assure-we-don-t-have-stale-statistic-files-befo.patch @@ -0,0 +1,37 @@ +From 40537f6a3e3389b8377b0ae790c62ea0da8aa6d8 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Wed, 15 Jul 2015 14:15:52 +0100 +Subject: [PATCH] reds: Assure we don't have stale statistic files before + trying to create a new one + +If a previous Qemu executable is not able to delete the statistic file +on the next creation with same name (statitics file are based on pid +numbers so if pid get reused for another Qemu process you get the same +name) it fails as you can't open a file with 0444 permissions (these +are the permission used to create these files). +This patch assure there are no stale file trying to remove it before the +creation of the new one. As file is based on pid and name used for spice +you are not deleting another file. + +Fixes: rhbz#1177326 + +Signed-off-by: Frediano Ziglio +--- + server/reds.c | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/server/reds.c b/server/reds.c +index 57ef07a..c74894a 100644 +--- a/server/reds.c ++++ b/server/reds.c +@@ -3291,6 +3291,7 @@ static int do_spice_init(SpiceCoreInterface *core_interface) + shm_name_len = strlen(SPICE_STAT_SHM_NAME) + 20; + reds->stat_shm_name = (char *)spice_malloc(shm_name_len); + snprintf(reds->stat_shm_name, shm_name_len, SPICE_STAT_SHM_NAME, getpid()); ++ shm_unlink(reds->stat_shm_name); + if ((fd = shm_open(reds->stat_shm_name, O_CREAT | O_RDWR, 0444)) == -1) { + spice_error("statistics shm_open failed, %s", strerror(errno)); + } +-- +2.1.0 + 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 deleted file mode 100644 index 279aeca..0000000 --- a/SOURCES/0046-Fix-race-condition-on-red_get_clip_rects.patch +++ /dev/null @@ -1,42 +0,0 @@ -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/0046-worker-validate-correctly-surfaces.patch b/SOURCES/0046-worker-validate-correctly-surfaces.patch new file mode 100644 index 0000000..f7a89d8 --- /dev/null +++ b/SOURCES/0046-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/0047-Fix-race-in-red_get_image.patch b/SOURCES/0047-Fix-race-in-red_get_image.patch deleted file mode 100644 index abaf5b3..0000000 --- a/SOURCES/0047-Fix-race-in-red_get_image.patch +++ /dev/null @@ -1,77 +0,0 @@ -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/0047-worker-avoid-double-free-or-double-create-of-surface.patch b/SOURCES/0047-worker-avoid-double-free-or-double-create-of-surface.patch new file mode 100644 index 0000000..2ef2398 --- /dev/null +++ b/SOURCES/0047-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/0048-Define-a-constant-to-limit-data-from-guest.patch b/SOURCES/0048-Define-a-constant-to-limit-data-from-guest.patch new file mode 100644 index 0000000..d306102 --- /dev/null +++ b/SOURCES/0048-Define-a-constant-to-limit-data-from-guest.patch @@ -0,0 +1,42 @@ +From 2e1cac5508cd04815e0e624cdbc436857934f689 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 11:58:11 +0100 +Subject: [PATCH 48/64] 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/0048-Fix-race-condition-in-red_get_string.patch b/SOURCES/0048-Fix-race-condition-in-red_get_string.patch deleted file mode 100644 index 8bc6bb5..0000000 --- a/SOURCES/0048-Fix-race-condition-in-red_get_string.patch +++ /dev/null @@ -1,62 +0,0 @@ -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 deleted file mode 100644 index 9bf8b82..0000000 --- a/SOURCES/0049-Fix-integer-overflow-computing-glyph_size-in-red_get.patch +++ /dev/null @@ -1,63 +0,0 @@ -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/0049-Fix-some-integer-overflow-causing-large-memory-alloc.patch b/SOURCES/0049-Fix-some-integer-overflow-causing-large-memory-alloc.patch new file mode 100644 index 0000000..6b24d9e --- /dev/null +++ b/SOURCES/0049-Fix-some-integer-overflow-causing-large-memory-alloc.patch @@ -0,0 +1,69 @@ +From c0860a1bce777a2b54a31a01d09a71e80657b5ff Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Thu, 17 Sep 2015 15:00:22 +0100 +Subject: [PATCH 49/64] 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/0050-Check-properly-surface-to-be-created.patch b/SOURCES/0050-Check-properly-surface-to-be-created.patch new file mode 100644 index 0000000..f40ce03 --- /dev/null +++ b/SOURCES/0050-Check-properly-surface-to-be-created.patch @@ -0,0 +1,78 @@ +From b541f856f5c7fd70c1dad688306bcdfadfc2e4b9 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 16:02:59 +0100 +Subject: [PATCH 50/64] 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/0050-Fix-race-condition-in-red_get_data_chunks_ptr.patch b/SOURCES/0050-Fix-race-condition-in-red_get_data_chunks_ptr.patch deleted file mode 100644 index 71b0a38..0000000 --- a/SOURCES/0050-Fix-race-condition-in-red_get_data_chunks_ptr.patch +++ /dev/null @@ -1,66 +0,0 @@ -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-Fix-buffer-reading-overflow.patch b/SOURCES/0051-Fix-buffer-reading-overflow.patch new file mode 100644 index 0000000..db3b272 --- /dev/null +++ b/SOURCES/0051-Fix-buffer-reading-overflow.patch @@ -0,0 +1,38 @@ +From 214a0c88ecbac0229ddc2d7067a8a3ce9821df5c Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:00:37 +0100 +Subject: [PATCH 51/64] 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/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 deleted file mode 100644 index 331c3a2..0000000 --- a/SOURCES/0051-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch +++ /dev/null @@ -1,75 +0,0 @@ -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-32-bit-integer-overflow-in-bitmap_consistent.patch b/SOURCES/0052-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch new file mode 100644 index 0000000..e075eb4 --- /dev/null +++ b/SOURCES/0052-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch @@ -0,0 +1,48 @@ +From 0ede6619b828335a10ba94fe2739e01d5162d81c Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 13:09:35 +0100 +Subject: [PATCH 52/64] 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/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 deleted file mode 100644 index eb26991..0000000 --- a/SOURCES/0052-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch +++ /dev/null @@ -1,102 +0,0 @@ -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-race-condition-on-red_get_clip_rects.patch b/SOURCES/0053-Fix-race-condition-on-red_get_clip_rects.patch new file mode 100644 index 0000000..f358959 --- /dev/null +++ b/SOURCES/0053-Fix-race-condition-on-red_get_clip_rects.patch @@ -0,0 +1,42 @@ +From eede1d8307b48016274077eea4dd9c7a296bf84a Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:01:51 +0100 +Subject: [PATCH 53/64] 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/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 deleted file mode 100644 index 6875045..0000000 --- a/SOURCES/0053-Fix-some-possible-overflows-in-red_get_string-for-32.patch +++ /dev/null @@ -1,41 +0,0 @@ -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-Fix-race-in-red_get_image.patch b/SOURCES/0054-Fix-race-in-red_get_image.patch new file mode 100644 index 0000000..ac9cc18 --- /dev/null +++ b/SOURCES/0054-Fix-race-in-red_get_image.patch @@ -0,0 +1,77 @@ +From e4112afbe0f55df29068933e471fe348f79d4f04 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:04:10 +0100 +Subject: [PATCH 54/64] 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/0054-Make-sure-we-can-read-QXLPathSeg-structures.patch b/SOURCES/0054-Make-sure-we-can-read-QXLPathSeg-structures.patch deleted file mode 100644 index f363968..0000000 --- a/SOURCES/0054-Make-sure-we-can-read-QXLPathSeg-structures.patch +++ /dev/null @@ -1,40 +0,0 @@ -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 deleted file mode 100644 index 51c989c..0000000 --- a/SOURCES/0055-Avoid-race-condition-copying-segments-in-red_get_pat.patch +++ /dev/null @@ -1,31 +0,0 @@ -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/0055-Fix-race-condition-in-red_get_string.patch b/SOURCES/0055-Fix-race-condition-in-red_get_string.patch new file mode 100644 index 0000000..a8a6cb4 --- /dev/null +++ b/SOURCES/0055-Fix-race-condition-in-red_get_string.patch @@ -0,0 +1,62 @@ +From ee52db855a3b25965d5e363d64831864efe3f4f3 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:05:20 +0100 +Subject: [PATCH 55/64] 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/0056-Fix-integer-overflow-computing-glyph_size-in-red_get.patch b/SOURCES/0056-Fix-integer-overflow-computing-glyph_size-in-red_get.patch new file mode 100644 index 0000000..24c253f --- /dev/null +++ b/SOURCES/0056-Fix-integer-overflow-computing-glyph_size-in-red_get.patch @@ -0,0 +1,63 @@ +From d6307f37304bddf6d76c42a6bdd8475ccab9582d Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 10:13:24 +0100 +Subject: [PATCH 56/64] 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/0056-Prevent-data_size-to-be-set-independently-from-data.patch b/SOURCES/0056-Prevent-data_size-to-be-set-independently-from-data.patch deleted file mode 100644 index a78ea9a..0000000 --- a/SOURCES/0056-Prevent-data_size-to-be-set-independently-from-data.patch +++ /dev/null @@ -1,29 +0,0 @@ -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-Fix-race-condition-in-red_get_data_chunks_ptr.patch b/SOURCES/0057-Fix-race-condition-in-red_get_data_chunks_ptr.patch new file mode 100644 index 0000000..d128555 --- /dev/null +++ b/SOURCES/0057-Fix-race-condition-in-red_get_data_chunks_ptr.patch @@ -0,0 +1,66 @@ +From a1ead07b6a0facefafb959542f3088f427c7eb2d Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 12:12:19 +0100 +Subject: [PATCH 57/64] 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/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 deleted file mode 100644 index b7bf58f..0000000 --- a/SOURCES/0057-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch +++ /dev/null @@ -1,34 +0,0 @@ -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/SOURCES/0058-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch b/SOURCES/0058-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch new file mode 100644 index 0000000..f28db57 --- /dev/null +++ b/SOURCES/0058-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch @@ -0,0 +1,75 @@ +From 098e6e9e479a1188c5b5050ac8a0d4f495ec4842 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 12:14:55 +0100 +Subject: [PATCH 58/64] 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/0059-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch b/SOURCES/0059-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch new file mode 100644 index 0000000..2f11b59 --- /dev/null +++ b/SOURCES/0059-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch @@ -0,0 +1,102 @@ +From f89da55f4c4fcc09f201562854f3768911fdab07 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 12:28:54 +0100 +Subject: [PATCH 59/64] 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/0060-Fix-some-possible-overflows-in-red_get_string-for-32.patch b/SOURCES/0060-Fix-some-possible-overflows-in-red_get_string-for-32.patch new file mode 100644 index 0000000..b94f03f --- /dev/null +++ b/SOURCES/0060-Fix-some-possible-overflows-in-red_get_string-for-32.patch @@ -0,0 +1,41 @@ +From b61b33beae8b7560d05fe0d8349446bc16d08c62 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 8 Sep 2015 13:06:03 +0100 +Subject: [PATCH 60/64] 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/0061-Make-sure-we-can-read-QXLPathSeg-structures.patch b/SOURCES/0061-Make-sure-we-can-read-QXLPathSeg-structures.patch new file mode 100644 index 0000000..ad6bc16 --- /dev/null +++ b/SOURCES/0061-Make-sure-we-can-read-QXLPathSeg-structures.patch @@ -0,0 +1,40 @@ +From 13dadd08709caa8e736cbbc886f5f8ba7bad7785 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 15 Sep 2015 16:25:17 +0100 +Subject: [PATCH 61/64] 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/0062-Avoid-race-condition-copying-segments-in-red_get_pat.patch b/SOURCES/0062-Avoid-race-condition-copying-segments-in-red_get_pat.patch new file mode 100644 index 0000000..25da97b --- /dev/null +++ b/SOURCES/0062-Avoid-race-condition-copying-segments-in-red_get_pat.patch @@ -0,0 +1,31 @@ +From a2510f4df1c01a48515504c25cd9f0d9d1e839d0 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Tue, 15 Sep 2015 16:38:23 +0100 +Subject: [PATCH 62/64] 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/0063-Prevent-data_size-to-be-set-independently-from-data.patch b/SOURCES/0063-Prevent-data_size-to-be-set-independently-from-data.patch new file mode 100644 index 0000000..1ccbb11 --- /dev/null +++ b/SOURCES/0063-Prevent-data_size-to-be-set-independently-from-data.patch @@ -0,0 +1,29 @@ +From acda240cbeb8a15aafd73b55a22fd3dfa7f03df1 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Thu, 17 Sep 2015 14:28:36 +0100 +Subject: [PATCH 63/64] 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/0064-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch b/SOURCES/0064-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch new file mode 100644 index 0000000..7b1a381 --- /dev/null +++ b/SOURCES/0064-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch @@ -0,0 +1,34 @@ +From ffa092aa729613de9fe9f7f59beea4338b324f5c Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Thu, 17 Sep 2015 15:01:05 +0100 +Subject: [PATCH 64/64] 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 f24555c..25eeb81 100644 --- a/SPECS/spice.spec +++ b/SPECS/spice.spec @@ -1,6 +1,6 @@ Name: spice Version: 0.12.4 -Release: 9%{?dist}.3 +Release: 15%{?dist} Summary: Implements the SPICE protocol Group: User Interface/Desktops License: LGPLv2+ @@ -43,26 +43,34 @@ Patch34: 0034-Validate-surface-bounding-box-before-using-it.patch 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 +Patch38: 0038-Add-const-to-test_capability-first-argument.patch +Patch39: 0039-Introduce-red_link_info_test_capability.patch +Patch40: 0040-Don-t-set-SpiceLinkReply-pub_key-if-client-advertise.patch +Patch41: 0041-server-don-t-assert-on-invalid-client-message.patch +Patch42: 0042-Don-t-truncate-large-now-values-in-_spice_timer_set.patch +Patch43: 0043-Avoid-race-conditions-reading-monitor-configs-from-g.patch +Patch44: 0044-Lock-the-pixmap-image-cache-for-the-entire-fill_bits.patch +Patch45: 0045-reds-Assure-we-don-t-have-stale-statistic-files-befo.patch +Patch46: 0046-worker-validate-correctly-surfaces.patch +Patch47: 0047-worker-avoid-double-free-or-double-create-of-surface.patch +Patch48: 0048-Define-a-constant-to-limit-data-from-guest.patch +Patch49: 0049-Fix-some-integer-overflow-causing-large-memory-alloc.patch +Patch50: 0050-Check-properly-surface-to-be-created.patch +Patch51: 0051-Fix-buffer-reading-overflow.patch +Patch52: 0052-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch +Patch53: 0053-Fix-race-condition-on-red_get_clip_rects.patch +Patch54: 0054-Fix-race-in-red_get_image.patch +Patch55: 0055-Fix-race-condition-in-red_get_string.patch +Patch56: 0056-Fix-integer-overflow-computing-glyph_size-in-red_get.patch +Patch57: 0057-Fix-race-condition-in-red_get_data_chunks_ptr.patch +Patch58: 0058-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch +Patch59: 0059-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch +Patch60: 0060-Fix-some-possible-overflows-in-red_get_string-for-32.patch +Patch61: 0061-Make-sure-we-can-read-QXLPathSeg-structures.patch +Patch62: 0062-Avoid-race-condition-copying-segments-in-red_get_pat.patch +Patch63: 0063-Prevent-data_size-to-be-set-independently-from-data.patch +Patch64: 0064-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch + # https://bugzilla.redhat.com/show_bug.cgi?id=613529 %if 0%{?rhel} @@ -175,6 +183,13 @@ using spice-server, you will need to install spice-server-devel. %patch55 -p1 %patch56 -p1 %patch57 -p1 +%patch58 -p1 +%patch59 -p1 +%patch60 -p1 +%patch61 -p1 +%patch62 -p1 +%patch63 -p1 +%patch64 -p1 %build @@ -205,18 +220,37 @@ mkdir -p %{buildroot}%{_libexecdir} %changelog -* Wed Sep 23 2015 Frediano Ziglio 0.12.4-9.3 +* Wed Sep 23 2015 Frediano Ziglio 0.12.4-15 - CVE-2015-5260 CVE-2015-5261 fixed various security flaws - Resolves: rhbz#1262771 + Resolves: rhbz#1267134 -* Tue Sep 15 2015 Frediano Ziglio 0.12.4-9.2 +* Thu Sep 10 2015 Frediano Ziglio 0.12.4-14 - 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 - Resolves: rhbz#1239127 + Resolves: rhbz#1260971 + +* Tue Jul 21 2015 Frediano Ziglio 0.12.4-13 +- Clean stale statistics file before creating a new one + Resolves: rhbz#1177326 + +* Fri Jul 10 2015 Fabiano Fidêncio 0.12.4-12 +- Fix a backport issue on Patch0040. + Related: rhbz#1071176 + Resolves: rhbz#1241860 + +* Thu Jul 09 2015 Fabiano Fidêncio 0.12.4-11 +- Don't assert on invalid client message + Resolves: rhbz#1227410 +- Don't truncate large 'now' values in _spice_timer_set + Resolves: rhbz#1227408 +- Avoid race conditions reading monitor configs from guest + Resolves: rhbz#1239128 +- Lock the pixmap image cache for the entire fill_bits call + Resolves: rhbz#1235443 + +* Wed Jul 08 2015 Fabiano Fidêncio 0.12.4-10 +- Fix qemu segmentation fault (core dumped) when boot KVM guest with + spice in FIPS enabled mode. + Resolves: rhbz#1071176 * Mon Jan 05 2015 Marc-Andre Lureau 0.12.4-9 - Allow recent TLS/SSL methods, block SSLv2/SSLv3. Resolves: rhbz#1175540