Re: [PATCH v3] usb: cdns3: Fix PCI device reference leak in cdns3_pci_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] usb: cdns3: TOCTOU race in probe corrupts shared wrap state
- [High] pci: UAF of MMIO resources on partial unbind of compound device
- [Medium] usb: cdns3: PCI device reference leak on pcim_enable_device() failure
- [Medium] pci: memory leak of cdns3_wrap on concurrent unbind due to devres timing
--

commit 8da98a756feb810e31a39db878f286ba953f3a5d
Author: Ma Ke <make_ruc2021@163.com>
Date:   Tue Oct 13 02:08:27 2026 +0000

    usb: cdns3: Fix PCI device reference leak in cdns3_pci_probe()

This commit addresses a reference leak of the sibling PCI function in
cdns3_pci_probe() and cdns3_pci_remove() by ensuring pci_dev_put() is
called. It also tracks the allocation of the shared cdns3_wrap structure
to avoid freeing a dangling pointer when the second function fails probe.

> diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c b/drivers/usb/cdns3/cdns3-pci-wrap.c
> index fd06a3d8e638f..31089eb9ca29f 100644
> --- a/drivers/usb/cdns3/cdns3-pci-wrap.c
> +++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
> @@ -72,6 +72,7 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
>  	struct cdns3_wrap *wrap;
>  	struct resource *res;
>  	struct pci_dev *func;
> +	bool own_wrap;
>  	int err;

[Severity: Medium]
Does this code still leak the func reference if pcim_enable_device() fails?

Earlier in cdns3_pci_probe(), before the changes in this patch take effect,
we have this code:

	func = cdns3_get_second_fun(pdev);
	if (unlikely(!func))
		return -EINVAL;

	err = pcim_enable_device(pdev);
	if (err) {
		dev_err(&pdev->dev, "Enabling PCI device has failed %d\n", err);
		return err;
	}

If pcim_enable_device() fails here, we return immediately without dropping
the func reference acquired by cdns3_get_second_fun(). Should this error path
be routed to the new put_pci label as well?

[ ... ]

> @@ -99,12 +100,17 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
>  		if (!wrap) {
>  			dev_err(&pdev->dev,
>  				"second function not initialized, retrying\n");
> -			return -EPROBE_DEFER;
> +			err = -EPROBE_DEFER;
> +			goto put_pci;
>  		}

[Severity: High]
Can a Time-of-Check to Time-of-Use race happen here during concurrent probe?

If both function 0 and function 1 are probed concurrently (e.g., via sysfs
or async boot probing):

1. CPU0 enables device 0.

2. CPU1 enables device 1, sees device 0 is enabled, but wrap is NULL because
   CPU0 hasn't set drvdata yet. CPU1 defers and keeps device 1 enabled.

3. CPU0 sees device 1 enabled, assumes wrap is initialized, and registers a
   platform device with incomplete resources.

4. CPU1 retries later, gets the wrap structure from CPU0, and proceeds to
   register a second platform device.

This second platform device registration overwrites wrap->plat_dev and
wrap->devfn. Since the remove method relies on wrap->devfn to unregister the
platform device, does this permanently leak the first platform device?

[ ... ]

> @@ -184,8 +193,10 @@ static void cdns3_pci_remove(struct pci_dev *pdev)
>  	if (wrap->devfn == pdev->devfn)
>  		platform_device_unregister(wrap->plat_dev);

[Severity: High]
Does this leave the platform device accessing disabled MMIO resources on a
partial unbind?

The driver registers a single platform device for both functions, assigning it
the resources (BARs) of both. If the function that was probed first is unbound
via sysfs, wrap->devfn != pdev->devfn (since only the second probed function
records its devfn). The unregister is skipped.

When cdns3_pci_remove() completes, pcim_release() will disable the unbound
function's memory decoding. The platform device remains registered and its
driver will continue accessing disabled MMIO regions. Can this lead to master
aborts or system hangs?

> -	if (!pci_is_enabled(func))
> +	if (!func || !pci_is_enabled(func))
>  		kfree(wrap);

[Severity: Medium]
Will this skip freeing the shared cdns3_wrap on concurrent unbind?

If both functions are unbound concurrently via sysfs, they will both enter
cdns3_pci_remove() at the same time. Since device enablement is managed by
devres (via pcim_enable_device), devres_release_all() only decrements
enable_cnt after the remove function returns.

When both threads check pci_is_enabled(func) for their sibling, both will see
it as true (because neither sibling's devres cleanup has run yet). Both will
skip the kfree(wrap) call, permanently leaking the structure.

> +
> +	pci_dev_put(func);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013020827.4099844-1-make_ruc2021@163.com?part=1