Re: [PATCH] PCI/ASPM: Fix NULL pointer dereference in sysfs show/store callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci/aspm: use-after-free race in sysfs callbacks due to lockless pcie_link_state check
--

commit 9e5cf0a1d40aaa7028d286ec96a1c08c4a3dfd68
Author: Shawn Lin <shawn.lin@rock-chips.com>

    PCI/ASPM: Fix NULL pointer dereference in sysfs show/store callbacks
    
    Reading or writing a link state sysfs attribute after the upstream
    bridge's pcie_link_state has been freed triggers a NULL pointer
    dereference.

Cc: stable@vger.kernel.org

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index f8e64971c6c3d..0e0ba0e677e50 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1777,6 +1777,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 -ENODEV;
> +
>  	return sysfs_emit(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
>  }

[Severity: High]
Does this locklessly fetch and check the link pointer, leaving it exposed to
concurrent freeing?

If another process triggers the removal of function 0 of the downstream
component concurrently, pcie_aspm_exit_link_state() could acquire the locks,
clear the link state, and free the link after the check but before
sysfs_emit() dereferences it. Can this lead to a use-after-free regression?

> @@ -1788,6 +1791,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;
> +
>  	if (kstrtobool(buf, &state_enable) < 0)
>  		return -EINVAL;
>  

[Severity: High]
Could this cause a similar regression here?

The pointer is validated locklessly but dereferenced inside the lock. It can
be freed by a concurrent thread while the current thread is waiting for
pci_bus_sem or aspm_lock further down in the function.

> @@ -1836,6 +1842,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 -ENODEV;
> +
>  	return sysfs_emit(buf, "%d\n", link->clkpm_enabled);
>  }

[Severity: High]
Does this code have the same use-after-free race condition?

The link pointer is validated outside any locks, but could be freed by a
concurrent removal before it is dereferenced in sysfs_emit().

> @@ -1847,6 +1856,9 @@ static ssize_t clkpm_store(struct device *dev,
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>  	bool state_enable;
>  
> +	if (!link)
> +		return -ENODEV;
> +
>  	if (kstrtobool(buf, &state_enable) < 0)
>  		return -EINVAL;
>  

[Severity: High]
Does this demonstrate the same race condition as aspm_attr_store_common()?

The pointer is validated locklessly, but dereferenced inside the lock. It can
be freed by a concurrent thread while the current thread is waiting for the
locks further down in the function.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1791596305-153177-1-git-send-email-shawn.lin@rock-chips.com?part=1