[PATCH] PCI/sysfs: NULL res_attr slot after kfree as defence against double-remove

[Gianfranco Dutka]

pci_remove_resource_files() frees the bin_attribute pointed at by
pdev->res_attr[i] / pdev->res_attr_wc[i] but does not clear the slot
after kfree().  If pci_remove_sysfs_dev_files() is ever invoked twice
against the same pdev, the second pass dereferences a freed
bin_attribute and faults inside strlen() called from
kernfs_remove_by_name_ns().

To the best of my knowledge no in-tree caller hits this today -- the
primary path (pci_stop_dev() -> pci_remove_sysfs_dev_files()) is
expected to fire once per device -- so this is being submitted as
defence-in-depth rather than as a fix for a reproducer in the upstream
tree.  The motivation is that a freed-but-not-cleared pointer in a
device-lifetime table is a sharp edge: any future caller (in-tree or
out-of-tree) that re-enters the teardown path turns it into a UAF
with no warning.  Other allocator-managed pointer tables in the
kernel NULL the slot after kfree() for exactly this reason, and the
two-line change makes pci_remove_resource_files() idempotent at
essentially zero cost.

For full disclosure: the way I encountered this was via an
out-of-tree PCIe hotplug driver on an AMD Ryzen Embedded V3000
platform.  The driver re-enters pci_stop_and_remove_bus_device()
against a pdev that the standard pci_destroy_dev() path has already
torn down, and with slub_debug=FZPU the second entry faults with the
classic POISON_FREE signature:

  BUG: unable to handle page fault for address: 6b6b6b6b6b6b6b6b
  RIP: strlen+0x4
  Call Trace:
    kernfs_find_ns
    kernfs_remove_by_name_ns
    sysfs_remove_bin_file
    pci_remove_resource_files
    pci_remove_sysfs_dev_files
    pci_stop_bus_device
    pci_stop_and_remove_bus_device
    <out-of-tree hotplug driver remove path>

I am aware that "out-of-tree driver re-enters teardown" is not by
itself a reason to take a change upstream, and I would understand a
NACK on that basis.  The reason I am still sending it is that the
fix is local, mechanical, matches an idiom already used elsewhere in
the kernel, and removes a class of bug rather than papering over the
specific caller that surfaced it.

Signed-off-by: Gianfranco Dutka <gianfranco.dutka@xxxxxxxxxx>
---
 drivers/pci/pci-sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1228,11 +1228,13 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
 		if (res_attr) {
 			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
 			kfree(res_attr);
+			pdev->res_attr[i] = NULL;
 		}

 		res_attr = pdev->res_attr_wc[i];
 		if (res_attr) {
 			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
 			kfree(res_attr);
+			pdev->res_attr_wc[i] = NULL;
 		}
 	}
 }
--
2.43.0