Re: [PATCH v7 2/4] PCI: Use standard wait times for PCIe link monitoring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup of chained IRQ handlers and IRQ domains on error paths in multiple PCIe drivers leads to Use-After-Free.
- [High] advk_pcie_wait_for_link() fails to guarantee the mandatory 100ms post-reset delay before configuration requests.
--

commit 1edbe267835d4abccd02c4abc064267f402a8ce2
Author: Thierry Reding <treding@nvidia.com>

PCI: Use standard wait times for PCIe link monitoring

Instead of defining the wait values for each driver, use common values
defined in the core pci.h header file. Convert all drivers to msleep()
and use the existing definition.

> diff --git a/drivers/pci/controller/mobiveil/pcie-mobiveil.c b/drivers/pci/controller/mobiveil/pcie-mobiveil.c
> index 62ecbaeb0a604..e8346851c49b6 100644
> --- a/drivers/pci/controller/mobiveil/pcie-mobiveil.c
> +++ b/drivers/pci/controller/mobiveil/pcie-mobiveil.c
> @@ -218,11 +218,11 @@ int mobiveil_bringup_link(struct mobiveil_pcie *pcie)
>  	int retries;
>  
>  	/* check if the link is up or not */
> -	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> +	for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++) {
>  		if (mobiveil_pcie_link_up(pcie))
>  			return 0;
>  
> -		usleep_range(LINK_WAIT_MIN, LINK_WAIT_MAX);
> +		msleep(PCIE_LINK_WAIT_SLEEP_MS);
>  	}
>  
>  	dev_err(&pcie->pdev->dev, "link never came up\n");

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential
Use-After-Free in the error paths handling these link bring-up failures?

In the caller mobiveil_pcie_host_probe(), if mobiveil_bringup_link() fails, it
returns directly:

	ret = mobiveil_bringup_link(pcie);
	if (ret) {
		dev_info(dev, "link bring-up failed\n");
		return ret;
	}

Since chained handlers are set up earlier using
irq_set_chained_handler_and_data(..., pcie), returning an error causes the
devres-managed pcie struct to be freed. If the interrupt fires after this, won't
the active handler dereference the freed pcie pointer? 

A similar issue appears in nwl_pcie_probe() if nwl_wait_for_link() fails. Should
these error paths unregister the chained handlers and call irq_domain_remove()
to prevent this UAF and resource leak?

[ ... ]
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index fd9c7d53e8a78..272c5c8fc1e5c 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
[ ... ]
> @@ -350,13 +347,13 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
>  	int retries;
>  
>  	/* check if the link is up or not */
> -	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> +	for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++) {
>  		if (advk_pcie_link_up(pcie)) {
>  			pci_host_common_link_train_delay(pcie->link_gen);
>  			return 0;
>  		}
>  
> -		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> +		msleep(PCIE_LINK_WAIT_SLEEP_MS);
>  	}

[Severity: High]
This is a pre-existing issue, but does advk_pcie_wait_for_link() fail to
guarantee the mandatory 100ms post-reset delay before configuration requests?

The PCIe Base Specification mandates a minimum 100ms delay after a conventional
reset (e.g., PERST# deassertion) before the host can send Configuration
Requests. The caller advk_pcie_setup_hw() asserts PERST# and delegates the
post-reset delay to this function.

If the link trains rapidly and reaches L0 early, this loop returns immediately.
For link speeds of Gen2 or lower, pci_host_common_link_train_delay() is a no-op,
so no additional delay is added. 

Does this allow the driver to issue Configuration Requests before the endpoint's
100ms initialization period has elapsed, potentially causing enumeration failures
for compliant endpoints?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-tegra264-pcie-v7-0-eae7ae964629@nvidia.com?part=2