[PATCH] ACPI: PCI: Clear driver_data on all error paths in acpi_pci_root_add()

[PATCH] ACPI: PCI: Clear driver_data on all error paths in acpi_pci_root_add()

[Chen Pei]

acpi_pci_root_add() assigns the freshly allocated root to
device->driver_data before dmar_device_add() and pci_acpi_scan_root().
Both failure paths reach the end: label where root is kfree()'d, but
only the pci_acpi_scan_root() path clears driver_data first.

When dmar_device_add() fails during a hot-add, root is freed while
device->driver_data still points at it.  The ACPI core does not clear
driver_data on attach failure, so a later acpi_pci_find_root() call
may dereference this dangling pointer.

Move the NULL assignment to the shared end: label so every error path
clears driver_data before freeing root.

Fixes: db89b4f0dbab ("ACPI: catch calls of acpi_driver_data on pointer of wrong type")
Reported-by: Sashiko AI review <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260526025118.38935-1-cp0613@linux.alibaba.com
Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
---
 drivers/acpi/pci_root.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 4c06c3ffd0cb..f6cddd11075c 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -725,7 +725,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
 		dev_err(&device->dev,
 			"Bus %04x:%02x not present in PCI namespace\n",
 			root->segment, (unsigned int)root->secondary.start);
-		device->driver_data = NULL;
 		result = -ENODEV;
 		goto remove_dmar;
 	}
@@ -765,6 +764,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
 	if (hotadd)
 		dmar_device_remove(handle);
 end:
+	device->driver_data = NULL;
 	kfree(root);
 	return result;
 }
-- 
2.50.1

Re: [PATCH] ACPI: PCI: Clear driver_data on all error paths in acpi_pci_root_add()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The acpi_pci_root_remove() function frees the acpi_pci_root object but fails to clear device->driver_data, leaving a dangling pointer.
- [High] Lockless access to `device->driver_data` causes a TOCTOU Use-After-Free race during hot-add failure and hot-remove.
--

--- Patch [1]: [PATCH] ACPI: PCI: Clear driver_data on all error paths in acpi_pci_root_add() ---
commit 9ff3c7d0724a7259ad865b11b68825024947383a
Author: Chen Pei <cp0613@linux.alibaba.com>

ACPI: PCI: Clear driver_data on all error paths in acpi_pci_root_add()

This commit addresses a dangling pointer issue in acpi_pci_root_add() by
ensuring device->driver_data is set to NULL on all error paths before
freeing the root object. This prevents use-after-free conditions during a
failed hot-add operation.

Link: https://sashiko.dev/#/patchset/20260526025118.38935-1-cp0613@linux.alibaba.com

> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 4c06c3ffd0cbb..f6cddd11075cb 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c

[ ... ]

> @@ -765,6 +764,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
>  	if (hotadd)
>  		dmar_device_remove(handle);
>  end:
> +	device->driver_data = NULL;
>  	kfree(root);

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless cleanup
introduce a use-after-free race with concurrent readers?

If acpi_pci_root_add() publishes the root pointer early:

        device->driver_data = root;

and a concurrent thread (e.g., AER or CXL probe) calls
acpi_pci_find_root() and locklessly reads this pointer:

drivers/acpi/pci_root.c:acpi_pci_find_root() {
        ...
        root = acpi_driver_data(device);
        ...
}

Could that concurrent reader dereference a freed pointer if
acpi_pci_root_add() subsequently encounters an error, clears
device->driver_data, and executes kfree(root) here without an RCU
grace period or locking to synchronize the read and the free?

[Severity: High]
This is also a pre-existing issue, but while we are fixing the error paths
here, does acpi_pci_root_remove() have the same dangling pointer
vulnerability that this patch fixes for the add path?

Looking at acpi_pci_root_remove(), it appears to free the acpi_pci_root
object but fails to clear device->driver_data:

drivers/acpi/pci_root.c:acpi_pci_root_remove() {
        ...
        kfree(root);
        ...
}

If a PCI root bridge is hot-removed and another subsystem later calls
acpi_pci_find_root(), could it locklessly read the dangling pointer
and dereference it?

>  	return result;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707121258.11640-1-cp0613@linux.alibaba.com?part=1

Re: [PATCH] ACPI: PCI: Clear driver_data on all error paths in acpi_pci_root_add()

[Chen Pei]

Thanks for the review.

> Pre-existing issues:
> - [High] The acpi_pci_root_remove() function frees the acpi_pci_root
>   object but fails to clear device->driver_data, leaving a dangling
>   pointer.

Agreed. acpi_pci_root_remove() calls kfree(root) without resetting
device->driver_data, which leaves exactly the same dangling pointer
this patch removes from the add() error paths. I'll fix it in v2 by
also clearing device->driver_data in acpi_pci_root_remove(), so the
add and remove paths leave driver_data consistent.

> - [High] Lockless access to `device->driver_data` causes a TOCTOU
>   Use-After-Free race during hot-add failure and hot-remove.

I don't think this is reachable, so I'll leave the locking model
unchanged:

- driver_data is published early (device->driver_data = root), but the
  PCI hierarchy below this root bridge is not created until
  pci_acpi_scan_root() and pci_bus_add_devices(), both of which run
  only on the success path, after the dmar_device_add() error goto.
  During the error window there are no child PCI devices yet, so an AER
  or CXL consumer under this bridge has no way to obtain the handle and
  call acpi_pci_find_root() against a half-initialized root.

- The early publish of driver_data is pre-existing; this patch does not
  change it, it only makes the error-path cleanup consistent.

- ACPI .add and .remove are serialized by acpi_scan_lock, so add cannot
  race remove.

If there is a concrete caller that can reach acpi_pci_find_root() for
this handle during the add-failure window, I'm happy to revisit. Adding
RCU or locking to the find_root() fast path otherwise seems unwarranted
for this fix.

I'll send a v2 with the acpi_pci_root_remove() fix.

Thanks,
Pei