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

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

[Cédric Le Goater]

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:
-- 
2.55.0

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

[Alex Williamson]

On Fri, 24 Jul 2026 11:24:13 +0200
Cédric Le Goater <clg@redhat.com> 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:

I thought we guaranteed that when (vdev->rom == NULL) that
(vdev->rom_size == 0), thus we end up with a harmless zero-sized memcpy
here.  Also val is pre-initialized to ~0 so the memset() is redundant.

Does this actually change any behavior or is it only meant to sanitize
the undefined (NULL + addr) pointer that's never dereferenced?  If the
latter, I'd drop the memset and note it explicitly as a sanitization,
not a fix.  Thanks,

Alex

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

[Cédric Le Goater]

On 7/24/26 16:17, Alex Williamson wrote:
> On Fri, 24 Jul 2026 11:24:13 +0200
> Cédric Le Goater <clg@redhat.com> 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:
> 
> I thought we guaranteed that when (vdev->rom == NULL) that
> (vdev->rom_size == 0), thus we end up with a harmless zero-sized memcpy
> here.  Also val is pre-initialized to ~0 so the memset() is redundant.
> 
> Does this actually change any behavior or is it only meant to sanitize
> the undefined (NULL + addr) pointer that's never dereferenced?  If the
> latter, I'd drop the memset and note it explicitly as a sanitization,
> not a fix.  Thanks,
Right. the memcpy is a no-op because the size is 0 and val stays ~0.
I had that patch in my tree for a while, but Mario's recent fixes made
it useless. Let's drop it.

Thanks,

C.

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

[Philippe Mathieu-Daudé]

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;
          }
      }
---

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);
+
+out:
      trace_vfio_rom_read(vdev->vbasedev.name, addr, size, data);

      return data;
---

(untested).

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

[Alex Williamson]

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