* [PATCH 0/2] qxl: map rom r/o, remove shadow. @ 2020-02-25 5:59 Gerd Hoffmann 2020-02-25 5:59 ` [PATCH 1/2] qxl: map rom r/o Gerd Hoffmann 2020-02-25 5:59 ` [PATCH 2/2] qxl: drop shadow_rom Gerd Hoffmann 0 siblings, 2 replies; 7+ messages in thread From: Gerd Hoffmann @ 2020-02-25 5:59 UTC (permalink / raw) To: qemu-devel Cc: sstabellini, pmatouse, Michael S. Tsirkin, mdroth, ppandit, Gerd Hoffmann, Paolo Bonzini 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] qxl: map rom r/o 2020-02-25 5:59 [PATCH 0/2] qxl: map rom r/o, remove shadow Gerd Hoffmann @ 2020-02-25 5:59 ` Gerd Hoffmann 2020-02-25 9:07 ` Philippe Mathieu-Daudé 2020-02-25 5:59 ` [PATCH 2/2] qxl: drop shadow_rom Gerd Hoffmann 1 sibling, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2020-02-25 5:59 UTC (permalink / raw) To: qemu-devel Cc: sstabellini, pmatouse, Michael S. Tsirkin, mdroth, ppandit, Gerd Hoffmann, Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] qxl: map rom r/o 2020-02-25 5:59 ` [PATCH 1/2] qxl: map rom r/o Gerd Hoffmann @ 2020-02-25 9:07 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-25 9:07 UTC (permalink / raw) To: Gerd Hoffmann, qemu-devel Cc: sstabellini, pmatouse, Michael S. Tsirkin, mdroth, ppandit, Paolo Bonzini 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] qxl: drop shadow_rom 2020-02-25 5:59 [PATCH 0/2] qxl: map rom r/o, remove shadow Gerd Hoffmann 2020-02-25 5:59 ` [PATCH 1/2] qxl: map rom r/o Gerd Hoffmann @ 2020-02-25 5:59 ` Gerd Hoffmann 2020-02-25 7:39 ` Paolo Bonzini 1 sibling, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2020-02-25 5:59 UTC (permalink / raw) To: qemu-devel Cc: sstabellini, pmatouse, Michael S. Tsirkin, mdroth, ppandit, Gerd Hoffmann, Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] qxl: drop shadow_rom 2020-02-25 5:59 ` [PATCH 2/2] qxl: drop shadow_rom Gerd Hoffmann @ 2020-02-25 7:39 ` Paolo Bonzini 2020-02-25 8:23 ` Gerd Hoffmann 0 siblings, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2020-02-25 7:39 UTC (permalink / raw) To: Gerd Hoffmann, qemu-devel Cc: Michael S. Tsirkin, sstabellini, pmatouse, mdroth, ppandit 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), > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] qxl: drop shadow_rom 2020-02-25 7:39 ` Paolo Bonzini @ 2020-02-25 8:23 ` Gerd Hoffmann 2020-02-25 8:29 ` Paolo Bonzini 0 siblings, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2020-02-25 8:23 UTC (permalink / raw) To: Paolo Bonzini Cc: sstabellini, pmatouse, Michael S. Tsirkin, mdroth, qemu-devel, ppandit 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] qxl: drop shadow_rom 2020-02-25 8:23 ` Gerd Hoffmann @ 2020-02-25 8:29 ` Paolo Bonzini 0 siblings, 0 replies; 7+ messages in thread From: Paolo Bonzini @ 2020-02-25 8:29 UTC (permalink / raw) To: Gerd Hoffmann Cc: sstabellini, pmatouse, Michael S. Tsirkin, mdroth, qemu-devel, ppandit 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-25 9:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-25 5:59 [PATCH 0/2] qxl: map rom r/o, remove shadow Gerd Hoffmann 2020-02-25 5:59 ` [PATCH 1/2] qxl: map rom r/o Gerd Hoffmann 2020-02-25 9:07 ` Philippe Mathieu-Daudé 2020-02-25 5:59 ` [PATCH 2/2] qxl: drop shadow_rom Gerd Hoffmann 2020-02-25 7:39 ` Paolo Bonzini 2020-02-25 8:23 ` Gerd Hoffmann 2020-02-25 8:29 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).