Re: [PATCH v6 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] Uninitialized variable `ret` is returned from `cdns_drd_host_on` when `cdns->no_drd` is true.
- [High] Error path in `cdnsp_pci_probe()` conditionally frees a shared `wrap` structure, leading to a use-after-free and double-free vulnerability.
- [High] Concurrent execution of `cdnsp_pci_probe()` for both PCI functions causes a NULL pointer dereference due to lockless synchronization.
--

commit ced8f360cae2b8973233c3e0a1eebbd2eb3a4cc6
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 not implemented or inaccessible, forcing the driver to rely on
a fixed peripheral or host mode.

> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> index 432007cfe6954..feb9162298709 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 {
>  		wrap = kzalloc_obj(*wrap);

Is it possible for the Host and OTG PCI functions to be probed concurrently?

If the first thread observes pci_is_enabled(func) as true because the
second thread just enabled it, but the second thread hasn't yet reached
pci_set_drvdata() at the end of its probe, could pci_get_drvdata() return
NULL here?

This would cause a NULL pointer dereference when accessing wrap->dev_res
further down.

[ ... ]

> @@ -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);

If the second function (OTG) retrieves a shared wrap structure allocated by
the first function (Host), and platform_device_register_full() fails, the
error path will execute kfree(wrap).

Does this create a dangling pointer in the first function's drvdata, leading
to a use-after-free or double-free later when the first function is removed?

[ ... ]

> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> index 84fb38a5723a3..f87cf85cb97a1 100644
> --- a/drivers/usb/cdns3/drd.c
> +++ b/drivers/usb/cdns3/drd.c
> @@ -181,6 +184,9 @@ int cdns_drd_host_on(struct cdns *cdns)
>  	u32 val, ready_bit;
>  	int ret;
>  
> +	if (cdns->no_drd)
> +		goto phy_set;
> +
>  	/* Enable host mode. */
>  	writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,

[ ... ]

> @@ -197,6 +203,7 @@ int cdns_drd_host_on(struct cdns *cdns)
>  	if (ret)
>  		dev_err(cdns->dev, "timeout waiting for xhci_ready\n");
>  
> +phy_set:
>  	phy_set_mode(cdns->usb2_phy, PHY_MODE_USB_HOST);
>  	phy_set_mode(cdns->usb3_phy, PHY_MODE_USB_HOST);
>  	return ret;

When cdns->no_drd is true, execution jumps to the phy_set label, bypassing
the initialization of the local variable ret.

Could this return uninitialized stack memory and cause callers like
cdns_role_start() to see a spurious failure and unexpectedly abort the host
role startup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-b4-no_drd_config-v6-0-e7789cd0e581@cadence.com?part=2