[PATCH v2] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

[PATCH v2] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

[Krzysztof Wilczyński]

pci_proc_attach_device() creates procfs entries for PCI devices and is
called from pci_bus_add_device().  It lazily creates the per-bus procfs
directory (bus->procdir) via proc_mkdir() on first use, and returns
early if proc_initialized is not yet set.

On x86 with ACPI, PCI enumeration occurs at subsys_initcall, before
pci_proc_init() sets proc_initialized at device_initcall.  The
for_each_pci_dev() loop in pci_proc_init() then creates procfs entries
for these already-enumerated devices, but runs without holding
pci_rescan_remove_lock.

On ARM64 with devicetree, PCI host bridges probe at device_initcall.
With async probing enabled, pci_bus_add_device() can run concurrently
with pci_proc_init(), and both may call pci_proc_attach_device() for
the same device or for different devices on the same bus.  As
pci_host_probe() holds pci_rescan_remove_lock while pci_proc_init()
does not, there is no serialisation between the two paths.

When two threads concurrently call pci_proc_attach_device() for devices
on the same bus, both observe bus->procdir as NULL and both call
proc_mkdir().  The proc filesystem serialises directory creation
internally, so only one caller succeeds.  The other receives NULL
(duplicate entry) and unconditionally stores it to bus->procdir,
corrupting the valid pointer set by the first caller.

Thus, extract the bus procfs directory creation from
pci_proc_attach_device() into a new pci_proc_attach_bus() function,
and call it from the two bus creation paths: pci_register_host_bridge()
for root buses and pci_alloc_child_bus() for child buses.  These are
the only two callers of pci_alloc_bus(), so for buses created after
proc_initialized is set, bus->procdir is in place before any device
can be added to the bus.

Therefore, by the time pci_proc_attach_device() runs on these buses,
bus->procdir is already set and the racy proc_mkdir() call is never
reached.

For buses created before pci_proc_init() sets proc_initialized (the
common x86 ACPI case), the bus creation hooks return early.  As such,
add a fallback call to pci_proc_attach_bus() from pci_proc_attach_device()
to handle these pre-init buses.

Additionally, wrap the for_each_pci_dev() loop in pci_proc_init() with
pci_lock_rescan_remove() to serialise against concurrent PCI bus
operations, add an early return in pci_proc_attach_device() when
dev->procent is already set to make the function idempotent, and clear
bus->procdir in pci_proc_detach_bus() to prevent use of a dangling
pointer after proc_remove().

Closes: https://lore.kernel.org/linux-pci/20250702155112.40124-2-heshuan@bytedance.com/
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
Changes in v2:
  https://lore.kernel.org/linux-pci/20260430003542.455198-1-kwilczynski@kernel.org/

  - Extracted bus->procdir creation into new pci_proc_attach_bus()
    function, addressing the suggestion to move proc_mkdir() to a bus
    creation path, as per Bjorn Helgaas' feedback.
  - Added bus->procdir = NULL in pci_proc_detach_bus() for symmetry
    and to prevent dangling pointer after proc_remove().

 drivers/pci/pci.h   |  6 +++--
 drivers/pci/probe.c |  6 +++++
 drivers/pci/proc.c  | 63 +++++++++++++++++++++++++++++++--------------
 3 files changed, 53 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 13d998fbacce..5e8520bb229c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -343,13 +343,15 @@ void pci_allocate_vc_save_buffers(struct pci_dev *dev);
 
 /* PCI /proc functions */
 #ifdef CONFIG_PROC_FS
+int pci_proc_attach_bus(struct pci_bus *bus);
+int pci_proc_detach_bus(struct pci_bus *bus);
 int pci_proc_attach_device(struct pci_dev *dev);
 int pci_proc_detach_device(struct pci_dev *dev);
-int pci_proc_detach_bus(struct pci_bus *bus);
 #else
+static inline int pci_proc_attach_bus(struct pci_bus *bus) { return 0; }
+static inline int pci_proc_detach_bus(struct pci_bus *bus) { return 0; }
 static inline int pci_proc_attach_device(struct pci_dev *dev) { return 0; }
 static inline int pci_proc_detach_device(struct pci_dev *dev) { return 0; }
-static inline int pci_proc_detach_bus(struct pci_bus *bus) { return 0; }
 #endif
 
 /* Functions for PCI Hotplug drivers to use */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index bccc7a4bdd79..90248813d2cf 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1071,6 +1071,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	/* Create legacy_io and legacy_mem files for this bus */
 	pci_create_legacy_files(bus);
 
+	/* Create procfs directory for this bus */
+	pci_proc_attach_bus(bus);
+
 	if (parent)
 		dev_info(parent, "PCI host bridge to bus %s\n", name);
 	else
@@ -1279,6 +1282,9 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	/* Create legacy_io and legacy_mem files for this bus */
 	pci_create_legacy_files(child);
 
+	/* Create procfs directory for this bus */
+	pci_proc_attach_bus(child);
+
 	return child;
 }
 
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index ce36e35681e8..a33a246ec830 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -416,34 +416,51 @@ static const struct seq_operations proc_bus_pci_devices_op = {
 
 static struct proc_dir_entry *proc_bus_pci_dir;
 
-int pci_proc_attach_device(struct pci_dev *dev)
+int pci_proc_attach_bus(struct pci_bus *bus)
 {
-	struct pci_bus *bus = dev->bus;
-	struct proc_dir_entry *e;
 	char name[16];
 
 	if (!proc_initialized)
 		return -EACCES;
 
-	if (!bus->procdir) {
-		if (pci_proc_domain(bus)) {
-			sprintf(name, "%04x:%02x", pci_domain_nr(bus),
-					bus->number);
-		} else {
-			sprintf(name, "%02x", bus->number);
-		}
-		bus->procdir = proc_mkdir(name, proc_bus_pci_dir);
-		if (!bus->procdir)
-			return -ENOMEM;
-	}
+	if (bus->procdir)
+		return 0;
+
+	if (pci_proc_domain(bus))
+		sprintf(name, "%04x:%02x", pci_domain_nr(bus), bus->number);
+	else
+		sprintf(name, "%02x", bus->number);
+
+	bus->procdir = proc_mkdir(name, proc_bus_pci_dir);
+	if (!bus->procdir)
+		return -ENOMEM;
+
+	return 0;
+}
+
+int pci_proc_attach_device(struct pci_dev *dev)
+{
+	char name[16];
+	int ret;
+
+	if (!proc_initialized)
+		return -EACCES;
+
+	if (dev->procent)
+		return 0;
+
+	/* Ensure bus procdir exists for buses created before pci_proc_init() */
+	ret = pci_proc_attach_bus(dev->bus);
+	if (ret)
+		return ret;
 
 	sprintf(name, "%02x.%x", PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
-	e = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR, bus->procdir,
-			     &proc_bus_pci_ops, dev);
-	if (!e)
+	dev->procent = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR,
+					dev->bus->procdir, &proc_bus_pci_ops,
+					dev);
+	if (!dev->procent)
 		return -ENOMEM;
-	proc_set_size(e, dev->cfg_size);
-	dev->procent = e;
+	proc_set_size(dev->procent, dev->cfg_size);
 
 	return 0;
 }
@@ -458,18 +475,24 @@ int pci_proc_detach_device(struct pci_dev *dev)
 int pci_proc_detach_bus(struct pci_bus *bus)
 {
 	proc_remove(bus->procdir);
+	bus->procdir = NULL;
 	return 0;
 }
 
 static int __init pci_proc_init(void)
 {
 	struct pci_dev *dev = NULL;
+
 	proc_bus_pci_dir = proc_mkdir("bus/pci", NULL);
 	proc_create_seq("devices", 0, proc_bus_pci_dir,
-		    &proc_bus_pci_devices_op);
+			&proc_bus_pci_devices_op);
+
 	proc_initialized = 1;
+
+	pci_lock_rescan_remove();
 	for_each_pci_dev(dev)
 		pci_proc_attach_device(dev);
+	pci_unlock_rescan_remove();
 
 	return 0;
 }
-- 
2.54.0

Re: [PATCH v2] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] TOCTOU race leading to Use-After-Free and resource leaks via corrupted procfs pointer
--

commit cd2e3cd5b4869787fb5b78b8cfef90b55821ad2b
Author: Krzysztof Wilczyński <kwilczynski@kernel.org>

PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

This commit attempts to fix a race condition between pci_proc_init() and
pci_bus_add_device() by lazily attaching procfs directories when buses are
created, and by protecting the device iteration in pci_proc_init() with
pci_lock_rescan_remove().

Closes: https://lore.kernel.org/linux-pci/20250702155112.40124-2-heshuan@bytedance.com/

> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index ce36e35681e8e..a33a246ec8309 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -416,34 +416,51 @@ static const struct seq_operations proc_bus_pci_devices_op = {
>  
>  static struct proc_dir_entry *proc_bus_pci_dir;
>  
> +int pci_proc_attach_bus(struct pci_bus *bus)
> +{
> +	char name[16];
> +
> +	if (!proc_initialized)
> +		return -EACCES;
> +
> +	if (bus->procdir)
> +		return 0;
> +
> +	if (pci_proc_domain(bus))
> +		sprintf(name, "%04x:%02x", pci_domain_nr(bus), bus->number);
> +	else
> +		sprintf(name, "%02x", bus->number);
> +
> +	bus->procdir = proc_mkdir(name, proc_bus_pci_dir);
> +	if (!bus->procdir)
> +		return -ENOMEM;

[Severity: High]
Can this direct assignment overwrite a valid pointer with NULL if a concurrent
creation fails?

If an asynchronous probe runs concurrently without holding
pci_rescan_remove_lock(), two threads might both observe bus->procdir as
NULL. If one successfully creates the entry, the second could fail due to a
name collision and return NULL, unconditionally overwriting the valid
bus->procdir.

Could this cause subsequent child devices to erroneously create their proc
files in the root of /proc?

> +
> +	return 0;
> +}
> +

[ ... ]

>  	sprintf(name, "%02x.%x", PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> -	e = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR, bus->procdir,
> -			     &proc_bus_pci_ops, dev);
> -	if (!e)
> +	dev->procent = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR,
> +					dev->bus->procdir, &proc_bus_pci_ops,
> +					dev);
> +	if (!dev->procent)
>  		return -ENOMEM;

[Severity: High]
Does this assignment introduce a risk of a use-after-free and resource leak?

Similar to the bus directory, if concurrent threads call
pci_proc_attach_device() for the same device, one might overwrite a
valid dev->procent with NULL when proc_create_data() fails.

Later, pci_proc_detach_device() would call proc_remove(NULL) which does
nothing. The procfs file would be leaked, and because the file's data points
to the freed pci_dev struct, subsequent reads from userspace could trigger a
use-after-free.

> -	proc_set_size(e, dev->cfg_size);
> -	dev->procent = e;
> +	proc_set_size(dev->procent, dev->cfg_size);
>  
>  	return 0;
>  }

[ ... ]

>  static int __init pci_proc_init(void)
>  {
>  	struct pci_dev *dev = NULL;
> +
>  	proc_bus_pci_dir = proc_mkdir("bus/pci", NULL);
>  	proc_create_seq("devices", 0, proc_bus_pci_dir,
> -		    &proc_bus_pci_devices_op);
> +			&proc_bus_pci_devices_op);
> +
>  	proc_initialized = 1;
> +
> +	pci_lock_rescan_remove();
>  	for_each_pci_dev(dev)
>  		pci_proc_attach_device(dev);
> +	pci_unlock_rescan_remove();

[Severity: High]
Is this lock sufficient to serialize against lockless async probe paths?

Paths like the VMD driver (vmd_probe() -> vmd_enable_domain() ->
pci_bus_add_devices()) or platform drivers can run asynchronously and do not
always hold pci_rescan_remove_lock().

If an asynchronous probe adds devices to the global pci_bus_type list before
calling pci_proc_attach_device(), and pci_proc_init() iterates the list
under the lock, could both threads still find the new devices and call
pci_proc_attach_device() concurrently?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606170146.673140-1-kwilczynski@kernel.org?part=1

Re: [PATCH v2] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

[Krzysztof Wilczyński]

Hello,

> > +	bus->procdir = proc_mkdir(name, proc_bus_pci_dir);
> > +	if (!bus->procdir)
> > +		return -ENOMEM;
> 
> [Severity: High]
> Can this direct assignment overwrite a valid pointer with NULL if a concurrent
> creation fails?
> 
> If an asynchronous probe runs concurrently without holding
> pci_rescan_remove_lock(), two threads might both observe bus->procdir as
> NULL. If one successfully creates the entry, the second could fail due to a
> name collision and return NULL, unconditionally overwriting the valid
> bus->procdir.
> 
> Could this cause subsequent child devices to erroneously create their proc
> files in the root of /proc?

[...]
> >  	sprintf(name, "%02x.%x", PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> > -	e = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR, bus->procdir,
> > -			     &proc_bus_pci_ops, dev);
> > -	if (!e)
> > +	dev->procent = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR,
> > +					dev->bus->procdir, &proc_bus_pci_ops,
> > +					dev);
> > +	if (!dev->procent)
> >  		return -ENOMEM;
> 
> [Severity: High]
> Does this assignment introduce a risk of a use-after-free and resource leak?
> 
> Similar to the bus directory, if concurrent threads call
> pci_proc_attach_device() for the same device, one might overwrite a
> valid dev->procent with NULL when proc_create_data() fails.
> 
> Later, pci_proc_detach_device() would call proc_remove(NULL) which does
> nothing. The procfs file would be leaked, and because the file's data points
> to the freed pci_dev struct, subsequent reads from userspace could trigger a
> use-after-free.

I will send another revision to address this feedback.

Thank you!

	Krzysztof