Re: [PATCH] PCI/ASPM: fix UAF by removing cached downstream

Ding Hui
2023-05-01T04:01:40Z

On 2023/5/1 10:10, Bjorn Helgaas wrote:
> On Sat, Apr 29, 2023 at 09:26:04PM +0800, Ding Hui wrote:
>> If the function 0 of a multifunction device is removed, an freed
>> downstream pointer will be left in struct pcie_link_state, and then
>> when pcie_config_aspm_link() be invoked from any path, we will get a
>> KASAN use-after-free report.
>
> Thanks for finding this problem, debugging it, and the patch!
>
> In this case we're doing a "software remove" and the other functions
> are still present, right? It's kind of annoying that there's only one
> link, but all the functions of a multifunction device have a Link
> Control register, and the spec "recommends" that software program the
> same ASPM control value for all the functions.
>

Yes, that is the case.

> The hardware of course doesn't know anything about this software
> remove; all the functions are still physically present and powered up.
>
> That makes me think that if software ignores the "removed" function
> and continues to operate ASPM on the N-1 remaining functions, we're
> outside the spec recommendations because the ASPM configuration is no
> longer the same across all the functions.
>
> So my inclination would be disable ASPM completely when any function
> of a multi-function device is removed. What are your thoughts on
> this?
>

Agree with you.

Previously, I thought another fix that was if the function 0 is removed,
we can free the link state to disable ASPM for this link.

Now following you suggestion, it can be expanded to any child function.

How about fixing like this?

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 66d7514ca111..657e0647d19f 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1011,12 +1011,11 @@ 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.
+ * All PCIe functions are in one slot.
+ * The spec "recommends" that software program set the same ASPM control
+ * value for all the functions.
+ * Disable ASPM when any child function is removed.
*/
- if (!list_empty(&parent->subordinate->devices))
- goto out;
-
link = parent->link_state;
root = link->root;
parent_link = link->parent;

--
Thanks,
-dinghui