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

[Sathyanarayanan Kuppuswamy]

Hi,

On 5/4/23 5:34 AM, Ding Hui wrote:

Maybe you can use the following title?

"PCI/ASPM: Fix UAF by disabling ASPM for link when child function is removed

> 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),

As per PCIe spec r6.0, sec 7.5.3.7, it is recommended

> 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@xxxxxxxxxxxxxx>

Any bugzilla link with error log and reproduction steps?

> Suggestion-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

Suggested-by?

> Signed-off-by: Ding Hui <dinghui@xxxxxxxxxxxxxx>
> ---
>  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)
>  
>  	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.

How about following?

/*
 * For any function removed, disable ASPM for the link. See PCIe r6.0,
 * sec 7.7.3.7 for details.
 */

> +	 * 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);
>  }

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer