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

[Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>]

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 <raizudeen.kerneldev@gmail.com>
> > ---
> >  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.
> 
> >  	}
> > -	return NULL;
> > +	up_read(&pci_bus_sem);
> 
> Why not use guard() instead?
Honestly, I was so focused on getting the locking logic right that I
completely missed I meant forgot to use guard(). I will make sure to use
guard() in the v2 patch.
> 
> thanks,
> 
> greg k-h

Thanks & regards,
Mohamad Raizudeen