[PATCH v2 0/2] pci: add romsize property

[PATCH v2 0/2] pci: add romsize property

[Paolo Bonzini]

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.

The main difference from v1 is the first patch, which fixes overflow
issues in nearby code.  The second patch is the same as v1 except for
replacing %d->%u in the error message.

Paolo

Paolo Bonzini (2):
  pci: reject too large ROMs
  pci: add romsize property

 hw/pci/pci.c             | 28 ++++++++++++++++++++++++----
 hw/xen/xen_pt_load_rom.c | 14 ++++++++++++--
 include/hw/pci/pci.h     |  1 +
 3 files changed, 37 insertions(+), 6 deletions(-)

-- 
2.29.2

[PATCH v2 1/2] pci: reject too large ROMs

[Paolo Bonzini]

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.

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a6b0c5602e..bbce10050b 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"
@@ -2258,7 +2258,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;
@@ -2316,7 +2316,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];
@@ -2366,6 +2366,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

[PATCH v2 2/2] pci: add romsize property

[Paolo Bonzini]

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).

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@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 bbce10050b..5b3fe3c294 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -68,6 +68,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),
@@ -2107,6 +2108,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 %d 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 */
@@ -2372,7 +2378,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));
 
@@ -2382,7 +2397,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..153812f8cd 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 %d",
+                         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 259f9c992d..b028245b62 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -343,6 +343,7 @@ struct PCIDevice {
 
     /* Location of option rom */
     char *romfile;
+    uint32_t romsize;
     bool has_rom;
     MemoryRegion rom;
     uint32_t rom_bar;
-- 
2.29.2

Re: [PATCH v2 2/2] pci: add romsize property

[BALATON Zoltan]

On Fri, 29 Jan 2021, 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).

I've found I have to use this command to disable vgabios-ati.bin:

qemu-system-ppc -M sam460ex -device ati-vga,romfile=""

otherwise the BIOS emulator in the guest firmware crashes and this works 
so I think romfile can be empty and it's a useful feature to have in this 
case for example. I don't know if this patch changes anything about that 
but the commit message saying that romfile cannot be empty may be wrong.

Regards,
BALATON Zoltan

> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Peter Xu <peterx@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 bbce10050b..5b3fe3c294 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -68,6 +68,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),
> @@ -2107,6 +2108,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 %d 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 */
> @@ -2372,7 +2378,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));
>
> @@ -2382,7 +2397,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..153812f8cd 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 %d",
> +                         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 259f9c992d..b028245b62 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -343,6 +343,7 @@ struct PCIDevice {
>
>     /* Location of option rom */
>     char *romfile;
> +    uint32_t romsize;
>     bool has_rom;
>     MemoryRegion rom;
>     uint32_t rom_bar;
>

Re: [PATCH v2 2/2] pci: add romsize property

[Paolo Bonzini]

On 29/01/21 20:51, BALATON Zoltan wrote:
> otherwise the BIOS emulator in the guest firmware crashes and this works 
> so I think romfile can be empty and it's a useful feature to have in 
> this case for example. I don't know if this patch changes anything about 
> that but the commit message saying that romfile cannot be empty may be 
> wrong.

The empty property value configures the device not to have a ROM file at 
all.  The commit message says that ROM files (if they exist) cannot be 
empty, corresponding to this code in pci_add_option_rom:

     } else if (size == 0) {
         error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
         g_free(path);
         return;
     }

Paolo

Re: [PATCH v2 2/2] pci: add romsize property

[BALATON Zoltan]

On Fri, 29 Jan 2021, Paolo Bonzini wrote:
> On 29/01/21 20:51, BALATON Zoltan wrote:
>> otherwise the BIOS emulator in the guest firmware crashes and this works so 
>> I think romfile can be empty and it's a useful feature to have in this case 
>> for example. I don't know if this patch changes anything about that but the 
>> commit message saying that romfile cannot be empty may be wrong.
>
> The empty property value configures the device not to have a ROM file at all. 
> The commit message says that ROM files (if they exist) cannot be empty, 
> corresponding to this code in pci_add_option_rom:
>
>    } else if (size == 0) {
>        error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
>        g_free(path);
>        return;
>    }

OK, then it was just not clear to me that the commit message talks about 
the romfile itself and not the property.

By the way, does it make sense to compare uint32_t value to -1 and could 
that provoke some compiler/sanitiser warnings? Is it better to have a 
signed type or use UINT32_MAX or simlar instead?

Regards,
BALATON Zoltan

Re: [PATCH v2 2/2] pci: add romsize property

[Paolo Bonzini]

On 29/01/21 21:06, BALATON Zoltan wrote:
>> The empty property value configures the device not to have a ROM file 
>> at all. The commit message says that ROM files (if they exist) cannot 
>> be empty, corresponding to this code in pci_add_option_rom:
>>
>>    } else if (size == 0) {
>>        error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
>>        g_free(path);
>>        return;
>>    }
> 
> OK, then it was just not clear to me that the commit message talks about 
> the romfile itself and not the property.
> 
> By the way, does it make sense to compare uint32_t value to -1 and could 
> that provoke some compiler/sanitiser warnings? Is it better to have a 
> signed type or use UINT32_MAX or simlar instead?

There is probably some warning for it but I think not even -Wextra 
enables it by default.

Paolo

Re: [PATCH v2 1/2] pci: reject too large ROMs

[Peter Xu]

On Fri, Jan 29, 2021 at 08:28:37PM +0100, 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 2/2] pci: add romsize property

[Gerd Hoffmann]

  Hi,

> +    DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1),

IIRC we have a DEFINE_PROP_SIZE() which can parse units and therefore
accepts -- for example -- "512k" or "1M".

take care,
  Gerd

Re: [PATCH v2 1/2] pci: reject too large ROMs

[Philippe Mathieu-Daudé]

On 1/29/21 8:28 PM, 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/pci/pci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

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

Re: [PATCH v2 2/2] pci: add romsize property

[Peter Xu]

On Mon, Feb 01, 2021 at 08:56:32AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > +    DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1),
> 
> IIRC we have a DEFINE_PROP_SIZE() which can parse units and therefore
> accepts -- for example -- "512k" or "1M".

Actually IMHO there's some fair point to make it uint32: even 1 byte would
matter here or migration fails.  Hence, we don't need to worry about things
like 512KB or 512KiB, for example.

Not to mention that I bet 99.99% qemu users won't really use this parameter,
only if we'd migrate across distros.  That's rare, we'd copy the exact byte
value of the source ROM size here (e.g. via "info ramblock", or "ls -l" the
romfile then round to pow2 and specify on dest) or we simply copy this param
over from another source VM.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 2/2] pci: add romsize property

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Mon, Feb 01, 2021 at 08:56:32AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > +    DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1),
> > 
> > IIRC we have a DEFINE_PROP_SIZE() which can parse units and therefore
> > accepts -- for example -- "512k" or "1M".
> 
> Actually IMHO there's some fair point to make it uint32: even 1 byte would
> matter here or migration fails.  Hence, we don't need to worry about things
> like 512KB or 512KiB, for example.
> 
> Not to mention that I bet 99.99% qemu users won't really use this parameter,
> only if we'd migrate across distros.  That's rare, we'd copy the exact byte
> value of the source ROM size here (e.g. via "info ramblock", or "ls -l" the
> romfile then round to pow2 and specify on dest) or we simply copy this param
> over from another source VM.

Well, we should sometime, set it for the default upstream machine types,
so that distros get a better chance of getting their rom images right
and consistent.

Dave

> Thanks,
> 
> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 2/2] pci: add romsize property

[David Edmondson]

On Friday, 2021-01-29 at 20:28:38 +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).
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Peter Xu <peterx@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 bbce10050b..5b3fe3c294 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -68,6 +68,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),
> @@ -2107,6 +2108,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 %d is not a power of two", pci_dev->romsize);
> +        return;

It would be nice to be consistent with the format type when reporting
errors - it's %d here...

> +    }
> +
>      /* 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 */
> @@ -2372,7 +2378,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);

%u here...

> +            g_free(path);
> +            return;
> +        }
> +    } else {
> +        pdev->romsize = pow2ceil(size);
> +    }
>  
>      vmsd = qdev_get_vmsd(DEVICE(pdev));
>  
> @@ -2382,7 +2397,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..153812f8cd 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 %d",

%d here...

> +                         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 259f9c992d..b028245b62 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -343,6 +343,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.
-- 
Tell her I'll be waiting, in the usual place.

Re: [PATCH v2 1/2] pci: reject too large ROMs

[Laszlo Ersek]

On 01/29/21 20:28, 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/pci/pci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a6b0c5602e..bbce10050b 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"
> @@ -2258,7 +2258,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;
> @@ -2316,7 +2316,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];
> @@ -2366,6 +2366,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);
>  
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH v2 2/2] pci: add romsize property

[Laszlo Ersek]

On 02/02/21 11:05, David Edmondson wrote:
> On Friday, 2021-01-29 at 20:28:38 +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).
>>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Reviewed-by: Peter Xu <peterx@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 bbce10050b..5b3fe3c294 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -68,6 +68,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),
>> @@ -2107,6 +2108,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 %d is not a power of two", pci_dev->romsize);
>> +        return;
> 
> It would be nice to be consistent with the format type when reporting
> errors - it's %d here...
> 
>> +    }
>> +
>>      /* 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 */
>> @@ -2372,7 +2378,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);
> 
> %u here...
> 
>> +            g_free(path);
>> +            return;
>> +        }
>> +    } else {
>> +        pdev->romsize = pow2ceil(size);
>> +    }
>>  
>>      vmsd = qdev_get_vmsd(DEVICE(pdev));
>>  
>> @@ -2382,7 +2397,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..153812f8cd 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 %d",
> 
> %d here...
> 
>> +                         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 259f9c992d..b028245b62 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -343,6 +343,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 believe this may be worth a v3; for that:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo