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@xxxxxxxxxxxxxx>
> Suggestion-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> 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)
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
>
