Re: [PATCH] PCI/ASPM: Add missing NULL check for link in sysfs show/store callbacks

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Inconsistent Return Code in sysfs show callbacks

Pre-existing issues:
- [High] Use-After-Free (TOCTOU Race) due to lockless fetch of pcie_link_state
--

--- Patch [1]: [PATCH] PCI/ASPM: Add missing NULL check for link in sysfs show/store callbacks ---
commit 123c7550e88a60508484ee2915e5a520ce8e1364
Author: ZhaoJinming <zhaojinming@uniontech.com>

PCI/ASPM: Add missing NULL check for link in sysfs show/store callbacks

This commit adds NULL checks for the return value of pcie_aspm_get_link() in
the ASPM and clkpm sysfs show and store callbacks. It aims to prevent NULL
pointer dereferences when the device is not PCIe or the link state has not
been initialized.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 172783e7f519a..108d151a786e7 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1654,6 +1654,9 @@ static ssize_t aspm_attr_show_common(struct device *dev,
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>  
> +	if (!link)
> +		return 0;
> +

[Severity: Low]
Does returning 0 here mean a successful read of 0 bytes, presenting an empty
file to userspace instead of an error? Should this return -ENODEV similar
to the corresponding store callback?

>  	return sysfs_emit(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
>  }
>  
> @@ -1665,6 +1668,9 @@ static ssize_t aspm_attr_store_common(struct device *dev,
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>  	bool state_enable;
>  
> +	if (!link)
> +		return -ENODEV;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does fetching the link
locklessly here introduce a use-after-free race?

If a concurrent device hot-unplug of a sibling function occurs,
pcie_aspm_exit_link_state() could acquire the aspm_lock, free the shared link
structure, and set the parent's link_state to NULL.

Because this sysfs callback fetches the link before acquiring the lock, could
it hold a dangling pointer, which is then dereferenced later in this function
when the lock is acquired?

aspm_attr_store_common()
...
	down_read(&pci_bus_sem);
	mutex_lock(&aspm_lock);
...
	link->aspm_disable &= ~state;

Does this same lockless fetch race also exist in clkpm_store(),
aspm_attr_show_common(), and clkpm_show()?

[ ... ]

> @@ -1713,6 +1719,9 @@ static ssize_t clkpm_show(struct device *dev,
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>  
> +	if (!link)
> +		return 0;
> +

[Severity: Low]
Does returning 0 here mean a successful read of 0 bytes, presenting an empty
file to userspace instead of an error? Should this return -ENODEV similar
to the clkpm_store() callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727072915.3954329-1-zhaojinming@uniontech.com?part=1