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

[Greg KH]

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 <raizudeen.kerneldev@xxxxxxxxx>
> > > ---
> > >  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.

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