[PATCH] hw/i386/pc: Don't try to init PCI NICs if there is no PCI bus

[PATCH] hw/i386/pc: Don't try to init PCI NICs if there is no PCI bus

[Peter Maydell]

The 'isapc' machine type has no PCI bus, but pc_nic_init() still
calls pci_init_nic_devices() passing it a NULL bus pointer.  This
causes the clang sanitizer to complain:

$ ./build/clang/qemu-system-i386 -M isapc
../../hw/pci/pci.c:1866:39: runtime error: member access within null pointer of type 'PCIBus' (aka 'struct PCIBus')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/pci/pci.c:1866:39 in

This is because pci_init_nic_devices() does
 &bus->qbus
which is undefined behaviour on a NULL pointer even though we're not
actually dereferencing the pointer. (We don't actually crash as
a result, so if you aren't running a sanitizer build then there
are no user-visible effects.)

Make pc_nic_init() avoid trying to initialize PCI NICs on a non-PCI
system.

Cc: qemu-stable@nongnu.org
Fixes: 8d39f9ba14d64 ("hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This shows up if you run "make check" on a ubsan build.
---
 hw/i386/pc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2047633e4cf..1af1a1a1823 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1251,7 +1251,9 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
     }
 
     /* Anything remaining should be a PCI NIC */
-    pci_init_nic_devices(pci_bus, mc->default_nic);
+    if (pci_bus) {
+        pci_init_nic_devices(pci_bus, mc->default_nic);
+    }
 
     rom_reset_order_override();
 }
-- 
2.34.1

Re: [PATCH] hw/i386/pc: Don't try to init PCI NICs if there is no PCI bus

[Peter Maydell]

On Tue, 5 Nov 2024 at 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The 'isapc' machine type has no PCI bus, but pc_nic_init() still
> calls pci_init_nic_devices() passing it a NULL bus pointer.  This
> causes the clang sanitizer to complain:
>
> $ ./build/clang/qemu-system-i386 -M isapc
> ../../hw/pci/pci.c:1866:39: runtime error: member access within null pointer of type 'PCIBus' (aka 'struct PCIBus')
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/pci/pci.c:1866:39 in
>
> This is because pci_init_nic_devices() does
>  &bus->qbus
> which is undefined behaviour on a NULL pointer even though we're not
> actually dereferencing the pointer. (We don't actually crash as
> a result, so if you aren't running a sanitizer build then there
> are no user-visible effects.)
>
> Make pc_nic_init() avoid trying to initialize PCI NICs on a non-PCI
> system.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 8d39f9ba14d64 ("hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This shows up if you run "make check" on a ubsan build.

Incidentally, if pci_init_nic_devices() had done the
more standard way to do "get a BusState* from a PCIBus*",
i.e. use the QOM cast macro "BUS(bus)", that would also
have avoided the UB (because QOM cast macros on NULL
are valid and return NULL). But I figured not passing NULL
in the first place was probably the intention rather
than quietly handling NULL.

thanks
-- PMM

Re: [PATCH] hw/i386/pc: Don't try to init PCI NICs if there is no PCI bus

[Bernhard Beschow]

Am 5. November 2024 17:18:13 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>The 'isapc' machine type has no PCI bus, but pc_nic_init() still
>calls pci_init_nic_devices() passing it a NULL bus pointer.  This
>causes the clang sanitizer to complain:
>
>$ ./build/clang/qemu-system-i386 -M isapc
>../../hw/pci/pci.c:1866:39: runtime error: member access within null pointer of type 'PCIBus' (aka 'struct PCIBus')
>SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/pci/pci.c:1866:39 in
>
>This is because pci_init_nic_devices() does
> &bus->qbus
>which is undefined behaviour on a NULL pointer even though we're not
>actually dereferencing the pointer. (We don't actually crash as
>a result, so if you aren't running a sanitizer build then there
>are no user-visible effects.)
>
>Make pc_nic_init() avoid trying to initialize PCI NICs on a non-PCI
>system.
>
>Cc: qemu-stable@nongnu.org
>Fixes: 8d39f9ba14d64 ("hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()")
>Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>---
>This shows up if you run "make check" on a ubsan build.
>---
> hw/i386/pc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index 2047633e4cf..1af1a1a1823 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -1251,7 +1251,9 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
>     }
> 
>     /* Anything remaining should be a PCI NIC */
>-    pci_init_nic_devices(pci_bus, mc->default_nic);
>+    if (pci_bus) {
>+        pci_init_nic_devices(pci_bus, mc->default_nic);
>+    }

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

> 
>     rom_reset_order_override();
> }

Re: [PATCH] hw/i386/pc: Don't try to init PCI NICs if there is no PCI bus

[Paolo Bonzini]

Queued, thanks.

Paolo