Messages in this thread | | | Date | Wed, 3 May 2023 21:31:42 +0800 | Subject | Re: [PATCH] PCI/ASPM: fix UAF by removing cached downstream | From | Ding Hui <> |
| |
On 2023/5/2 12:52 上午, Bjorn Helgaas wrote: > On Mon, May 01, 2023 at 11:50:50AM +0800, Ding Hui wrote: >> 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. > > Since we're updating the comment anyway, let's clean up the "slot" > language here. The PCIe spec doesn't use "slot" in the context of the > bus/device/function PCIe topology; it only uses it when referring to a > physical connector where a card might be installed. What we want here > is "Device," and then we have to consider whether ARI makes any > difference here. > > The spec says (referring to ASPM Control): > > For Multi-Function Devices (including ARI Devices), it is > recommended that software program the same value for this field in > all Functions. For non-ARI Multi-Function Devices, only capabilities > enabled in all Functions are enabled for the component as a whole. > > For ARI Devices, ASPM Control is determined solely by the setting in > Function 0, regardless of Function 0’s D-state. The settings in the > other Functions always return whatever value software programmed for > each, but otherwise are ignored by the component. > > A spec reference, e.g., "PCIe r6.0, sec 7.5.3.7", would be good here. > > Anyway, I think the idea of "software removing" a single function is > kind of a niche situation that we don't need to worry about > optimizing, and I think turning off ASPM completely will avoid a lot > of weird corner cases. >
Thanks for details.
I'll redo the patch with different title.
>> */ >> - if (!list_empty(&parent->subordinate->devices)) >> - goto out; >> - >> link = parent->link_state; >> root = link->root; >> parent_link = link->parent; >> >> >> -- >> Thanks, >> -dinghui >> >
-- Thanks, -dinghui
| |