Re: [PATCH] vfio/pci: Fix ROM load failure handling in vfio_rom_read()

[Alex Williamson <alex@shazbot.org>]

On Fri, 24 Jul 2026 17:41:45 +0200
Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> wrote:

> On 24/7/26 11:24, Cédric Le Goater wrote:
> > When vfio_pci_load_rom() fails, vdev->rom is NULL but the memcpy
> > still computes a source pointer from it, which is undefined behavior.
> > Guard the access and return a 0xff pattern instead, which is what
> > hardware returns for an absent ROM.
> > 
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > ---
> >   hw/vfio/pci.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index a3147d28665abd29fd804bd08bdffc3c7440033c..16d4f70ea58c9e5b174dd57950df55dc2ab91f6f 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1176,8 +1176,13 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
> >           }
> >       }
> >   
> > -    memcpy(&val, vdev->rom + addr,
> > -           (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0);
> > +    /* If ROM loading failed, return 0xff pattern */
> > +    if (vdev->rom_read_failed) {
> > +        memset(&val, 0xff, sizeof(val));
> > +    } else {
> > +        memcpy(&val, vdev->rom + addr,
> > +               (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0);
> > +    }
> >   
> >       switch (size) {
> >       case 1:  
> 
> Maybe simpler:
> 
> -- >8 --  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c204706e630..d06a2d76523 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1151,13 +1151,18 @@ static uint64_t vfio_rom_read(void *opaque, 
> hwaddr addr, unsigned size)
>       } val = { .qword = ~0ULL };
>       uint64_t data = 0;
> 
> +    if (unlikely(vdev->rom_read_failed)) {
> +        return -1;
> +    }
> +
>       /* Load the ROM lazily when the guest tries to read it */
> -    if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
> +    if (unlikely(!vdev->rom)) {
>           Error *local_err = NULL;
> 
>           vdev->rom_read_failed = !vfio_pci_load_rom(vdev, &local_err);
>           if (vdev->rom_read_failed) {
>               error_report_err(local_err);
> +            return -1;
>           }
>       }
> ---

Loses tracing, introduces more return paths, uses implicit -1 handling
to fill return value... not simpler, imo.

> 
> But we can also rewrite into a more readable code using ld/st
> (also avoiding memcpy zero-sized):
> 
> -- >8 --  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c204706e630..af51c898f50 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -33,6 +33,7 @@
>   #include "migration/vmstate.h"
>   #include "migration/cpr.h"
>   #include "qobject/qdict.h"
> +#include "qemu/bswap.h"
>   #include "qemu/error-report.h"
>   #include "qemu/main-loop.h"
>   #include "qemu/module.h"
> @@ -1143,42 +1144,30 @@ static int 
> vfio_pci_config_space_write(VFIOPCIDevice *vdev, off_t offset,
>   static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
>   {
>       VFIOPCIDevice *vdev = opaque;
> -    union {
> -        uint8_t byte;
> -        uint16_t word;
> -        uint32_t dword;
> -        uint64_t qword;
> -    } val = { .qword = ~0ULL };
> -    uint64_t data = 0;
> +    uint64_t data = -1;
> +
> +    if (unlikely(vdev->rom_read_failed)) {
> +        goto out;
> +    }
> 
>       /* Load the ROM lazily when the guest tries to read it */
> -    if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
> +    if (unlikely(!vdev->rom)) {
>           Error *local_err = NULL;
> 
>           vdev->rom_read_failed = !vfio_pci_load_rom(vdev, &local_err);
>           if (vdev->rom_read_failed) {
>               error_report_err(local_err);
> +            goto out;
>           }
>       }
> 
> -    memcpy(&val, vdev->rom + addr,
> -           (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0);
> -
> -    switch (size) {
> -    case 1:
> -        data = val.byte;
> -        break;
> -    case 2:
> -        data = le16_to_cpu(val.word);
> -        break;
> -    case 4:
> -        data = le32_to_cpu(val.dword);
> -        break;
> -    default:
> -        hw_error("vfio: unsupported read size, %d bytes\n", size);
> -        break;
> +    if (addr < vdev->rom_size) {
> +        memcpy(&val, vdev->rom + addr, MIN(size, vdev->rom_size - addr));
>       }
> 
> +    data = ldn_le_p(&val, size);

val is undeclared.

> +
> +out:
>       trace_vfio_rom_read(vdev->vbasedev.name, addr, size, data);
> 
>       return data;
> ---

As described in my previous reply, there's not actually a bug to fix
here.  The code behaves as intended and, in this respect, always has.
I don't really see a need for the churn.  Thanks,

Alex