Re: [PATCH] PCI: Fix use-after-free race in pci_find_bus()

On Wed, Aug 12, 2026 at 04:45:14PM +0900, Greg KH wrote:
> On Wed, Aug 12, 2026 at 12:56:59PM +0530, Mohamad Raizudeen wrote:
> > On Wed, Aug 12, 2026 at 12:00:33PM +0900, Greg KH wrote:
> > > On Wed, Aug 12, 2026 at 08:17:13AM +0530, Mohamad Raizudeen wrote:
> > > > pci_find_bus() iterates over the list of PCI root buses using
> > > > pci_find_next_bus(). This helper acquires pci_bus_sem, retrieves the
> > > > next bus and drops the lock before returning the pointer to the caller.
> > > > 
> > > > pci_find_bus() then uses this pointer to check the domain and traverses
> > > > the child buses via pci_do_find_bus() without holding the pci_bus_sem
> > > > lock.
> > > > 
> > > > If a PCI bus is concurrently removed for example via hotplug between
> > > > loop iterations, the from pointer passed back into pci_find_next_bus()
> > > > becomes stale, leading to a user-after-free when dereferencing
> > > > from->node.next. Additionally, traversing the bus tree without holding
> > > > the lock is a race condition.
> > > 
> > > Did you find this actually happens?  How did you find this at all?
> > 
> > I found this purely through by reading and reviewing the code, while
> > analyzing the locking patterns in the PCI subsystem. I have not seen it
> > crash in production, but the race condition is statically clear from
> > reading the code.
> > >
> > > > 
> > > > Fix this by iterating pci_root_buses list directly using
> > > > list_for_each_entry() inside pci_find_bus() while holding the
> > > > pci_bus_sem read lock for the entire duration of the search. This
> > > > ensures the list and tree structures cannot change while being
> > > > traversed, eliminating the use-after-free.
> > > 
> > > Why do two changes here, and not just make a patch series?
> > I did both in one patch because they are connected. Since
> > pci_find_next_bus() drops the lock early, I couldn't use it to hold the
> > lock for the whole search. I had to change the loop to fix the locking.
> > > 
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Mohamad Raizudeen <[email protected]>
> > > > ---
> > > >  drivers/pci/search.c | 20 +++++++++++---------
> > > >  1 file changed, 11 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > > > index e3d3177fce54..f50e83061b76 100644
> > > > --- a/drivers/pci/search.c
> > > > +++ b/drivers/pci/search.c
> > > > @@ -142,17 +142,19 @@ static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr)
> > > >   */
> > > >  struct pci_bus *pci_find_bus(int domain, int busnr)
> > > >  {
> > > > -	struct pci_bus *bus = NULL;
> > > > -	struct pci_bus *tmp_bus;
> > > > +	struct pci_bus *bus;
> > > > +	struct pci_bus *tmp_bus = NULL;
> > > >  
> > > > -	while ((bus = pci_find_next_bus(bus)) != NULL)  {
> > > > -		if (pci_domain_nr(bus) != domain)
> > > > -			continue;
> > > > -		tmp_bus = pci_do_find_bus(bus, busnr);
> > > > -		if (tmp_bus)
> > > > -			return tmp_bus;
> > > > +	down_read(&pci_bus_sem);
> > > > +	list_for_each_entry(bus, &pci_root_buses, node) {
> > > > +		if (pci_domain_nr(bus) == domain) {
> > > > +			tmp_bus = pci_do_find_bus(bus, busnr);
> > > > +			if (tmp_bus)
> > > > +				break;
> > > > +		}
> > > 
> > > Are you sure this logic is the same as the original?
> > Yes, I used list_for_each_entry() that does the exact same thing as the
> > old while loop, but it let me keep the lock saefely for the whole
> > search.
> > 
> > > pci_find_next_bus() does grab the needed lock here, so why do you think
> > > this is racy?
> > You are right, it grabs the lock. But it drops the lock before returning
> > the bus pointer. So the caller then uses that pointer without a lock and
> > passes it back for the next loop. If a bus is removed in that moment,
> > the next call reads freed memory.
> > > 
> > > And pci_bus_sem is just for root busses, not the individual busses,
> > > right?
> > It protects the root bus list, but it also protects the child buses.
> > Since pci_do_find_bus() walks through the child buses, needed to hold
> > the lock to make that safe too.
> > > 
> > > How was this tested?
> > I compiled ir and booted it in x86_64 qemu vm. It booted fine without
> > any PCI crashes. I will run the same test on v2 patch before sending it.
> 
> Booting in a vm is very simple as a vm does not have many PCI devices.
> Try it on a real system with a big topology as well as a pci hotplug
> system please.
>
Yes, I agree and to be honest my hardware is quite limited. I do not
have access to physical machine with a large PCI topology or hotplug
capabilities to test this properly.

> Also note that this function should only be called when pci devices are
> being added to the system, so odds are it can't race with a device being
> removed due to the pci bus lock in the first place, right?
> 
> thanks,
> 
> greg k-h

Well... I understand now. If the callers that add devices to the system
like pci_host_probe() or pci_scan_child_bus(). they are already holding
the pci_lock_rescan_remove() lock, then pci_find_bus() can't race with a
removal. Actually, I was worried because pci_find_bus() is exported
symbol so I thought an external driver might call it without holding
that lock. Now I understood clearly.

Thanks & regards,
Mohamad Raizudeen