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

["Li,Rongqing" <lirongqing@baidu.com>]

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