linux-kernel - Re: [PATCH] PCI/ASPM: fix UAF by disable ASPM for link when child function is removed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 6 May 2023 08:44:19 +0800
From:   Ding Hui <dinghui@...gfor.com.cn>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     bhelgaas@...gle.com, sathyanarayanan.kuppuswamy@...ux.intel.com,
        vidyas@...dia.com, david.e.box@...ux.intel.com,
        kai.heng.feng@...onical.com, michael.a.bottini@...ux.intel.com,
        rajatja@...gle.com, qinzongquan@...gfor.com.cn,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI/ASPM: fix UAF by disable ASPM for link when child
 function is removed

On 2023/5/6 4:58, Bjorn Helgaas wrote:
> On Thu, May 04, 2023 at 08:34:18PM +0800, Ding Hui wrote:
>> If the Function 0 of a Multi-Function device is software removed,
>> a 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 trigger use-after-free.
>>
>> Based on the PCIe spec about ASPM Control (PCIe r6.0, sec 7.5.3.7),
>> for Multi-Function Devices (including ARI Devices), it is recommended
>> that software program the same value in all Functions. For ARI
>> Devices, ASPM Control is determined solely by the setting in Function 0.
>>
>> So we can just disable ASPM of the whole component if any child
>> function is removed, the downstream pointer will be avoided from
>> use-after-free, that will also avoid other potential corner cases.
>>
>> Fixes: b5a0a9b59c81 ("PCI/ASPM: Read and set up L1 substate capabilities")
>> Debugged-by: Zongquan Qin <qinzongquan@...gfor.com.cn>
>> Suggestion-by: Bjorn Helgaas <bhelgaas@...gle.com>
>> Signed-off-by: Ding Hui <dinghui@...gfor.com.cn>
>> ---
>>   drivers/pci/pcie/aspm.c | 15 +++++++--------
>>   1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 66d7514ca111..1bf8306141aa 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -1010,18 +1010,17 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
> 
> Not directly related to your patch, but this looks racy to me:
> 
>    void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>    {
>      struct pci_dev *parent = pdev->bus->self;
> 
>      if (!parent || !parent->link_state)
>        return;
> 
>      down_read(&pci_bus_sem);
>      mutex_lock(&aspm_lock);
> 
>      link = parent->link_state;
>      root = link->root;
>      ...
>      free_link_state(link);
>        link->pdev->link_state = NULL;
>        kfree(link);
> 
> Since we check "parent->link_state" before acquiring the locks, I
> suspect that removing two functions at the same time could end up with
> a NULL pointer dereference:
> 
>    pcie_aspm_exit_link_state(fn 0)    pcie_aspm_exit_link_state(fn 1)
>    parent = X                         parent = X
>    parent->link_state != NULL         parent->link_state != NULL
> 
>    acquire locks
>    free_link_state(link)
>    link->pdev->link_state = NULL # aka parent->link_state
>    kfree(link)
>    release locks
> 
>                                       acquire locks
>                                       link = parent->link_state # now NULL
>                                       root = link->root         # NULL ptr
> 
> What do you think?  I guess if this *is* a race, it should be fixed by
> a separate patch.
> 

Maybe there's no need to worry about the race, when a pci device is removing,
the pci_rescan_remove_lock should be acquired.

Anyway, double check would be safer.

>>   	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 */
>> +	/*
>> +	 * Any function is removed (including software removing), just
>> +	 * disable ASPM for the link, in case we can not configure the same
>> +	 * setting for all functions.
>> +	 * See PCIe r6.0, sec 7.5.3.7.
>> +	 */
>>   	pcie_config_aspm_link(link, 0);
>>   	list_del(&link->sibling);
>>   	/* Clock PM is for endpoint device */
>> @@ -1032,7 +1031,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.17.1
>>
> 

-- 
Thanks,
- Ding Hui