Re: [PATCH v2 4/5] usb: cdns3: Add CIX Sky1 glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Massive resource leak on probe error paths (I/O memory, clocks, and resets).
- [High] Thread-safety and double-free vulnerability due to modifying a static global of_dev_auxdata array during probe.
- [High] Unbalanced runtime PM usage count and missing PM disable in cdnsp_sky1_remove.
- [High] Incorrect reset control assertion logic in remove path fails to quiesce hardware.
- [High] NULL pointer dereference when accessing xhci_dev->dev in platform_suspend.
- [Medium] OF node reference leak in sky1_of_get_addr_by_name() because the reference obtained by of_get_next_child() is never released.
- [Medium] Destructive error handling in cdnsp_sky1_clk_enable_resume corrupts internal clock pointers.
- [Medium] Potential Use-After-Free (UAF) due to missing device refcounting in iterator callbacks during shutdown.
- [Medium] Undefined behavior due to bitwise shift into the sign bit.
- [Medium] PM subsystem guideline violation: PM sleep callbacks are assigned without the required pm_sleep_ptr() wrapper.
--

commit 0232318426cffe797c62a93651116101c396ebb4
Author: Hongliang Yang <hongliang.yang@cixtech.com>

usb: cdns3: Add CIX Sky1 glue driver

This commit adds a glue layer driver for the Cadence USBSSP controller
integrated in the CIX Sky1 SoC. The driver manages the controller clocks,
resets, and the mode strap controls located in the Sky1 S5 system
controller.

> diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> new file mode 100644
> index 0000000000000..447c683b726c1
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.c

[ ... ]

> +static int cdnsp_sky1_clk_enable_resume(struct device *dev)
> +{
> +	int i, ret;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct clk **cix_usb_clks = data->cix_usb_clks;
> +
> +	for (i = 0; i < CIX_USB_CLK_OFF_NUM; i++) {
> +		ret = clk_prepare_enable(cix_usb_clks[i]);
> +		if (ret) {
> +			dev_err(dev, "failed to enable clock %s: %d\n",
> +				cix_usb_clk_names[i], ret);
> +			goto err_usb_clks;
> +		}
> +	}
> +	return 0;
> +
> +err_usb_clks:
> +	cix_usb_clks[i] = NULL;
> +	while (--i >= 0) {
> +		clk_disable_unprepare(cix_usb_clks[i]);
> +		cix_usb_clks[i] = NULL;
> +	}
> +	return ret;
> +};

[Severity: Medium]
Does setting cix_usb_clks[i] to NULL here permanently wipe the devm-managed
clock references from the driver's private state? If a system clock fails
to resume, won't subsequent calls to enable or disable these clocks receive
NULL pointers and silently fail to configure the hardware?

[ ... ]

> +static int cdns_sky1_platform_suspend(struct device *dev,
> +				      bool suspend, bool wakeup)
> +{
> +	struct cdns *cdns = dev_get_drvdata(dev);
> +	struct platform_device *xhci_dev = cdns->host_dev;
> +	struct usb_hcd  *hcd;
> +	struct device *parent = cdns->dev->parent;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(parent);
> +	u32 value;
> +	int ret = 0;
> +	int count = 3;
> +
> +	data->wakeup = wakeup;
> +
> +	if (cdns->role != USB_ROLE_HOST)
> +		return 0;
> +
> +	hcd = dev_get_drvdata(&xhci_dev->dev);

[Severity: High]
Could xhci_dev be NULL if the host initialization failed or was unbound?
If so, would accessing &xhci_dev->dev lead to an out-of-bounds pointer
dereference during system suspend?

[ ... ]

> +static void *sky1_of_get_addr_by_name(struct device_node *parent, char *name)
> +{
> +	struct device_node *node;
> +	int index;
> +
> +	node = of_get_next_child(parent, NULL);
> +	if (node) {
> +		index = of_property_match_string(node, "reg-names", name);
> +		if (index >= 0)
> +			return of_iomap(node, index);
> +	}
> +	return NULL;
> +}

[Severity: Medium]
Does of_get_next_child() return the node with an incremented reference count?
Should there be a call to of_node_put(node) before returning to prevent a
memory leak?

[ ... ]

> +static int cdnsp_sky1_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct cdnsp_sky1 *data;
> +	int ret = 0;
> +	struct cdns3_platform_data *cdns_sky1_pdata;

[ ... ]

> +	ret = cdnsp_sky1_drd_init(data);
> +	if (ret == -ETIMEDOUT)
> +		return -EPROBE_DEFER;
> +	if (ret)
> +		return ret;

[Severity: High]
If probe fails here and returns directly, are the I/O memory mappings
for xhci_base and device_base (which were mapped via non-devm of_iomap)
left active? Should they be unmapped and the clocks/resets cleaned up?

[ ... ]

> +	/* release by platform_device_release */
> +	cdns_sky1_pdata = kzalloc(sizeof(*cdns_sky1_pdata), GFP_KERNEL);
> +	if (!cdns_sky1_pdata)
> +		return -ENOMEM;
> +	cdns_sky1_pdata->platform_suspend = cdns_sky1_platform_suspend;
> +	cdns_sky1_pdata->quirks = CDNS3_DEFAULT_PM_RUNTIME_ALLOW;
> +	cdns_sky1_auxdata->platform_data = cdns_sky1_pdata;
> +	ret = of_platform_populate(node, NULL, cdns_sky1_auxdata, dev);

[Severity: High]
Is it safe to assign a dynamically allocated instance-specific pointer
(cdns_sky1_pdata) to the statically allocated global array cdns_sky1_auxdata?
Could this lead to a double-free vulnerability if multiple controller instances
exist or if the driver is repeatedly unbound and rebound?

> +	if (ret) {
> +		dev_err(dev, "failed to create children: %d\n", ret);
> +		goto err;
> +	}
> +	device_set_wakeup_capable(dev, true);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	return 0;
> +err:
> +	kfree(cdns_sky1_pdata);
> +	return ret;
> +}

[Severity: High]
Does this shared error path fail to clean up the I/O memory mappings, assert
resets, and disable clocks before returning?

> +static void cdnsp_sky1_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	pm_runtime_get_sync(dev);
> +	of_platform_depopulate(dev);
> +	sky1_put_addr(data->xhci_base);
> +	sky1_put_addr(data->device_base);
> +	reset_control_deassert(data->reset);
> +	reset_control_deassert(data->preset);

[Severity: High]
Should these be reset_control_assert() instead of deassert() to correctly
quiesce the hardware before the clocks are disabled?

> +	cdnsp_sky1_clk_disable_all(dev);
> +	platform_set_drvdata(pdev, NULL);
> +}

[Severity: High]
Does this function need to call pm_runtime_put_sync() and pm_runtime_disable()?
Because pm_runtime_get_sync(dev) increments the usage counter at the start of
the function, could omitting the decrement lead to a leaked usage count and a
broken PM state machine for the device on subsequent binds?

[ ... ]

> +static const struct dev_pm_ops cdnsp_sky1_pm_ops = {
> +	.suspend = cdnsp_sky1_system_suspend,
> +	.resume = cdnsp_sky1_system_resume,
> +};

[Severity: Medium]
Should these sleep callbacks be wrapped with pm_sleep_ptr()? Without it,
if CONFIG_PM is enabled but CONFIG_PM_SLEEP is disabled, could this generate
dead code warnings for the suspend and resume functions?

[ ... ]

> +static void cdnsp_sky1_shutdown(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct cdns *cdns = NULL;
> +	struct device *gadget_dev = NULL;
> +
> +	if (!device_may_wakeup(dev)) {
> +		/*
> +		 * Find the cdns3 child device, then find its gadget device
> +		 * and release the function driver before disabling clocks.
> +		 * This ensures all register accesses in gadget_unbind_driver
> +		 * complete before clocks are turned off.
> +		 */
> +		device_for_each_child(dev, &cdns, cdnsp_sky1_find_cdns);
> +		if (cdns)
> +			device_for_each_child(cdns->dev, &gadget_dev,
> +					      cdnsp_sky1_find_gadget_match);
> +		if (gadget_dev)
> +			device_release_driver(gadget_dev);

[Severity: Medium]
Does device_for_each_child() release the reference on the child device
immediately after the callback returns?

If so, could extracting the cdns and gadget_dev pointers out of the iterator
loop without acquiring references create a use-after-free window when they
are subsequently dereferenced?

> diff --git a/drivers/usb/cdns3/cdnsp-sky1.h b/drivers/usb/cdns3/cdnsp-sky1.h
> new file mode 100644
> index 0000000000000..49c3d5eab1744
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.h

[ ... ]

> +#define D_XEC_CFG_3XPORT_MODE_VALUE     0xa0031e07
> +#define XEC_CFG_3XPORT_MODE_VALUE       0xa0031e07
> +#define CFG_3XPORT_MODE_DIS_SSP         (~(1 << 31))

[Severity: Medium]
Does shifting 1 by 31 bits here trigger undefined behavior since 1 is a
signed int? Could this use 1U or BIT(31) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831082309.3854507-1-hongliang.yang@cixtech.com?part=4