[PATCH 0/2] vapic: confine the VAPIC region to 0xc0000..0xe0000

[PATCH 0/2] vapic: confine the VAPIC region to 0xc0000..0xe0000

[Paolo Bonzini]

The VAPIC region is mapped as writable RAM, at very high priority,
above existing memory.  If the guest is allowed to map it everywhere,
it can overlap PCI BARs or even SMRAM.  Ensure that the whole
region first in the 128K of low memory that are reserved to
option ROMs.

Patch 1 is a separate bugfix that I noticed while working on
this area, while patch 2 is the actual fix.

Paolo Bonzini (2):
  i386/vapic: unref MemoryRegion if vapic_map_rom_writable fails
  vapic: confine the VAPIC region to 0xc0000..0xe0000

 hw/i386/vapic.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

-- 
2.55.0

[PATCH 1/2] i386/vapic: unref MemoryRegion if vapic_map_rom_writable fails

[Paolo Bonzini]

memory_region_find returns the memory region with an elevated
reference count.  Drop the reference count also if the memory
region cannot be mapped writable.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/vapic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/vapic.c b/hw/i386/vapic.c
index 8dd9188d96f..78dc2312bca 100644
--- a/hw/i386/vapic.c
+++ b/hw/i386/vapic.c
@@ -602,11 +602,13 @@ static int vapic_map_rom_writable(VAPICROMState *s)
 
     /* read ROM size from RAM region */
     if (rom_paddr + 2 >= memory_region_size(section.mr)) {
+        memory_region_unref(section.mr);
         return -1;
     }
     ram = memory_region_get_ram_ptr(section.mr);
     rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;
     if (rom_size == 0) {
+        memory_region_unref(section.mr);
         return -1;
     }
     s->rom_size = rom_size;
-- 
2.55.0

Re: [PATCH 1/2] i386/vapic: unref MemoryRegion if vapic_map_rom_writable fails

[Philippe Mathieu-Daudé]

On 26/8/26 20:15, Paolo Bonzini wrote:
> memory_region_find returns the memory region with an elevated
> reference count.  Drop the reference count also if the memory
> region cannot be mapped writable.

Maybe another one in kvm_get_xen_state().

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

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/i386/vapic.c | 2 ++
>   1 file changed, 2 insertions(+)

[PATCH 2/2] vapic: confine the VAPIC region to 0xc0000..0xe0000

[Paolo Bonzini]

The VAPIC region is mapped as writable RAM, at very high priority,
above existing memory.  If the guest is allowed to map it everywhere,
it can overlap PCI BARs or even SMRAM.  Ensure that the whole
region first in the 128K of low memory that are reserved to
option ROMs.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4206
Reported-by: Artem Dinaburg <https://gitlab.com/artem35>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/vapic.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/i386/vapic.c b/hw/i386/vapic.c
index 78dc2312bca..5c3911cf9c8 100644
--- a/hw/i386/vapic.c
+++ b/hw/i386/vapic.c
@@ -34,6 +34,10 @@
 #define ROM_BLOCK_SIZE          512
 #define ROM_BLOCK_MASK          (~(ROM_BLOCK_SIZE - 1))
 
+/* Option ROM window on PC/Q35 machines; the vapic ROM must live in here. */
+#define OPTION_ROM_START        0xc0000
+#define OPTION_ROM_END          0xe0000
+
 typedef enum VAPICMode {
     VAPIC_INACTIVE = 0,
     VAPIC_ACTIVE   = 1,
@@ -592,6 +596,14 @@ static int vapic_map_rom_writable(VAPICROMState *s)
     size_t rom_size;
     uint8_t *ram;
 
+    /*
+     * The VAPIC region should be mapped in place, refuse mapping it
+     * outside of the option ROM window.
+     */
+    if (rom_paddr < OPTION_ROM_START || rom_paddr >= OPTION_ROM_END) {
+        return -1;
+    }
+
     if (s->rom_mapped_writable) {
         memory_region_del_subregion(mr, &s->rom);
         object_unparent(OBJECT(&s->rom));
@@ -607,10 +619,11 @@ static int vapic_map_rom_writable(VAPICROMState *s)
     }
     ram = memory_region_get_ram_ptr(section.mr);
     rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;
-    if (rom_size == 0) {
+    if (rom_size == 0 || rom_size > OPTION_ROM_END - rom_paddr) {
         memory_region_unref(section.mr);
         return -1;
     }
+
     s->rom_size = rom_size;
 
     /* We need to round to avoid creating subpages
@@ -618,6 +631,7 @@ static int vapic_map_rom_writable(VAPICROMState *s)
     rom_size += rom_paddr & ~TARGET_PAGE_MASK;
     rom_paddr &= TARGET_PAGE_MASK;
     rom_size = TARGET_PAGE_ALIGN(rom_size);
+    assert(rom_paddr >= OPTION_ROM_START && rom_paddr + rom_size <= OPTION_ROM_END);
 
     memory_region_init_alias(&s->rom, OBJECT(s), "kvmvapic-rom", section.mr,
                              rom_paddr, rom_size);
-- 
2.55.0

Re: [PATCH 2/2] vapic: confine the VAPIC region to 0xc0000..0xe0000

[Philippe Mathieu-Daudé]

On 26/8/26 20:15, Paolo Bonzini wrote:
> The VAPIC region is mapped as writable RAM, at very high priority,
> above existing memory.  If the guest is allowed to map it everywhere,
> it can overlap PCI BARs or even SMRAM.  Ensure that the whole
> region first in the 128K of low memory that are reserved to
> option ROMs.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4206
> Reported-by: Artem Dinaburg <https://gitlab.com/artem35>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/i386/vapic.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)

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