ACK.
On Tue, Sep 08, 2015 at 11:11:41AM +0100, Frediano Ziglio wrote:
> 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.
>
> This patch solves CVE-2015-3247.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/red_worker.c | 44 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 2f2d5a9..e2feb23 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -11222,19 +11222,18 @@ static inline void red_monitors_config_item_add(DisplayChannelClient *dcc)
>
> static void worker_update_monitors_config(RedWorker *worker,
> QXLMonitorsConfig *dev_monitors_config,
> - unsigned int max_monitors)
> + uint16_t count, uint16_t max_allowed)
> {
> int heads_size;
> MonitorsConfig *monitors_config;
> int i;
> - unsigned int count = MIN(dev_monitors_config->count, max_monitors);
>
> 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,
> @@ -11247,7 +11246,7 @@ static void worker_update_monitors_config(RedWorker *worker,
> monitors_config->refs = 1;
> monitors_config->worker = worker;
> monitors_config->count = count;
> - monitors_config->max_allowed = MIN(dev_monitors_config->max_allowed, max_monitors);
> + monitors_config->max_allowed = max_allowed;
> memcpy(monitors_config->heads, dev_monitors_config->heads, heads_size);
> }
>
> @@ -11636,33 +11635,52 @@ 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, msg->max_monitors);
> + worker_update_monitors_config(worker, dev_monitors_config,
> + MIN(count, msg->max_monitors),
> + MIN(max_allowed, msg->max_monitors));
> red_worker_push_monitors_config(worker);
> }
>
> --
> 2.4.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
pgpAeaylhZ8v_.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel