Re: [PATCH] PCI/ASPM: Update ASPM sysfs on MFD function removal to avoid use-after-free

[Jay Fang <f.fangjian@huawei.com>]

On 2024/8/1 5:46, Bjorn Helgaas wrote:
> On Tue, Jul 30, 2024 at 05:57:43PM +0800, Ding Hui wrote:
>> On 2024/7/30 9:16, Jay Fang wrote:
>>>  From 'commit 456d8aa37d0f ("PCI/ASPM: Disable ASPM on MFD function removal
>>> to avoid use-after-free")' we know that PCIe spec r6.0, sec 7.5.3.7,
>>> recommends that software program the same ASPM Control(pcie_link_state)
>>> value in all functions of multi-function devices, and free the
>>> pcie_link_state when any child function is removed.
>>>
>>> However, ASPM Control sysfs is still visible to other children even if it
>>> has been removed by any child function, and careless use it will
>>> trigger use-after-free error, e.g.:
>>>
>>>    # lspci -tv
>>>      -[0000:16]---00.0-[17]--+-00.0  Device 19e5:0222
>>>                              \-00.1  Device 19e5:0222
>>>    # echo 1 > /sys/bus/pci/devices/0000:17:00.0/remove       // pcie_link_state will be released
>>>    # echo 1 > /sys/bus/pci/devices/0000:17:00.1/link/l1_aspm // will trigger error
>>>
>>>    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030
>>>    Call trace:
>>>     aspm_attr_store_common.constprop.0+0x10c/0x154
>>>     l1_aspm_store+0x24/0x30
>>>     dev_attr_store+0x20/0x34
>>>     sysfs_kf_write+0x4c/0x5c
>>>
>>> We can solve this problem by updating the ASPM Control sysfs of all
>>> children immediately after ASPM Control have been freed.
>>>
>>> Fixes: 456d8aa37d0f ("PCI/ASPM: Disable ASPM on MFD function removal to avoid use-after-free")
>>> Signed-off-by: Jay Fang <f.fangjian@huawei.com>
>>> ---
>>>   drivers/pci/pcie/aspm.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index cee2365e54b8..eee9e6739924 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -1262,6 +1262,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>>>   		pcie_config_aspm_path(parent_link);
>>>   	}
>>> +	pcie_aspm_update_sysfs_visibility(parent);
>>> +
>>
>> To be more rigorous, is there still a race window in
>> aspm_attr_{show,store}_common or clkpm_{show,store} before updating
>> the visibility, we can get an old or NULL pointer by
>> pcie_aspm_get_link()?
> 
> Yeah, I think we still have a problem even with this patch.
If so, maybe we need a new solution to completely sovle this problem.

> 
> For one thing, aspm_attr_store_common() captures the pointer from
> pcie_aspm_get_link() before the critical section, so by the time it
> *uses* the pointer, pcie_aspm_exit_link_state() may have freed the
> link state.
> 
> And there are several other callers of pcie_aspm_get_link() that
> either call it before a critical section or don't have a critical
> section at all.
> 
> I think it may be a mistake to alloc/free the link state separately
> from the pci_dev itself.
> 
>>>   	mutex_unlock(&aspm_lock);
>>>   	up_read(&pci_bus_sem);
>>>   }
>>
>> -- 
>> Thanks,
>> - Ding Hui
>>
> 
> .
>