Re: [PATCH 2/2] hwrng: cctrng - use device-managed api for runtime pm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] hwrng: cctrng: unchecked devm_pm_runtime_enable() and late
  devres registration
- [Medium] hwrng: cctrng: double clk_disable_unprepare() on unbind via
  devm_pm_runtime_enable()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] hwrng: cctrng: use-after-free due to uncancelled works in
  cctrng_remove() [1]
--

commit b87f66a9bcd0c0b30376ba4eb9926de4dd58fc40
Author: Martin Kaiser <martin@kaiser.cx>

hwrng: cctrng - use device-managed api for runtime pm

This commit replaces manual pm_runtime_enable() and pm_runtime_disable()
calls with devm_pm_runtime_enable() and removes cctrng_remove().

> diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
> index ef3ca6d8cc850..ac77c6afea5b7 100644
> --- a/drivers/char/hw_random/cctrng.c
> +++ b/drivers/char/hw_random/cctrng.c
[ ... ]
> @@ -534,25 +518,13 @@ static int cctrng_probe(struct platform_device *pdev)
>  	cc_trng_hw_trigger(drvdata);
>  
>  	/* All set, we can allow auto-suspend */
> -	cc_trng_pm_go(drvdata);
> +	devm_pm_runtime_enable(dev);

[Severity: Medium]
Should cctrng_probe() check the return value of devm_pm_runtime_enable(dev)
and call it in cc_trng_pm_init() before devm_hwrng_register()?

If devm_pm_runtime_enable() fails to allocate its devres entry,
devm_add_action_or_reset() immediately runs pm_runtime_disable_action(),
leaving runtime PM disabled, autosuspend cleared, and the usage count from
pm_runtime_resume_and_get() leaked while cctrng_probe() returns 0.

Calling devm_pm_runtime_enable() after devm_hwrng_register() and
cc_trng_hw_trigger() also means devres teardown on unbind disables runtime
PM before unregistering the hwrng device, and allows cc_trng_compwork_handler()
to call pm_runtime_put_autosuspend() while runtime PM is still disabled.

[Severity: Medium]
Can switching to devm_pm_runtime_enable(dev) cause clk_disable_unprepare()
to be called twice on drvdata->clk when unbinding the driver?

cctrng_probe() enables drvdata->clk via devm_clk_get_optional_enabled().
Unlike the previous cctrng_remove() which only called pm_runtime_disable(),
devm_pm_runtime_enable()'s cleanup action calls
pm_runtime_dont_use_autosuspend() before pm_runtime_disable().  When the
device is unbound while idle, clearing autosuspend with runtime PM still
enabled synchronously invokes cctrng_suspend(), which calls
clk_disable_unprepare(drvdata->clk) before the devres release for
devm_clk_get_optional_enabled() disables and unprepares drvdata->clk a
second time.

>  	dev_info(dev, "ARM cctrng device initialized\n");
>  
>  	return 0;
>  }
>  
> -static void cctrng_remove(struct platform_device *pdev)
> -{
> -	struct cctrng_drvdata *drvdata = platform_get_drvdata(pdev);
> -	struct device *dev = &pdev->dev;
> -
> -	dev_dbg(dev, "Releasing cctrng resources...\n");
> -
> -	cc_trng_pm_fini(drvdata);
> -
> -	dev_info(dev, "ARM cctrng device terminated\n");
> -}

[Severity: Critical]
This problem wasn't introduced by this patch, but can unbinding the driver
without cancelling drvdata->compwork and drvdata->startwork cause a
use-after-free on drvdata [1]?

CPU0                                    CPU1 (kworker)
device_release_driver()
  devres_release_all()
    devm_hwrng_release()
    devm_irq_release()
    devm_ioremap_release()
    ...                                 cc_trng_compwork_handler()
    /* drvdata freed */                   cc_iowrite(drvdata, ...)
                                            <-- use-after-free

cc_isr() schedules drvdata->compwork and cctrng_read() schedules
drvdata->startwork, and compwork can also reschedule startwork.  Nothing
cancels either work item during teardown before devres unmaps drvdata->cc_base
and frees drvdata.

[1] https://sashiko.dev/#/bug/linux-c156c6f0-25a8-4aa0-8a30-20ef6163ee06

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114170335.1680546-2-martin@kaiser.cx?part=2