diff mbox series

[v2] PCI/ASPM: Fix UAF by disabling ASPM for link when child function is removed

Message ID 20230507034057.20970-1-dinghui@sangfor.com.cn
State New
Headers
Series [v2] PCI/ASPM: Fix UAF by disabling ASPM for link when child function is removed |

Commit Message

Ding Hui May 7, 2023, 3:40 a.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, e.g.:

Reproducer:

  [root@host ~]# cat repro.sh
  #!/bin/bash
  DEV_F0="0000:03:00.0"
  echo 1 > /sys/bus/pci/devices/$DEV_F0/remove
  echo powersave > /sys/module/pcie_aspm/parameters/policy

Result:

  ==================================================================
  BUG: KASAN: slab-use-after-free in pcie_config_aspm_link+0x42d/0x500
  Read of size 4 at addr ffff8881070c80a0 by task repro.sh/2056
  CPU: 3 PID: 2056 Comm: repro.sh Not tainted 6.3.0+ #15
  Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
  Call Trace:
   <TASK>
   dump_stack_lvl+0x33/0x50
   print_address_description.constprop.0+0x27/0x310
   print_report+0x3e/0x70
   kasan_report+0xae/0xe0
   pcie_config_aspm_link+0x42d/0x500
   pcie_aspm_set_policy+0x8e/0x1a0
   param_attr_store+0x162/0x2c0
   module_attr_store+0x3e/0x80
   kernfs_fop_write_iter+0x2d5/0x460
   vfs_write+0x72e/0xae0
   ksys_write+0xed/0x1c0
   do_syscall_64+0x38/0x90
   entry_SYSCALL_64_after_hwframe+0x72/0xdc

As per PCIe spec r6.0, sec 7.5.3.7, it is recommended that software
program the same value in all Functions for Multi-Function Devices
(including ARI Devices). 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>
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
v2:
  - better commit title and message
  - update comment
  - add reproduction steps

v1: https://lore.kernel.org/lkml/20230504123418.4438-1-dinghui@sangfor.com.cn/

Link: https://lore.kernel.org/lkml/20230429132604.31853-1-dinghui@sangfor.com.cn/

---
 drivers/pci/pcie/aspm.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Comments

Bjorn Helgaas May 18, 2023, 9:54 p.m. UTC | #1 
On Sun, May 07, 2023 at 11:40:57AM +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, e.g.:
> 
> Reproducer:
> 
>   [root@host ~]# cat repro.sh
>   #!/bin/bash
>   DEV_F0="0000:03:00.0"
>   echo 1 > /sys/bus/pci/devices/$DEV_F0/remove
>   echo powersave > /sys/module/pcie_aspm/parameters/policy
> 
> Result:
> 
>   ==================================================================
>   BUG: KASAN: slab-use-after-free in pcie_config_aspm_link+0x42d/0x500
>   Read of size 4 at addr ffff8881070c80a0 by task repro.sh/2056
>   CPU: 3 PID: 2056 Comm: repro.sh Not tainted 6.3.0+ #15
>   Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0x33/0x50
>    print_address_description.constprop.0+0x27/0x310
>    print_report+0x3e/0x70
>    kasan_report+0xae/0xe0
>    pcie_config_aspm_link+0x42d/0x500
>    pcie_aspm_set_policy+0x8e/0x1a0
>    param_attr_store+0x162/0x2c0
>    module_attr_store+0x3e/0x80
>    kernfs_fop_write_iter+0x2d5/0x460
>    vfs_write+0x72e/0xae0
>    ksys_write+0xed/0x1c0
>    do_syscall_64+0x38/0x90
>    entry_SYSCALL_64_after_hwframe+0x72/0xdc
> 
> As per PCIe spec r6.0, sec 7.5.3.7, it is recommended that software
> program the same value in all Functions for Multi-Function Devices
> (including ARI Devices). 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>
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>

Applied to pci/aspm for v6.5, thank you!

> ---
> v2:
>   - better commit title and message
>   - update comment
>   - add reproduction steps
> 
> v1: https://lore.kernel.org/lkml/20230504123418.4438-1-dinghui@sangfor.com.cn/
> 
> Link: https://lore.kernel.org/lkml/20230429132604.31853-1-dinghui@sangfor.com.cn/
> 
> ---
>  drivers/pci/pcie/aspm.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 66d7514ca111..06152cc39fea 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1010,18 +1010,15 @@ 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 */
> +	/*
> +	 * For any function removed, disable ASPM for the link. See PCIe r6.0,
> +	 * sec 7.7.3.7 for details.
> +	 */
>  	pcie_config_aspm_link(link, 0);
>  	list_del(&link->sibling);
>  	/* Clock PM is for endpoint device */
> @@ -1032,7 +1029,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..06152cc39fea 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1010,18 +1010,15 @@  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 */
+	/*
+	 * For any function removed, disable ASPM for the link. See PCIe r6.0,
+	 * sec 7.7.3.7 for details.
+	 */
 	pcie_config_aspm_link(link, 0);
 	list_del(&link->sibling);
 	/* Clock PM is for endpoint device */
@@ -1032,7 +1029,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);
 }