diff mbox series

PCI/ASPM: fix UAF by disable ASPM for link when child function is removed

Message ID 20230504123418.4438-1-dinghui@sangfor.com.cn
State New
Headers
Series PCI/ASPM: fix UAF by disable ASPM for link when child function is removed |

Commit Message

Ding Hui May 4, 2023, 12:34 p.m. UTC 
  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@sangfor.com.cn>
Suggestion-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
 drivers/pci/pcie/aspm.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Kuppuswamy Sathyanarayanan May 5, 2023, 2:51 a.m. UTC | #1 
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@sangfor.com.cn>

Any bugzilla link with error log and reproduction steps?

> Suggestion-by: Bjorn Helgaas <bhelgaas@google.com>

Suggested-by?

> Signed-off-by: Ding Hui <dinghui@sangfor.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)
>  
>  	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);
>  }
Ding Hui May 5, 2023, 4:40 a.m. UTC | #2 
On 2023/5/5 10:51, Sathyanarayanan Kuppuswamy wrote:
> 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
> 

Thanks, I'll fix it in next patch.

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

Thanks, I'll fix it in next patch.

>> 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@sangfor.com.cn>
> 
> Any bugzilla link with error log and reproduction steps?

Yes, I should link previous
https://lore.kernel.org/lkml/20230429132604.31853-1-dinghui@sangfor.com.cn/

Since Bjorn think the ALL details is not necessary, so I'll add the
reproducer and compact result in next patch.

> 
>> Suggestion-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Suggested-by?

Sorry, I'll fix it in next patch.

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

Thanks, it's better.

>> +	 * 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);
>>   }
>
Bjorn Helgaas May 5, 2023, 8:58 p.m. UTC | #3 
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@sangfor.com.cn>
> Suggestion-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Ding Hui <dinghui@sangfor.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.

>  	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
>
Ding Hui May 6, 2023, 12:44 a.m. UTC | #4 
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@sangfor.com.cn>
>> Suggestion-by: Bjorn Helgaas <bhelgaas@google.com>
>> Signed-off-by: Ding Hui <dinghui@sangfor.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
>>
>
diff mbox series

Patch

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