spapr_get_fw_dev_path() is an impl of
FWPathProviderClass::get_dev_path(). This interface is used by
hw/core/qdev-fw.c via fw_path_provider_try_get_dev_path() in two
functions:

- static char *qdev_get_fw_dev_path_from_handler(), which is used only in
qdev_get_fw_dev_path_helper() and it's guarded by "if (dev &&
dev->parent_bus)";

- char *qdev_get_own_fw_dev_path_from_handler(), which is used in
softmmu/bootdevice.c in get_boot_device_path() like this:

    if (dev) {
        d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);

This means that, when called via softmmu/bootdevice.c, there's no check
of 'dev->parent_bus' being not NULL. The result is that the "BusState
*bus" arg of spapr_get_fw_dev_path() can potentially be NULL and if, at
the same time, "SCSIDevice *d" is not NULL, we'll hit this line:

    void *spapr = CAST(void, bus->parent, "spapr-vscsi");

And we'll SIGINT because 'bus' is NULL and we're accessing bus->parent.

Adding a simple 'bus != NULL' check to guard the instances where we
access 'bus->parent' can avoid this altogether.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 72f5dce751..3d6ec309dd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3053,7 +3053,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
     VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, TYPE_VHOST_SCSI_COMMON);
     PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
 
-    if (d) {
+    if (d && bus) {
         void *spapr = CAST(void, bus->parent, "spapr-vscsi");
         VirtIOSCSI *virtio = CAST(VirtIOSCSI, bus->parent, TYPE_VIRTIO_SCSI);
         USBDevice *usb = CAST(USBDevice, bus->parent, TYPE_USB_DEVICE);
-- 
2.34.1

On 1/21/22 22:38, Daniel Henrique Barboza wrote:
> spapr_get_fw_dev_path() is an impl of
> FWPathProviderClass::get_dev_path(). This interface is used by
> hw/core/qdev-fw.c via fw_path_provider_try_get_dev_path() in two
> functions:
> 
> - static char *qdev_get_fw_dev_path_from_handler(), which is used only in
> qdev_get_fw_dev_path_helper() and it's guarded by "if (dev &&
> dev->parent_bus)";
> 
> - char *qdev_get_own_fw_dev_path_from_handler(), which is used in
> softmmu/bootdevice.c in get_boot_device_path() like this:
> 
>      if (dev) {
>          d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
> 
> This means that, when called via softmmu/bootdevice.c, there's no check
> of 'dev->parent_bus' being not NULL. The result is that the "BusState
> *bus" arg of spapr_get_fw_dev_path() can potentially be NULL and if, at
> the same time, "SCSIDevice *d" is not NULL, we'll hit this line:
> 
>      void *spapr = CAST(void, bus->parent, "spapr-vscsi");
> 
> And we'll SIGINT because 'bus' is NULL and we're accessing bus->parent.
> 
> Adding a simple 'bus != NULL' check to guard the instances where we
> access 'bus->parent' can avoid this altogether.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   hw/ppc/spapr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 72f5dce751..3d6ec309dd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3053,7 +3053,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
>       VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, TYPE_VHOST_SCSI_COMMON);
>       PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>   
> -    if (d) {
> +    if (d && bus) {
>           void *spapr = CAST(void, bus->parent, "spapr-vscsi");
>           VirtIOSCSI *virtio = CAST(VirtIOSCSI, bus->parent, TYPE_VIRTIO_SCSI);
>           USBDevice *usb = CAST(USBDevice, bus->parent, TYPE_USB_DEVICE);
>