[PATCH 0/2] qxl: map rom r/o, remove shadow.

[PATCH 0/2] qxl: map rom r/o, remove shadow.

[Gerd Hoffmann]

Gerd Hoffmann (2):
  qxl: map rom r/o
  qxl: drop shadow_rom

 hw/display/qxl.h |  2 +-
 hw/display/qxl.c | 27 ++++++++++-----------------
 2 files changed, 11 insertions(+), 18 deletions(-)

-- 
2.18.2

[PATCH 1/2] qxl: map rom r/o

[Gerd Hoffmann]

Map qxl rom read-only into the guest, so the guest can't tamper with the
content.  qxl has a shadow copy of the rom to deal with that, but the
shadow doesn't cover the mode list.  A privilidged user in the guest can
manipulate the mode list and that to trick qemu into oob reads, leading
to a DoS via segfault if that read access happens to hit unmapped memory.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 21a43a1d5ec2..227da69a50d9 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2136,7 +2136,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
     pci_set_byte(&config[PCI_INTERRUPT_PIN], 1);
 
     qxl->rom_size = qxl_rom_size();
-    memory_region_init_ram(&qxl->rom_bar, OBJECT(qxl), "qxl.vrom",
+    memory_region_init_rom(&qxl->rom_bar, OBJECT(qxl), "qxl.vrom",
                            qxl->rom_size, &error_fatal);
     init_qxl_rom(qxl);
     init_qxl_ram(qxl);
-- 
2.18.2

[PATCH 2/2] qxl: drop shadow_rom

[Gerd Hoffmann]

Now that the rom bar is mapped read-only and the guest can't change
things under our feet we don't need the shadow rom any more.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.h |  2 +-
 hw/display/qxl.c | 25 +++++++++----------------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index 707631a1f573..3aedc7db5da0 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -95,11 +95,11 @@ typedef struct PCIQXLDevice {
     uint32_t           vgamem_size;
 
     /* rom pci bar */
-    QXLRom             shadow_rom;
     QXLRom             *rom;
     QXLModes           *modes;
     uint32_t           rom_size;
     MemoryRegion       rom_bar;
+    uint32_t           rom_mode;
 #if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
     uint16_t           max_outputs;
 #endif
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 227da69a50d9..0502802688f9 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -391,7 +391,6 @@ static void init_qxl_rom(PCIQXLDevice *d)
             sizeof(rom->client_monitors_config));
     }
 
-    d->shadow_rom = *rom;
     d->rom        = rom;
     d->modes      = modes;
 }
@@ -403,7 +402,7 @@ static void init_qxl_ram(PCIQXLDevice *d)
     QXLReleaseRing *ring;
 
     buf = d->vga.vram_ptr;
-    d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
+    d->ram = (QXLRam *)(buf + le32_to_cpu(d->rom->ram_header_offset));
     d->ram->magic       = cpu_to_le32(QXL_RAM_MAGIC);
     d->ram->int_pending = cpu_to_le32(0);
     d->ram->int_mask    = cpu_to_le32(0);
@@ -446,7 +445,7 @@ static void qxl_ram_set_dirty(PCIQXLDevice *qxl, void *ptr)
 /* can be called from spice server thread context */
 static void qxl_ring_set_dirty(PCIQXLDevice *qxl)
 {
-    ram_addr_t addr = qxl->shadow_rom.ram_header_offset;
+    ram_addr_t addr = qxl->rom->ram_header_offset;
     ram_addr_t end  = qxl->vga.vram_size;
     qxl_set_dirty(&qxl->vga.vram, addr, end);
 }
@@ -529,7 +528,6 @@ static void interface_set_compression_level(QXLInstance *sin, int level)
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
 
     trace_qxl_interface_set_compression_level(qxl->id, level);
-    qxl->shadow_rom.compression_level = cpu_to_le32(level);
     qxl->rom->compression_level = cpu_to_le32(level);
     qxl_rom_set_dirty(qxl);
 }
@@ -561,7 +559,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
     info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
     info->internal_groupslot_id = 0;
     info->qxl_ram_size =
-        le32_to_cpu(qxl->shadow_rom.num_pages) << QXL_PAGE_BITS;
+        le32_to_cpu(qxl->rom->num_pages) << QXL_PAGE_BITS;
     info->n_surfaces = qxl->ssd.num_surfaces;
 }
 
@@ -1035,9 +1033,6 @@ static void interface_set_client_capabilities(QXLInstance *sin,
         return;
     }
 
-    qxl->shadow_rom.client_present = client_present;
-    memcpy(qxl->shadow_rom.client_capabilities, caps,
-           sizeof(qxl->shadow_rom.client_capabilities));
     qxl->rom->client_present = client_present;
     memcpy(qxl->rom->client_capabilities, caps,
            sizeof(qxl->rom->client_capabilities));
@@ -1232,11 +1227,8 @@ static void qxl_check_state(PCIQXLDevice *d)
 
 static void qxl_reset_state(PCIQXLDevice *d)
 {
-    QXLRom *rom = d->rom;
-
     qxl_check_state(d);
-    d->shadow_rom.update_id = cpu_to_le32(0);
-    *rom = d->shadow_rom;
+    d->rom->update_id = cpu_to_le32(0);
     qxl_rom_set_dirty(d);
     init_qxl_ram(d);
     d->num_free_res = 0;
@@ -1600,7 +1592,7 @@ static void qxl_set_mode(PCIQXLDevice *d, unsigned int modenr, int loadvm)
         .format     = SPICE_SURFACE_FMT_32_xRGB,
         .flags      = loadvm ? QXL_SURF_FLAG_KEEP_DATA : 0,
         .mouse_mode = true,
-        .mem        = devmem + d->shadow_rom.draw_area_offset,
+        .mem        = devmem + d->rom->draw_area_offset,
     };
 
     trace_qxl_set_mode(d->id, modenr, mode->x_res, mode->y_res, mode->bits,
@@ -1620,7 +1612,6 @@ static void qxl_set_mode(PCIQXLDevice *d, unsigned int modenr, int loadvm)
     if (mode->bits == 16) {
         d->cmdflags |= QXL_COMMAND_FLAG_COMPAT_16BPP;
     }
-    d->shadow_rom.mode = cpu_to_le32(modenr);
     d->rom->mode = cpu_to_le32(modenr);
     qxl_rom_set_dirty(d);
 }
@@ -2277,6 +2268,7 @@ static int qxl_pre_save(void *opaque)
         d->last_release_offset = (uint8_t *)d->last_release - ram_start;
     }
     assert(d->last_release_offset < d->vga.vram_size);
+    d->rom_mode = d->rom->mode;
 
     return 0;
 }
@@ -2316,6 +2308,7 @@ static int qxl_post_load(void *opaque, int version)
     } else {
         d->last_release = (QXLReleaseInfo *)(ram_start + d->last_release_offset);
     }
+    d->rom->mode = d->rom_mode;
 
     d->modes = (QXLModes*)((uint8_t*)d->rom + d->rom->modes_offset);
 
@@ -2361,7 +2354,7 @@ static int qxl_post_load(void *opaque, int version)
     case QXL_MODE_COMPAT:
         /* note: no need to call qxl_create_memslots, qxl_set_mode
          * creates the mem slot. */
-        qxl_set_mode(d, d->shadow_rom.mode, 1);
+        qxl_set_mode(d, d->rom->mode, 1);
         break;
     }
     return 0;
@@ -2428,7 +2421,7 @@ static VMStateDescription qxl_vmstate = {
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(pci, PCIQXLDevice),
         VMSTATE_STRUCT(vga, PCIQXLDevice, 0, vmstate_vga_common, VGACommonState),
-        VMSTATE_UINT32(shadow_rom.mode, PCIQXLDevice),
+        VMSTATE_UINT32(rom_mode, PCIQXLDevice),
         VMSTATE_UINT32(num_free_res, PCIQXLDevice),
         VMSTATE_UINT32(last_release_offset, PCIQXLDevice),
         VMSTATE_UINT32(mode, PCIQXLDevice),
-- 
2.18.2

Re: [PATCH 2/2] qxl: drop shadow_rom

[Paolo Bonzini]

On 25/02/20 06:59, Gerd Hoffmann wrote:
> Now that the rom bar is mapped read-only and the guest can't change
> things under our feet we don't need the shadow rom any more.

Can't it do so when migrating from an older version?

Paolo

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl.h |  2 +-
>  hw/display/qxl.c | 25 +++++++++----------------
>  2 files changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index 707631a1f573..3aedc7db5da0 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -95,11 +95,11 @@ typedef struct PCIQXLDevice {
>      uint32_t           vgamem_size;
>  
>      /* rom pci bar */
> -    QXLRom             shadow_rom;
>      QXLRom             *rom;
>      QXLModes           *modes;
>      uint32_t           rom_size;
>      MemoryRegion       rom_bar;
> +    uint32_t           rom_mode;
>  #if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
>      uint16_t           max_outputs;
>  #endif
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 227da69a50d9..0502802688f9 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -391,7 +391,6 @@ static void init_qxl_rom(PCIQXLDevice *d)
>              sizeof(rom->client_monitors_config));
>      }
>  
> -    d->shadow_rom = *rom;
>      d->rom        = rom;
>      d->modes      = modes;
>  }
> @@ -403,7 +402,7 @@ static void init_qxl_ram(PCIQXLDevice *d)
>      QXLReleaseRing *ring;
>  
>      buf = d->vga.vram_ptr;
> -    d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
> +    d->ram = (QXLRam *)(buf + le32_to_cpu(d->rom->ram_header_offset));
>      d->ram->magic       = cpu_to_le32(QXL_RAM_MAGIC);
>      d->ram->int_pending = cpu_to_le32(0);
>      d->ram->int_mask    = cpu_to_le32(0);
> @@ -446,7 +445,7 @@ static void qxl_ram_set_dirty(PCIQXLDevice *qxl, void *ptr)
>  /* can be called from spice server thread context */
>  static void qxl_ring_set_dirty(PCIQXLDevice *qxl)
>  {
> -    ram_addr_t addr = qxl->shadow_rom.ram_header_offset;
> +    ram_addr_t addr = qxl->rom->ram_header_offset;
>      ram_addr_t end  = qxl->vga.vram_size;
>      qxl_set_dirty(&qxl->vga.vram, addr, end);
>  }
> @@ -529,7 +528,6 @@ static void interface_set_compression_level(QXLInstance *sin, int level)
>      PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
>  
>      trace_qxl_interface_set_compression_level(qxl->id, level);
> -    qxl->shadow_rom.compression_level = cpu_to_le32(level);
>      qxl->rom->compression_level = cpu_to_le32(level);
>      qxl_rom_set_dirty(qxl);
>  }
> @@ -561,7 +559,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>      info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
>      info->internal_groupslot_id = 0;
>      info->qxl_ram_size =
> -        le32_to_cpu(qxl->shadow_rom.num_pages) << QXL_PAGE_BITS;
> +        le32_to_cpu(qxl->rom->num_pages) << QXL_PAGE_BITS;
>      info->n_surfaces = qxl->ssd.num_surfaces;
>  }
>  
> @@ -1035,9 +1033,6 @@ static void interface_set_client_capabilities(QXLInstance *sin,
>          return;
>      }
>  
> -    qxl->shadow_rom.client_present = client_present;
> -    memcpy(qxl->shadow_rom.client_capabilities, caps,
> -           sizeof(qxl->shadow_rom.client_capabilities));
>      qxl->rom->client_present = client_present;
>      memcpy(qxl->rom->client_capabilities, caps,
>             sizeof(qxl->rom->client_capabilities));
> @@ -1232,11 +1227,8 @@ static void qxl_check_state(PCIQXLDevice *d)
>  
>  static void qxl_reset_state(PCIQXLDevice *d)
>  {
> -    QXLRom *rom = d->rom;
> -
>      qxl_check_state(d);
> -    d->shadow_rom.update_id = cpu_to_le32(0);
> -    *rom = d->shadow_rom;
> +    d->rom->update_id = cpu_to_le32(0);
>      qxl_rom_set_dirty(d);
>      init_qxl_ram(d);
>      d->num_free_res = 0;
> @@ -1600,7 +1592,7 @@ static void qxl_set_mode(PCIQXLDevice *d, unsigned int modenr, int loadvm)
>          .format     = SPICE_SURFACE_FMT_32_xRGB,
>          .flags      = loadvm ? QXL_SURF_FLAG_KEEP_DATA : 0,
>          .mouse_mode = true,
> -        .mem        = devmem + d->shadow_rom.draw_area_offset,
> +        .mem        = devmem + d->rom->draw_area_offset,
>      };
>  
>      trace_qxl_set_mode(d->id, modenr, mode->x_res, mode->y_res, mode->bits,
> @@ -1620,7 +1612,6 @@ static void qxl_set_mode(PCIQXLDevice *d, unsigned int modenr, int loadvm)
>      if (mode->bits == 16) {
>          d->cmdflags |= QXL_COMMAND_FLAG_COMPAT_16BPP;
>      }
> -    d->shadow_rom.mode = cpu_to_le32(modenr);
>      d->rom->mode = cpu_to_le32(modenr);
>      qxl_rom_set_dirty(d);
>  }
> @@ -2277,6 +2268,7 @@ static int qxl_pre_save(void *opaque)
>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>      }
>      assert(d->last_release_offset < d->vga.vram_size);
> +    d->rom_mode = d->rom->mode;
>  
>      return 0;
>  }
> @@ -2316,6 +2308,7 @@ static int qxl_post_load(void *opaque, int version)
>      } else {
>          d->last_release = (QXLReleaseInfo *)(ram_start + d->last_release_offset);
>      }
> +    d->rom->mode = d->rom_mode;
>  
>      d->modes = (QXLModes*)((uint8_t*)d->rom + d->rom->modes_offset);
>  
> @@ -2361,7 +2354,7 @@ static int qxl_post_load(void *opaque, int version)
>      case QXL_MODE_COMPAT:
>          /* note: no need to call qxl_create_memslots, qxl_set_mode
>           * creates the mem slot. */
> -        qxl_set_mode(d, d->shadow_rom.mode, 1);
> +        qxl_set_mode(d, d->rom->mode, 1);
>          break;
>      }
>      return 0;
> @@ -2428,7 +2421,7 @@ static VMStateDescription qxl_vmstate = {
>      .fields = (VMStateField[]) {
>          VMSTATE_PCI_DEVICE(pci, PCIQXLDevice),
>          VMSTATE_STRUCT(vga, PCIQXLDevice, 0, vmstate_vga_common, VGACommonState),
> -        VMSTATE_UINT32(shadow_rom.mode, PCIQXLDevice),
> +        VMSTATE_UINT32(rom_mode, PCIQXLDevice),
>          VMSTATE_UINT32(num_free_res, PCIQXLDevice),
>          VMSTATE_UINT32(last_release_offset, PCIQXLDevice),
>          VMSTATE_UINT32(mode, PCIQXLDevice),
> 

Re: [PATCH 2/2] qxl: drop shadow_rom

[Gerd Hoffmann]

On Tue, Feb 25, 2020 at 08:39:12AM +0100, Paolo Bonzini wrote:
> On 25/02/20 06:59, Gerd Hoffmann wrote:
> > Now that the rom bar is mapped read-only and the guest can't change
> > things under our feet we don't need the shadow rom any more.
> 
> Can't it do so when migrating from an older version?

Good point, and I think there is no easy way around it.
So drop 2/2 and merge 1/2 only I guess?

cheers,
  Gerd

Re: [PATCH 2/2] qxl: drop shadow_rom

[Paolo Bonzini]

On 25/02/20 09:23, Gerd Hoffmann wrote:
> On Tue, Feb 25, 2020 at 08:39:12AM +0100, Paolo Bonzini wrote:
>> On 25/02/20 06:59, Gerd Hoffmann wrote:
>>> Now that the rom bar is mapped read-only and the guest can't change
>>> things under our feet we don't need the shadow rom any more.
>>
>> Can't it do so when migrating from an older version?
> 
> Good point, and I think there is no easy way around it.
> So drop 2/2 and merge 1/2 only I guess?

Yes. :(

Paolo

Re: [PATCH 1/2] qxl: map rom r/o

[Philippe Mathieu-Daudé]

On 2/25/20 6:59 AM, Gerd Hoffmann wrote:
> Map qxl rom read-only into the guest, so the guest can't tamper with the
> content.  qxl has a shadow copy of the rom to deal with that, but the
> shadow doesn't cover the mode list.  A privilidged user in the guest can
> manipulate the mode list and that to trick qemu into oob reads, leading
> to a DoS via segfault if that read access happens to hit unmapped memory.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/display/qxl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 21a43a1d5ec2..227da69a50d9 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2136,7 +2136,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
>       pci_set_byte(&config[PCI_INTERRUPT_PIN], 1);
>   
>       qxl->rom_size = qxl_rom_size();
> -    memory_region_init_ram(&qxl->rom_bar, OBJECT(qxl), "qxl.vrom",
> +    memory_region_init_rom(&qxl->rom_bar, OBJECT(qxl), "qxl.vrom",
>                              qxl->rom_size, &error_fatal);
>       init_qxl_rom(qxl);
>       init_qxl_ram(qxl);
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>