Re: [PATCH v2] PCI/ASPM: Clear L1SS capability when L1 latency is unacceptable

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the ASPM sysfs show and store functions (`clkpm_show`, `clkpm_store`, `aspm_attr_show_common`, `aspm_attr_store_common`) fetch the `link_state` pointer locklessly and do not check for NULL before dereferencing it. If function 0 of a multi-function PCIe device is removed via hotplug or sysfs, `pcie_aspm_exit_link_state()` frees the parent bridge's `link_state`. Subsequent reads or writes to the ASPM sysfs attributes of remaining functions (e.g., function 1) will dereference a NULL pointer. If a sysfs write races with the removal of function 0, it results in a Use-After-Free.
--

--- Patch [1]: [PATCH v2] PCI/ASPM: Clear L1SS capability when L1 latency is unacceptable ---
commit 484f6a2751436d7940ad4ac7d04ecc20b6162859
Author: Aniket <[email protected]>

PCI/ASPM: Clear L1SS capability when L1 latency is unacceptable

This commit updates pcie_aspm_check_latency() to clear PCIE_LINK_STATE_L1SS
from link->aspm_capable when L1 exit latency is unacceptable. This prevents
exposing L1SS sysfs control files when the link cannot use L1. Additionally,
it updates pcie_config_aspm_link() to check link->aspm_support to ensure L1
PM Substates are properly cleared in hardware if they were left enabled.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 172783e7f519a..b2660892972a9 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but the ASPM sysfs show and store
functions fetch the link_state pointer locklessly and do not check for NULL
before dereferencing it.

drivers/pci/pcie/aspm.c:aspm_attr_show_common() {
    ...
	struct pci_dev *pdev = to_pci_dev(dev);
	struct pcie_link_state *link = pcie_aspm_get_link(pdev);

	return sysfs_emit(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
}

Does this code risk a NULL pointer dereference?

If function 0 of a multi-function PCIe device is removed via hotplug or sysfs,
pcie_aspm_exit_link_state() frees the parent bridge's link_state.

If an unprivileged user then reads an ASPM sysfs file on a remaining function
(like function 1), pcie_aspm_get_link() will return NULL, causing an
unconditional dereference here.

[Severity: High]
Additionally, could a concurrent sysfs write result in a use-after-free?

drivers/pci/pcie/aspm.c:clkpm_store() {
    ...
	struct pci_dev *pdev = to_pci_dev(dev);
	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
	bool state_enable;

	if (kstrtobool(buf, &state_enable) < 0)
		return -EINVAL;

	down_read(&pci_bus_sem);
	mutex_lock(&aspm_lock);

	link->clkpm_disable = !state_enable;
    ...
}

If a sysfs write races with the removal of function 0, the link pointer is
fetched locklessly before taking aspm_lock.

If the removal completes and frees the link before the lock is acquired, the
write operation will dereference freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1