[Spice-devel] [PATCH] Avoid race conditions reading monitor configs from guest

[Christophe Fergeau]

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 at redhat.com>
> ---
>  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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150908/0d93c3f9/attachment.sig>