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

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

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

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>
---
 drivers/pci/probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 86dbfd4..e07e189 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1269,6 +1269,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;
 	}
-- 
2.9.4

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

[sashiko-bot]

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

答复: [外部邮件] Re: [PATCH] PCI: Clear the bridge's subordinate pointer when child bus registration fails

[Li,Rongqing]

> 
> 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()")
> 

I think this issue has limited impact, so I didn't add a Fixes tag. Please, maintainer, check whether a Fixes tag is needed.


> > 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
> 

The issue reported by Sashiko has already been addressed by the below submit:
.
https://patchwork.kernel.org/project/linux-pci/patch/20261019161636.2540334-1-nirmoyd@nvidia.com/

Thanks

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