Re: [PATCH v7 2/2] usb: cdnsp: Add support for device-only configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] An error in the second probed function's platform_device_register_full() frees the shared wrap structure, leaving a dangling pointer in the first function's drvdata.
- [High] Concurrent probing of multifunction PCI devices leads to a NULL pointer dereference in cdnsp_pci_probe() due to a TOCTOU race on pci_is_enabled().
- [High] Removing PCI function 0 before function 1 leaves the shared platform device active while its underlying PCI resources are freed.
--

commit 720e53ba9595caee97ecf44de50fbb028048bc27
Author: Pawel Laszczak <pawell@cadence.com>

usb: cdnsp: Add support for device-only configuration

This commit introduces support for the Cadence USBSSP (cdnsp) controller in
hardware configurations where the Dual-Role Device (DRD) register block is
inaccessible. The driver is updated to operate exclusively in a fixed
peripheral/host mode for these configurations. It also modifies the BAR
indexing to accommodate the 32-bit addressing used in the DRD-disabled
hardware layout.

Closes: https://lore.kernel.org/oe-kbuild-all/202605141023.18vWXyw3-lkp@intel.com/

> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> index 432007cfe6954..7997af333307c 100644
> --- a/drivers/usb/cdns3/cdnsp-pci.c
> +++ b/drivers/usb/cdns3/cdnsp-pci.c
> @@ -93,7 +104,7 @@ static int cdnsp_pci_probe(struct pci_dev *pdev,
>  
>  	pci_set_master(pdev);
>  
> -	if (pci_is_enabled(func)) {
> +	if (func && pci_is_enabled(func)) {
>  		wrap = pci_get_drvdata(func);
>  	} else {

This isn't a bug introduced by this patch, but can concurrent probing of
multifunction PCI devices lead to a NULL pointer dereference here?

If the PCI subsystem probes the two functions concurrently, both could call
pcim_enable_device() before checking the other's status. Since both are
enabled, pci_is_enabled(func) evaluates to true for both, and both
threads execute wrap = pci_get_drvdata(func).

Because neither thread has reached the end of the probe function to call
pci_set_drvdata(), drvdata would be NULL for both, causing a panic when
wrap->dev_res is unconditionally dereferenced below.

[ ... ]

> @@ -158,6 +182,7 @@ static int cdnsp_pci_probe(struct pci_dev *pdev,
>  		plat_info.dma_mask = pdev->dma_mask;
>  		plat_info.data = &pdata;
>  		plat_info.size_data = sizeof(pdata);
> +		plat_info.properties = wrap->prop;
>  		wrap->devfn = pdev->devfn;
>  		/* register platform device */
>  		wrap->plat_dev = platform_device_register_full(&plat_info);

This isn't a bug introduced by this patch, but if
platform_device_register_full() fails during the second function's probe,
the error path executes kfree(wrap).

Because wrap is still stored in the first function's drvdata via
pci_set_drvdata(), does this leave a dangling pointer?

When the first function is subsequently unbound, cdnsp_pci_remove()
retrieves this freed wrap and calls kfree(wrap) again, potentially leading
to a use-after-free or double free.

[ ... ]

> @@ -185,13 +210,17 @@ static void cdnsp_pci_remove(struct pci_dev *pdev)
>  	if (wrap->devfn == pdev->devfn)
>  		platform_device_unregister(wrap->plat_dev);

This isn't a bug introduced by this patch, but could unbinding function 0
before function 1 leave the shared platform device active while its
underlying PCI resources are freed?

The platform device relies on memory-mapped BARs from both PCI function 0
and function 1. If a user unbinds function 0 first,
wrap->devfn == pdev->devfn will be false (as wrap->devfn matches the last
probed function, usually 1), skipping platform_device_unregister().

The PCI core then disables function 0 and unmaps its BARs, but the
platform driver remains active and continues to perform MMIO accesses to
the unmapped BARs, which could crash the system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-b4-no_drd_config-v7-0-18cc40e363fd@cadence.com?part=2