| From 584c6b15cb11a7fa9b12c7d37f0f693759d8dc04 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Sun, 7 May 2023 11:40:57 +0800 |
| Subject: PCI/ASPM: Disable ASPM on MFD function removal to avoid |
| use-after-free |
| |
| From: Ding Hui <dinghui@sangfor.com.cn> |
| |
| [ Upstream commit 456d8aa37d0f56fc9e985e812496e861dcd6f2f2 ] |
| |
| Struct pcie_link_state->downstream is a pointer to the pci_dev of function |
| 0. Previously we retained that pointer when removing function 0, and |
| subsequent ASPM policy changes dereferenced it, resulting in a |
| use-after-free warning from KASAN, e.g.: |
| |
| # echo 1 > /sys/bus/pci/devices/0000:03:00.0/remove |
| # echo powersave > /sys/module/pcie_aspm/parameters/policy |
| |
| BUG: KASAN: slab-use-after-free in pcie_config_aspm_link+0x42d/0x500 |
| Call Trace: |
| kasan_report+0xae/0xe0 |
| pcie_config_aspm_link+0x42d/0x500 |
| pcie_aspm_set_policy+0x8e/0x1a0 |
| param_attr_store+0x162/0x2c0 |
| module_attr_store+0x3e/0x80 |
| |
| PCIe spec r6.0, sec 7.5.3.7, recommends that software program the same ASPM |
| Control value in all functions of multi-function devices. |
| |
| Disable ASPM and free the pcie_link_state when any child function is |
| removed so we can discard the dangling pcie_link_state->downstream pointer |
| and maintain the same ASPM Control configuration for all functions. |
| |
| [bhelgaas: commit log and comment] |
| Debugged-by: Zongquan Qin <qinzongquan@sangfor.com.cn> |
| Suggested-by: Bjorn Helgaas <bhelgaas@google.com> |
| Fixes: b5a0a9b59c81 ("PCI/ASPM: Read and set up L1 substate capabilities") |
| Link: https://lore.kernel.org/r/20230507034057.20970-1-dinghui@sangfor.com.cn |
| Signed-off-by: Ding Hui <dinghui@sangfor.com.cn> |
| Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| drivers/pci/pcie/aspm.c | 21 ++++++++++++--------- |
| 1 file changed, 12 insertions(+), 9 deletions(-) |
| |
| diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c |
| index 66d7514ca111b..db32335039d61 100644 |
| --- a/drivers/pci/pcie/aspm.c |
| +++ b/drivers/pci/pcie/aspm.c |
| @@ -1010,21 +1010,24 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) |
| |
| down_read(&pci_bus_sem); |
| mutex_lock(&aspm_lock); |
| - /* |
| - * All PCIe functions are in one slot, remove one function will remove |
| - * the whole slot, so just wait until we are the last function left. |
| - */ |
| - if (!list_empty(&parent->subordinate->devices)) |
| - goto out; |
| |
| link = parent->link_state; |
| root = link->root; |
| parent_link = link->parent; |
| |
| - /* All functions are removed, so just disable ASPM for the link */ |
| + /* |
| + * link->downstream is a pointer to the pci_dev of function 0. If |
| + * we remove that function, the pci_dev is about to be deallocated, |
| + * so we can't use link->downstream again. Free the link state to |
| + * avoid this. |
| + * |
| + * If we're removing a non-0 function, it's possible we could |
| + * retain the link state, but PCIe r6.0, sec 7.5.3.7, recommends |
| + * programming the same ASPM Control value for all functions of |
| + * multi-function devices, so disable ASPM for all of them. |
| + */ |
| pcie_config_aspm_link(link, 0); |
| list_del(&link->sibling); |
| - /* Clock PM is for endpoint device */ |
| free_link_state(link); |
| |
| /* Recheck latencies and configure upstream links */ |
| @@ -1032,7 +1035,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) |
| pcie_update_aspm_capable(root); |
| pcie_config_aspm_path(parent_link); |
| } |
| -out: |
| + |
| mutex_unlock(&aspm_lock); |
| up_read(&pci_bus_sem); |
| } |
| -- |
| 2.39.2 |
| |