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 new file mode 100644 index 0000000..c909575 --- /dev/null +++ b/SOURCES/0038-Avoid-race-conditions-reading-monitor-configs-from-g.patch @@ -0,0 +1,117 @@ +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/SPECS/spice.spec b/SPECS/spice.spec index e982bdd..1fa2e80 100644 --- a/SPECS/spice.spec +++ b/SPECS/spice.spec @@ -1,6 +1,6 @@ Name: spice Version: 0.12.4 -Release: 9%{?dist} +Release: 9%{?dist}.1 Summary: Implements the SPICE protocol Group: User Interface/Desktops License: LGPLv2+ @@ -43,6 +43,7 @@ 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 # https://bugzilla.redhat.com/show_bug.cgi?id=613529 %if 0%{?rhel} @@ -135,6 +136,7 @@ using spice-server, you will need to install spice-server-devel. %patch35 -p1 %patch36 -p1 %patch37 -p1 +%patch38 -p1 %build @@ -165,6 +167,11 @@ mkdir -p %{buildroot}%{_libexecdir} %changelog +* 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 + * Mon Jan 05 2015 Marc-Andre Lureau 0.12.4-9 - Allow recent TLS/SSL methods, block SSLv2/SSLv3. Resolves: rhbz#1175540