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

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

[Aniket]

pcie_aspm_cap_init() initializes link->aspm_capable from
link->aspm_support, which includes PCIE_LINK_STATE_L1SS (L1.1, L1.2,
etc.) if supported by both ends of the link.

pcie_aspm_check_latency() then evaluates whether the exit latency of
the link exceeds the acceptable L1 latency advertised by downstream
endpoints. If the latency is unacceptable, it clears
PCIE_LINK_STATE_L1 from link->aspm_capable. However, it does not clear
PCIE_LINK_STATE_L1SS.

L1 PM Substates are substates of L1 and require L1 to be supported
and enabled. If L1 is not capable due to unacceptable exit latency,
L1 PM Substates cannot be supported either. Leaving
PCIE_LINK_STATE_L1SS set in link->aspm_capable causes
aspm_ctrl_attrs_are_visible() to expose L1SS sysfs control files (e.g.,
l1_1_aspm, l1_2_aspm) even though the link cannot use L1/L1SS.

Clear PCIE_LINK_STATE_L1SS along with PCIE_LINK_STATE_L1 from
link->aspm_capable when L1 exit latency is unacceptable.

In addition, update pcie_config_aspm_link() to check link->aspm_support
instead of link->aspm_capable before invoking pcie_config_aspm_l1ss().
This ensures that pcie_config_aspm_l1ss() is called to properly clear L1
PM Substates in the hardware configuration registers if the BIOS left
them enabled, even when link->aspm_capable no longer contains
PCIE_LINK_STATE_L1SS.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 drivers/pci/pcie/aspm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 172783e7f519..b2660892972a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -666,7 +666,8 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 		latency = max_t(u32, latency_up_l1, latency_dw_l1);
 		if ((link->aspm_capable & PCIE_LINK_STATE_L1) &&
 		    (latency + l1_switch_latency > acceptable_l1))
-			link->aspm_capable &= ~PCIE_LINK_STATE_L1;
+			link->aspm_capable &= ~(PCIE_LINK_STATE_L1 |
+						PCIE_LINK_STATE_L1SS);
 		l1_switch_latency += NSEC_PER_USEC;
 
 		link = link->parent;
@@ -1041,7 +1042,7 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 		pcie_config_aspm_dev(child, 0);
 	pcie_config_aspm_dev(parent, 0);
 
-	if (link->aspm_capable & PCIE_LINK_STATE_L1SS)
+	if (link->aspm_support & PCIE_LINK_STATE_L1SS)
 		pcie_config_aspm_l1ss(link, state);
 
 	pcie_config_aspm_dev(parent, upstream);
-- 
2.55.0.766.g2966f0265a-goog

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

[sashiko-bot]

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 <aniketmaurya@google.com>

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/20260821134521.2211943-1-aniketmaurya@google.com?part=1

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

[Aniket .]

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

This is a different issue and can be fixed separately.