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

[Ilpo Järvinen]

On Wed, 20 May 2026, Gianfranco Dutka wrote:

> 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.

Hi,

To me it looks more like pci_remove_resource_files() is expected to be 
entered only once.

If this change is accepted, the 2nd entry is "silently" allowed whereas 
better course of action would feel to catch that in 
pci_remove_resource_files() with:

	if (WARN_ON_ONCE(something))
		return;

That way, developers may become aware the code has some lifetime issue.

(I'm not strictly against your change but I think it would be much better 
to have sane lifetimes and catch offenders.)

> 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
> 

-- 
 i.