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 <[email protected]>
---
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).
