Re: [PATCH 1/7] s390/pci: fix double-free in zpci MSI cleanup

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Pre-existing memory leak of `struct resource_entry` in `zbus->resources` when PCI root bus creation fails.
--

--- Patch [1]: [PATCH 1/7] s390/pci: fix double-free in zpci MSI cleanup ---
commit d582c0ae794fba4fb3a5cf7c09155cbe33bdffb9
Author: Tobias Schumacher <[email protected]>

s390/pci: fix double-free in zpci MSI cleanup

zpci_remove_parent_msi_domain() can be called multiple times on the
same zbus, causing a double-free. This occurs when pci_create_root_bus()
fails after successful MSI domain creation in zpci_bus_create_pci_bus():
the error path calls zpci_remove_parent_msi_domain() to clean up, but
doesn't NULL the pointer. Later, when zpci_bus_release() is called via
kref_put(), it calls zpci_remove_parent_msi_domain() again, attempting
to free the already-freed domain and fwnode.

Add NULL check at function entry and NULL the pointer after cleanup to
make the function idempotent and safe for multiple calls.

Fixes: f770950a4709 ("s390/pci: Migrate s390 IRQ logic to IRQ domain API")
Cc: [email protected]
Signed-off-by: Tobias Schumacher <[email protected]>

> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index 9c9ed3d8d9597..c9520a16ca753 100644
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c

[Severity: Medium]
This is a pre-existing issue, but while reviewing the error path discussed in
the commit message (when pci_create_root_bus() fails), does zpci_bus_release()
leak zbus->resources?

In arch/s390/pci/pci_bus.c:zpci_bus_alloc(), a resource is added to the list:

    INIT_LIST_HEAD(&zbus->resources);
    zbus->bus_resource.start = 0;
    zbus->bus_resource.end = ZPCI_BUS_NR;
    zbus->bus_resource.flags = IORESOURCE_BUS;
    pci_add_resource(&zbus->resources, &zbus->bus_resource);

But in the error path where pci_create_root_bus() fails, zbus->bus is not set.
Later in zpci_bus_release(), pci_free_resource_list() is only called if
zbus->bus is true:

    if (zbus->bus) {
        pci_lock_rescan_remove();
        pci_stop_root_bus(zbus->bus);

        zpci_free_domain(zbus->domain_nr);
        pci_free_resource_list(&zbus->resources);

        pci_remove_root_bus(zbus->bus);
        pci_unlock_rescan_remove();
    }

Can this leak the struct resource_entry on this initialization error path?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1