Re: [PATCH] usb: cdnsp: drop runtime PM reference on gadget init failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usb: cdnsp: dangling cdns->gadget_dev on __cdnsp_gadget_init() failure
--

commit 70891c067700e0e2a381cc692d3a9164919460f6
Author: Wentao Liang <vulab@iscas.ac.cn>

usb: cdnsp: drop runtime PM reference on gadget init failure

This commit adds pm_runtime_put_autosuspend(cdns->dev) to the free_pdev error
path in __cdnsp_gadget_init() to drop the runtime PM reference acquired via
pm_runtime_get_sync(cdns->dev) when gadget initialization fails.

> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
> index e84405352caab..d3df4bc4a17bc 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -2083,6 +2083,7 @@ static int __cdnsp_gadget_init(struct cdns *cdns)
>  free_setup:
>  	kfree(pdev->setup_buf);
>  free_pdev:
> +	pm_runtime_put_autosuspend(cdns->dev);
>  	kfree(pdev);

[Severity: Critical]
When __cdnsp_gadget_init() fails and jumps to free_pdev, does dropping the
runtime PM reference and freeing pdev here expose a use-after-free on
cdns->gadget_dev during subsequent suspend or resume?

Before the fallible initialization steps (allocating pdev->setup_buf,
cdnsp_gen_setup(), cdnsp_gadget_init_endpoints(), usb_add_gadget_udc(), and
devm_request_threaded_irq()), __cdnsp_gadget_init() enables the DRD gadget and
stores pdev in cdns->gadget_dev:

drivers/usb/cdns3/cdnsp-gadget.c:__cdnsp_gadget_init()
	cdns_drd_gadget_on(cdns);

	pdev = kzalloc_obj(*pdev);
	if (!pdev)
		return -ENOMEM;

	pm_runtime_get_sync(cdns->dev);

	cdns->gadget_dev = pdev;

And in drivers/usb/cdns3/core.c, cdns_role_start() sets cdns->role =
USB_ROLE_DEVICE before calling start(), leaving cdns->role set to
USB_ROLE_DEVICE with state CDNS_ROLE_STATE_INACTIVE when start() fails:

drivers/usb/cdns3/core.c:cdns_role_start()
	mutex_lock(&cdns->mutex);
	cdns->role = role;
	mutex_unlock(&cdns->mutex);
	...
	mutex_lock(&cdns->mutex);
	ret = cdns->roles[role]->start(cdns);
	if (!ret)
		cdns->roles[role]->state = CDNS_ROLE_STATE_ACTIVE;
	mutex_unlock(&cdns->mutex);

Because the role state remains inactive, cdnsp_gadget_exit() is not called,
and the free_pdev path neither clears cdns->gadget_dev = NULL nor calls
cdns_drd_gadget_off(cdns).

With pm_runtime_put_autosuspend(cdns->dev) now allowing the device to enter
runtime suspend, a subsequent runtime resume or system suspend/resume will
call cdns_suspend() or cdns_resume(), which invoke
cdns->roles[cdns->role]->suspend() (cdnsp_gadget_suspend()) and
cdns->roles[cdns->role]->resume() (cdnsp_gadget_resume()) without checking if
the role state is active:

drivers/usb/cdns3/cdnsp-gadget.c:cdnsp_gadget_suspend()
	struct cdnsp_device *pdev = cdns->gadget_dev;
	unsigned long flags;

	spin_lock_irqsave(&pdev->lock, flags);
	cdnsp_disconnect_gadget(pdev);
	cdnsp_stop(pdev);

drivers/usb/cdns3/cdnsp-gadget.c:cdnsp_gadget_resume()
	struct cdnsp_device *pdev = cdns->gadget_dev;
	...
	if (!pdev->gadget_driver)
		return 0;

Both functions dereference the freed cdns->gadget_dev pointer. Should the
error path in __cdnsp_gadget_init() also clear cdns->gadget_dev = NULL and
call cdns_drd_gadget_off(cdns) (matching cdnsp_gadget_exit()), or should
cdns_suspend() and cdns_resume() check whether the role is active before
invoking the role callbacks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104021503.1468519-1-vulab@iscas.ac.cn?part=1