|
|
11e32f |
From 524eef10c6c6c2f3f30be28c56b8f96adc7901f0 Mon Sep 17 00:00:00 2001
|
|
|
11e32f |
From: Frediano Ziglio <fziglio@redhat.com>
|
|
|
11e32f |
Date: Tue, 9 Jun 2015 08:50:46 +0100
|
|
|
11e32f |
Subject: [PATCH] Avoid race conditions reading monitor configs from guest
|
|
|
11e32f |
|
|
|
11e32f |
For security reasons do not assume guest do not change structures it
|
|
|
11e32f |
pass to Qemu.
|
|
|
11e32f |
Guest could change count field while Qemu is copying QXLMonitorsConfig
|
|
|
11e32f |
structure leading to heap corruption.
|
|
|
11e32f |
This patch avoid it reading count only once.
|
|
|
11e32f |
|
|
|
11e32f |
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
|
|
11e32f |
---
|
|
|
11e32f |
server/red_worker.c | 46 ++++++++++++++++++++++++++++++++--------------
|
|
|
11e32f |
1 file changed, 32 insertions(+), 14 deletions(-)
|
|
|
11e32f |
|
|
|
11e32f |
diff --git a/server/red_worker.c b/server/red_worker.c
|
|
|
11e32f |
index 9e6a6ad..955cac2 100644
|
|
|
11e32f |
--- a/server/red_worker.c
|
|
|
11e32f |
+++ b/server/red_worker.c
|
|
|
11e32f |
@@ -11270,7 +11270,8 @@ static inline void red_monitors_config_item_add(DisplayChannelClient *dcc)
|
|
|
11e32f |
}
|
|
|
11e32f |
|
|
|
11e32f |
static void worker_update_monitors_config(RedWorker *worker,
|
|
|
11e32f |
- QXLMonitorsConfig *dev_monitors_config)
|
|
|
11e32f |
+ QXLMonitorsConfig *dev_monitors_config,
|
|
|
11e32f |
+ uint16_t count, uint16_t max_allowed)
|
|
|
11e32f |
{
|
|
|
11e32f |
int heads_size;
|
|
|
11e32f |
MonitorsConfig *monitors_config;
|
|
|
11e32f |
@@ -11279,22 +11280,22 @@ static void worker_update_monitors_config(RedWorker *worker,
|
|
|
11e32f |
monitors_config_decref(worker->monitors_config);
|
|
|
11e32f |
|
|
|
11e32f |
spice_debug("monitors config %d(%d)",
|
|
|
11e32f |
- dev_monitors_config->count,
|
|
|
11e32f |
- dev_monitors_config->max_allowed);
|
|
|
11e32f |
- for (i = 0; i < dev_monitors_config->count; i++) {
|
|
|
11e32f |
+ count,
|
|
|
11e32f |
+ max_allowed);
|
|
|
11e32f |
+ for (i = 0; i < count; i++) {
|
|
|
11e32f |
spice_debug("+%d+%d %dx%d",
|
|
|
11e32f |
dev_monitors_config->heads[i].x,
|
|
|
11e32f |
dev_monitors_config->heads[i].y,
|
|
|
11e32f |
dev_monitors_config->heads[i].width,
|
|
|
11e32f |
dev_monitors_config->heads[i].height);
|
|
|
11e32f |
}
|
|
|
11e32f |
- heads_size = dev_monitors_config->count * sizeof(QXLHead);
|
|
|
11e32f |
+ heads_size = count * sizeof(QXLHead);
|
|
|
11e32f |
worker->monitors_config = monitors_config =
|
|
|
11e32f |
spice_malloc(sizeof(*monitors_config) + heads_size);
|
|
|
11e32f |
monitors_config->refs = 1;
|
|
|
11e32f |
monitors_config->worker = worker;
|
|
|
11e32f |
- monitors_config->count = dev_monitors_config->count;
|
|
|
11e32f |
- monitors_config->max_allowed = dev_monitors_config->max_allowed;
|
|
|
11e32f |
+ monitors_config->count = count;
|
|
|
11e32f |
+ monitors_config->max_allowed = max_allowed;
|
|
|
11e32f |
memcpy(monitors_config->heads, dev_monitors_config->heads, heads_size);
|
|
|
11e32f |
}
|
|
|
11e32f |
|
|
|
11e32f |
@@ -11678,33 +11679,50 @@ void handle_dev_display_migrate(void *opaque, void *payload)
|
|
|
11e32f |
red_migrate_display(worker, rcc);
|
|
|
11e32f |
}
|
|
|
11e32f |
|
|
|
11e32f |
+static inline uint32_t qxl_monitors_config_size(uint32_t heads)
|
|
|
11e32f |
+{
|
|
|
11e32f |
+ return sizeof(QXLMonitorsConfig) + sizeof(QXLHead) * heads;
|
|
|
11e32f |
+}
|
|
|
11e32f |
+
|
|
|
11e32f |
static void handle_dev_monitors_config_async(void *opaque, void *payload)
|
|
|
11e32f |
{
|
|
|
11e32f |
RedWorkerMessageMonitorsConfigAsync *msg = payload;
|
|
|
11e32f |
RedWorker *worker = opaque;
|
|
|
11e32f |
- int min_size = sizeof(QXLMonitorsConfig) + sizeof(QXLHead);
|
|
|
11e32f |
int error;
|
|
|
11e32f |
+ uint16_t count, max_allowed;
|
|
|
11e32f |
QXLMonitorsConfig *dev_monitors_config =
|
|
|
11e32f |
(QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config,
|
|
|
11e32f |
- min_size, msg->group_id, &error);
|
|
|
11e32f |
+ qxl_monitors_config_size(1),
|
|
|
11e32f |
+ msg->group_id, &error);
|
|
|
11e32f |
|
|
|
11e32f |
if (error) {
|
|
|
11e32f |
/* TODO: raise guest bug (requires added QXL interface) */
|
|
|
11e32f |
return;
|
|
|
11e32f |
}
|
|
|
11e32f |
worker->driver_cap_monitors_config = 1;
|
|
|
11e32f |
- if (dev_monitors_config->count == 0) {
|
|
|
11e32f |
+ count = dev_monitors_config->count;
|
|
|
11e32f |
+ max_allowed = dev_monitors_config->max_allowed;
|
|
|
11e32f |
+ if (count == 0) {
|
|
|
11e32f |
spice_warning("ignoring an empty monitors config message from driver");
|
|
|
11e32f |
return;
|
|
|
11e32f |
}
|
|
|
11e32f |
- if (dev_monitors_config->count > dev_monitors_config->max_allowed) {
|
|
|
11e32f |
+ if (count > max_allowed) {
|
|
|
11e32f |
spice_warning("ignoring malformed monitors_config from driver, "
|
|
|
11e32f |
"count > max_allowed %d > %d",
|
|
|
11e32f |
- dev_monitors_config->count,
|
|
|
11e32f |
- dev_monitors_config->max_allowed);
|
|
|
11e32f |
+ count,
|
|
|
11e32f |
+ max_allowed);
|
|
|
11e32f |
return;
|
|
|
11e32f |
}
|
|
|
11e32f |
- worker_update_monitors_config(worker, dev_monitors_config);
|
|
|
11e32f |
+ /* get pointer again to check virtual size */
|
|
|
11e32f |
+ dev_monitors_config =
|
|
|
11e32f |
+ (QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config,
|
|
|
11e32f |
+ qxl_monitors_config_size(count),
|
|
|
11e32f |
+ msg->group_id, &error);
|
|
|
11e32f |
+ if (error) {
|
|
|
11e32f |
+ /* TODO: raise guest bug (requires added QXL interface) */
|
|
|
11e32f |
+ return;
|
|
|
11e32f |
+ }
|
|
|
11e32f |
+ worker_update_monitors_config(worker, dev_monitors_config, count, max_allowed);
|
|
|
11e32f |
red_worker_push_monitors_config(worker);
|
|
|
11e32f |
}
|
|
|
11e32f |
|