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

Submitted by Uri Lublin on Jan. 17, 2013, 2:26 p.m.

Details

Message ID 14b056ab8dac6aaf3dd59b7768b698708af040e7.1358430965.git.uril@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin Jan. 17, 2013, 2:26 p.m.
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.

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).
---
 src/qxl_driver.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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

Comments

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
>
On 01/21/2013 04:16 PM, Yonit Halperin wrote:
> 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.

Hi Yonit,

The source host does not re-reads the monitors configuration during 
pre-save.
The destination host reads it during post-reload.

The monitors_config area is migrated together with all the VM memory.
And the address to the monitors configuration is transferred as state.

Thanks,
     Uri.
On 01/21/2013 09:28 AM, Uri Lublin wrote:
> On 01/21/2013 04:16 PM, Yonit Halperin wrote:
>> 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.
>
> Hi Yonit,
>
> The source host does not re-reads the monitors configuration during
> pre-save.
> The destination host reads it during post-reload.
>
> The monitors_config area is migrated together with all the VM memory.
> And the address to the monitors configuration is transferred as state.
>
Yes, you are right, I forgot the monitor_config we save is the address 
and not the monitor configuration itself.
> Thanks,
>      Uri.
>
On 01/21/2013 04:16 PM, Yonit Halperin wrote:
> 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.

Hi Yonit,

Sorry, I missed this comment earlier.

Before this patch, qxl->monitors_config was set once when the first
memory mapping is done. Upon memory re-map it's possible (even
likely) that qxl->ram gets a different address. But qxl->monitors_config
still points to the first area. Which is probably allocated/mapped
to a different purpose, for example video-pages.


Lets assume the following:
   qxl ram size                         64MB is divided into:
     surface0 area size                          16MB               
(0x1000000)
     Video Pages (aka command ram)  48MB - 12KB   (0x2FFD000)
     monitors config                                 4KB               
(0x0001000)
     qxl ram header                                  8KB              
(0x0002000)

Assume that after the first mapping qxl->ram is        0x7F9C53350000
               and after the second mapping qxl->ram is   0x7F9C53400000

In this case monitors_config will point to 0x7F9C5734D000
     0x7F9C53350000
+           0x1000000
+           0x2FFD000
     -----------------------
     0x7F9C5734D000

and after memory remap Video Pages region will start at
    0x7F9C53400000
+          0x1000000
    -----------------------
    0x7F9C54400000

and end at
     0x7F9C53400000
+           0x1000000
+           0x2FFD000
     -----------------------
     0x7F9C573FD000

So after memory re-map, memory_config points to somewhere in the Video 
Pages area.

This is just an example.
I think that in any case where qxl->ram pointer changes upon memory 
re-map, there
will be memory corruption.

Thanks,
     Uri.