* [PATCH v3 0/2] pci: add romsize property @ 2021-02-03 13:18 Paolo Bonzini 2021-02-03 13:18 ` [PATCH v3 1/2] pci: reject too large ROMs Paolo Bonzini 2021-02-03 13:18 ` [PATCH v3 2/2] pci: add romsize property Paolo Bonzini 0 siblings, 2 replies; 6+ messages in thread From: Paolo Bonzini @ 2021-02-03 13:18 UTC (permalink / raw) To: qemu-devel; +Cc: mst, lersek, peterx, dme This property can be useful for distros to set up known-good ROM sizes for migration purposes. The VM will fail to start if the ROM is too large, and migration compatibility will not be broken if the ROM is too small. v1->v2: fix overflow issues in nearby code [Laszlo] v2->v3: consistently use %u in error messages [David] Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] pci: reject too large ROMs 2021-02-03 13:18 [PATCH v3 0/2] pci: add romsize property Paolo Bonzini @ 2021-02-03 13:18 ` Paolo Bonzini 2021-02-03 14:08 ` David Edmondson 2021-02-03 13:18 ` [PATCH v3 2/2] pci: add romsize property Paolo Bonzini 1 sibling, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2021-02-03 13:18 UTC (permalink / raw) To: qemu-devel; +Cc: Philippe Mathieu-Daudé, mst, lersek, peterx, dme get_image_size() returns an int64_t, which pci_add_option_rom() assigns to an "int" without any range checking. A 32-bit BAR could be up to 2 GiB in size, so reject anything above it. In order to accomodate a rounded-up size of 2 GiB, change pci_patch_ids's size argument to unsigned. Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/pci/pci.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 512e9042ff..58560c044d 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -25,6 +25,7 @@ #include "qemu/osdep.h" #include "qemu-common.h" #include "qemu/datadir.h" +#include "qemu/units.h" #include "hw/irq.h" #include "hw/pci/pci.h" #include "hw/pci/pci_bridge.h" @@ -2234,7 +2235,7 @@ static uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset) /* Patch the PCI vendor and device ids in a PCI rom image if necessary. This is needed for an option rom which is used for more than one device. */ -static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size) +static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) { uint16_t vendor_id; uint16_t device_id; @@ -2292,7 +2293,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size) static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, Error **errp) { - int size; + int64_t size; char *path; void *ptr; char name[32]; @@ -2342,6 +2343,11 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, error_setg(errp, "romfile \"%s\" is empty", pdev->romfile); g_free(path); return; + } else if (size > 2 * GiB) { + error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)", + pdev->romfile); + g_free(path); + return; } size = pow2ceil(size); -- 2.29.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] pci: reject too large ROMs 2021-02-03 13:18 ` [PATCH v3 1/2] pci: reject too large ROMs Paolo Bonzini @ 2021-02-03 14:08 ` David Edmondson 0 siblings, 0 replies; 6+ messages in thread From: David Edmondson @ 2021-02-03 14:08 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel Cc: lersek, Philippe Mathieu-Daudé, peterx, mst On Wednesday, 2021-02-03 at 14:18:27 +01, Paolo Bonzini wrote: > get_image_size() returns an int64_t, which pci_add_option_rom() assigns > to an "int" without any range checking. A 32-bit BAR could be up to > 2 GiB in size, so reject anything above it. In order to accomodate > a rounded-up size of 2 GiB, change pci_patch_ids's size argument > to unsigned. > > Reviewed-by: Peter Xu <peterx@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: David Edmondson <david.edmondson@oracle.com> > --- > hw/pci/pci.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 512e9042ff..58560c044d 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -25,6 +25,7 @@ > #include "qemu/osdep.h" > #include "qemu-common.h" > #include "qemu/datadir.h" > +#include "qemu/units.h" > #include "hw/irq.h" > #include "hw/pci/pci.h" > #include "hw/pci/pci_bridge.h" > @@ -2234,7 +2235,7 @@ static uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset) > > /* Patch the PCI vendor and device ids in a PCI rom image if necessary. > This is needed for an option rom which is used for more than one device. */ > -static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size) > +static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) > { > uint16_t vendor_id; > uint16_t device_id; > @@ -2292,7 +2293,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size) > static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, > Error **errp) > { > - int size; > + int64_t size; > char *path; > void *ptr; > char name[32]; > @@ -2342,6 +2343,11 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, > error_setg(errp, "romfile \"%s\" is empty", pdev->romfile); > g_free(path); > return; > + } else if (size > 2 * GiB) { > + error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)", > + pdev->romfile); > + g_free(path); > + return; > } > size = pow2ceil(size); > > -- > 2.29.2 dme. -- I used to worry, thought I was goin' mad in a hurry. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] pci: add romsize property 2021-02-03 13:18 [PATCH v3 0/2] pci: add romsize property Paolo Bonzini 2021-02-03 13:18 ` [PATCH v3 1/2] pci: reject too large ROMs Paolo Bonzini @ 2021-02-03 13:18 ` Paolo Bonzini 2021-02-03 14:08 ` David Edmondson 2021-02-03 19:14 ` Laszlo Ersek 1 sibling, 2 replies; 6+ messages in thread From: Paolo Bonzini @ 2021-02-03 13:18 UTC (permalink / raw) To: qemu-devel; +Cc: mst, lersek, Dr . David Alan Gilbert, peterx, dme This property can be useful for distros to set up known-good ROM sizes for migration purposes. The VM will fail to start if the ROM is too large, and migration compatibility will not be broken if the ROM is too small. Note that even though romsize is a uint32_t, it has to be between 1 (because empty ROM files are not accepted, and romsize must be greater than the file) and 2^31 (because values above are not powers of two and are rejected). Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> Message-Id: <20201218182736.1634344-1-pbonzini@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/pci/pci.c | 19 +++++++++++++++++-- hw/xen/xen_pt_load_rom.c | 14 ++++++++++++-- include/hw/pci/pci.h | 1 + 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 58560c044d..a9ebef8a35 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -69,6 +69,7 @@ static void pcibus_reset(BusState *qbus); static Property pci_props[] = { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), DEFINE_PROP_STRING("romfile", PCIDevice, romfile), + DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1), DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1), DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), @@ -2084,6 +2085,11 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) bool is_default_rom; uint16_t class_id; + if (pci_dev->romsize != -1 && !is_power_of_2(pci_dev->romsize)) { + error_setg(errp, "ROM size %u is not a power of two", pci_dev->romsize); + return; + } + /* initialize cap_present for pci_is_express() and pci_config_size(), * Note that hybrid PCIs are not set automatically and need to manage * QEMU_PCI_CAP_EXPRESS manually */ @@ -2349,7 +2355,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, g_free(path); return; } - size = pow2ceil(size); + if (pdev->romsize != -1) { + if (size > pdev->romsize) { + error_setg(errp, "romfile \"%s\" (%u bytes) is too large for ROM size %u", + pdev->romfile, (uint32_t)size, pdev->romsize); + g_free(path); + return; + } + } else { + pdev->romsize = pow2ceil(size); + } vmsd = qdev_get_vmsd(DEVICE(pdev)); @@ -2359,7 +2374,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, snprintf(name, sizeof(name), "%s.rom", object_get_typename(OBJECT(pdev))); } pdev->has_rom = true; - memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, size, &error_fatal); + memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize, &error_fatal); ptr = memory_region_get_ram_ptr(&pdev->rom); if (load_image_size(path, ptr, size) < 0) { error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile); diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c index a50a80837e..03422a8a71 100644 --- a/hw/xen/xen_pt_load_rom.c +++ b/hw/xen/xen_pt_load_rom.c @@ -53,10 +53,20 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev, } fseek(fp, 0, SEEK_SET); + if (dev->romsize != -1) { + if (st.st_size > dev->romsize) { + error_report("ROM BAR \"%s\" (%ld bytes) is too large for ROM size %u", + rom_file, (long) st.st_size, dev->romsize); + goto close_rom; + } + } else { + dev->romsize = st.st_size; + } + snprintf(name, sizeof(name), "%s.rom", object_get_typename(owner)); - memory_region_init_ram(&dev->rom, owner, name, st.st_size, &error_abort); + memory_region_init_ram(&dev->rom, owner, name, dev->romsize, &error_abort); ptr = memory_region_get_ram_ptr(&dev->rom); - memset(ptr, 0xff, st.st_size); + memset(ptr, 0xff, dev->romsize); if (!fread(ptr, 1, st.st_size, fp)) { error_report("pci-assign: Cannot read from host %s", rom_file); diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 66db08462f..1bc231480f 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -344,6 +344,7 @@ struct PCIDevice { /* Location of option rom */ char *romfile; + uint32_t romsize; bool has_rom; MemoryRegion rom; uint32_t rom_bar; -- 2.29.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] pci: add romsize property 2021-02-03 13:18 ` [PATCH v3 2/2] pci: add romsize property Paolo Bonzini @ 2021-02-03 14:08 ` David Edmondson 2021-02-03 19:14 ` Laszlo Ersek 1 sibling, 0 replies; 6+ messages in thread From: David Edmondson @ 2021-02-03 14:08 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel; +Cc: lersek, Dr . David Alan Gilbert, peterx, mst On Wednesday, 2021-02-03 at 14:18:28 +01, Paolo Bonzini wrote: > This property can be useful for distros to set up known-good ROM sizes for > migration purposes. The VM will fail to start if the ROM is too large, > and migration compatibility will not be broken if the ROM is too small. > > Note that even though romsize is a uint32_t, it has to be between 1 > (because empty ROM files are not accepted, and romsize must be greater > than the file) and 2^31 (because values above are not powers of two and > are rejected). > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Peter Xu <peterx@redhat.com> > Message-Id: <20201218182736.1634344-1-pbonzini@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: David Edmondson <david.edmondson@oracle.com> > --- > hw/pci/pci.c | 19 +++++++++++++++++-- > hw/xen/xen_pt_load_rom.c | 14 ++++++++++++-- > include/hw/pci/pci.h | 1 + > 3 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 58560c044d..a9ebef8a35 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -69,6 +69,7 @@ static void pcibus_reset(BusState *qbus); > static Property pci_props[] = { > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > + DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1), > DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1), > DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, > QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), > @@ -2084,6 +2085,11 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) > bool is_default_rom; > uint16_t class_id; > > + if (pci_dev->romsize != -1 && !is_power_of_2(pci_dev->romsize)) { > + error_setg(errp, "ROM size %u is not a power of two", pci_dev->romsize); > + return; > + } > + > /* initialize cap_present for pci_is_express() and pci_config_size(), > * Note that hybrid PCIs are not set automatically and need to manage > * QEMU_PCI_CAP_EXPRESS manually */ > @@ -2349,7 +2355,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, > g_free(path); > return; > } > - size = pow2ceil(size); > + if (pdev->romsize != -1) { > + if (size > pdev->romsize) { > + error_setg(errp, "romfile \"%s\" (%u bytes) is too large for ROM size %u", > + pdev->romfile, (uint32_t)size, pdev->romsize); > + g_free(path); > + return; > + } > + } else { > + pdev->romsize = pow2ceil(size); > + } > > vmsd = qdev_get_vmsd(DEVICE(pdev)); > > @@ -2359,7 +2374,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, > snprintf(name, sizeof(name), "%s.rom", object_get_typename(OBJECT(pdev))); > } > pdev->has_rom = true; > - memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, size, &error_fatal); > + memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize, &error_fatal); > ptr = memory_region_get_ram_ptr(&pdev->rom); > if (load_image_size(path, ptr, size) < 0) { > error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile); > diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c > index a50a80837e..03422a8a71 100644 > --- a/hw/xen/xen_pt_load_rom.c > +++ b/hw/xen/xen_pt_load_rom.c > @@ -53,10 +53,20 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev, > } > fseek(fp, 0, SEEK_SET); > > + if (dev->romsize != -1) { > + if (st.st_size > dev->romsize) { > + error_report("ROM BAR \"%s\" (%ld bytes) is too large for ROM size %u", > + rom_file, (long) st.st_size, dev->romsize); > + goto close_rom; > + } > + } else { > + dev->romsize = st.st_size; > + } > + > snprintf(name, sizeof(name), "%s.rom", object_get_typename(owner)); > - memory_region_init_ram(&dev->rom, owner, name, st.st_size, &error_abort); > + memory_region_init_ram(&dev->rom, owner, name, dev->romsize, &error_abort); > ptr = memory_region_get_ram_ptr(&dev->rom); > - memset(ptr, 0xff, st.st_size); > + memset(ptr, 0xff, dev->romsize); > > if (!fread(ptr, 1, st.st_size, fp)) { > error_report("pci-assign: Cannot read from host %s", rom_file); > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index 66db08462f..1bc231480f 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -344,6 +344,7 @@ struct PCIDevice { > > /* Location of option rom */ > char *romfile; > + uint32_t romsize; > bool has_rom; > MemoryRegion rom; > uint32_t rom_bar; > -- > 2.29.2 dme. -- I'm not the reason you're looking for redemption. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] pci: add romsize property 2021-02-03 13:18 ` [PATCH v3 2/2] pci: add romsize property Paolo Bonzini 2021-02-03 14:08 ` David Edmondson @ 2021-02-03 19:14 ` Laszlo Ersek 1 sibling, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2021-02-03 19:14 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel; +Cc: mst, Dr . David Alan Gilbert, peterx, dme On 02/03/21 14:18, Paolo Bonzini wrote: > This property can be useful for distros to set up known-good ROM sizes for > migration purposes. The VM will fail to start if the ROM is too large, > and migration compatibility will not be broken if the ROM is too small. > > Note that even though romsize is a uint32_t, it has to be between 1 > (because empty ROM files are not accepted, and romsize must be greater > than the file) and 2^31 (because values above are not powers of two and > are rejected). > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Peter Xu <peterx@redhat.com> > Message-Id: <20201218182736.1634344-1-pbonzini@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/pci/pci.c | 19 +++++++++++++++++-- > hw/xen/xen_pt_load_rom.c | 14 ++++++++++++-- > include/hw/pci/pci.h | 1 + > 3 files changed, 30 insertions(+), 4 deletions(-) Acked-by: Laszlo Ersek <lersek@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-03 19:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-02-03 13:18 [PATCH v3 0/2] pci: add romsize property Paolo Bonzini 2021-02-03 13:18 ` [PATCH v3 1/2] pci: reject too large ROMs Paolo Bonzini 2021-02-03 14:08 ` David Edmondson 2021-02-03 13:18 ` [PATCH v3 2/2] pci: add romsize property Paolo Bonzini 2021-02-03 14:08 ` David Edmondson 2021-02-03 19:14 ` Laszlo Ersek
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).