Re: [PATCH] PCI: Clear the bridge's subordinate pointer when child bus registration fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] PCI: missing Fixes trailer for pci_alloc_child_bus() error
  cleanup
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] PCI: double put of bridge->dev in
  pci_register_host_bridge() [1]
--

commit 145a85e5894672f4949013776fe03b0bdc6734a5
Author: Li RongQing <lirongqing@baidu.com>

> PCI: Clear the bridge's subordinate pointer when child bus registration fails
>
> pci_alloc_child_bus() stores the new bus in bridge->subordinate before
> device_register(&child->dev). If registration fails,
> put_device(&child->dev) runs release_pcibus_dev() and frees the bus, but
> bridge->subordinate still points at it.
>
> The caller returns without scanning the new bus, yet the rest of the rescan
> or removal path still follows dev->subordinate:
> pci_assign_unassigned_bridge_resources() and pci_stop_bus_device() walk
> bus->devices of the freed bus.
>
> Clear the pointer before dropping the reference.
>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

[Severity: Low]
Since 1f2768b6a3ee ("PCI: Fix reference leak in pci_alloc_child_bus()")
added the put_device(&child->dev) call that frees child on registration
failure without clearing bridge->subordinate, should this include:

  Fixes: 1f2768b6a3ee ("PCI: Fix reference leak in pci_alloc_child_bus()")

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 222ad696a1581..efa85ae052106 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1271,6 +1271,8 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  	pci_set_bus_msi_domain(child);
>  	ret = device_register(&child->dev);
>  	if (WARN_ON(ret < 0)) {
> +		if (bridge)
> +			bridge->subordinate = NULL;
>  		put_device(&child->dev);
>  		return NULL;
>  	}

[Severity: Critical]
This problem wasn't introduced by this patch, but does
pci_register_host_bridge() [1] drop bridge->dev twice when
device_register(&bus->dev) fails?

pci_register_host_bridge() takes a second reference to bridge->dev via
bus->bridge = get_device(&bridge->dev).  When device_register(&bus->dev)
fails, the error path jumps to unregister, which calls
put_device(&bridge->dev) and then put_device(&bus->dev):

drivers/pci/probe.c:pci_register_host_bridge() {
	...
	bus->bridge = get_device(&bridge->dev);
	...
	err = device_register(&bus->dev);
	bus_registered = true;
	if (err)
		goto unregister;
	...
unregister:
	put_device(&bridge->dev);
	device_del(&bridge->dev);
free:
	...
	if (bus_registered)
		put_device(&bus->dev);
}

Because put_device(&bus->dev) invokes release_pcibus_dev(), which calls
put_device(pci_bus->bridge), both references to bridge->dev are dropped
and the host bridge is freed before returning to the caller.  When a
caller such as pci_create_root_bus() then calls put_device(&bridge->dev)
on its own error path, it triggers a use-after-free and double put.

[1] https://sashiko.dev/#/bug/linux-274884f5-d0c4-48cf-95bc-82c1e0b3753d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110053410.2469-1-lirongqing@baidu.com?part=1