[Spice-devel] [xf86 qxl driver PATCH 5/5] qxl_driver: monitors_config: adjust to memory-remap

[Yonit Halperin]

Hi,
On 01/17/2013 09:26 AM, Uri Lublin wrote:
> Resolves: rhbz#883578
>
> Call qxl_allocate_monitors_config upon memory-remap such
> that qxl->monitors_config points to the start of
> monitors_config segment in qxl RAM memory.
>
> Currently after memory remap, it's possible that monitors_config
> memory and video-memory (or graphics) overlap, which means
> that one may overwrite another.
> Specifically in the bug above, monitors_config value are being
> overwritten by video pages, and on the destination bad values
> are read which cause problems on the server and client.
>
Can you please explain the path that leads to this overwriting?
I see that in qxl_map_memory qxl_allocate_monitors_config is already called.
> It may be a good idea to add some protection on the server side,
> e.g. calcluate checksum, compare values against modes, or limit
> ->count and ->max_allowed and ignore bad monitors_config values
>
> Also do not memset-0 monitors-config upon allocation (remapping)
> to not overwrite likely good configuration (in case it is
> being read by the host, e.g. upon migration).
I'm not sure that the code in qemu-qxl should even re-read the monitors 
configuration during pre-save because it was already updated on the 
UPDATE_MONITORS_CONFIG io call.

Regards,
Yonit.
> ---
>   src/qxl_driver.c |   13 ++++++-------
>   1 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> index 05c357b..e1893dc 100644
> --- a/src/qxl_driver.c
> +++ b/src/qxl_driver.c
> @@ -134,6 +134,10 @@ const OptionInfoRec DefaultOptions[] =
>       { -1, NULL, OPTV_NONE, {0}, FALSE }
>   };
>
> +
> +static void qxl_update_monitors_config (qxl_screen_t *qxl);
> +
> +
>   static const OptionInfoRec *
>   qxl_available_options (int chipid, int busid)
>   {
> @@ -318,15 +322,8 @@ qxl_io_monitors_config_async (qxl_screen_t *qxl)
>   static void
>   qxl_allocate_monitors_config (qxl_screen_t *qxl)
>   {
> -    int size = sizeof (QXLMonitorsConfig) + sizeof (QXLHead) * MAX_MONITORS_NUM;
> -
> -    if (qxl->monitors_config)
> -	return;
> -
>       qxl->monitors_config = (QXLMonitorsConfig *)(void *)
>   	((unsigned long)qxl->ram + qxl->rom->ram_header_offset - qxl->monitors_config_size);
> -
> -    memset (qxl->monitors_config, 0, size);
>   }
>
>   static uint64_t
> @@ -845,6 +842,8 @@ qxl_reset_and_create_mem_slots (qxl_screen_t *qxl)
>                                        (uint64_t)(uintptr_t)qxl->vram,
>                                        (uint64_t)(uintptr_t)qxl->vram + (uint64_t)qxl->vram_size);
>   #endif
> +
> +    qxl_allocate_monitors_config(qxl);
>   }
>
>   static void
>