Re: [PATCH 1/3] hwrng: drivers - Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

Pre-existing issues:
- [High] PM runtime and clock reference leak when the second clock defers probe.
- [High] Device IRQ is requested before hardware clocks and resets are ready.
- [High] Device IRQ is requested before the IP clock is enabled.
- [High] The driver leaks its PM runtime enablement, reset control deassertion, and clock enablements upon driver unbind because it lacks a `remove` function.
- [High] The driver queues background works (`compwork` and `startwork`) but fails to cancel them during driver removal, leading to a Use-After-Free.
- [High] The driver starts a failure recovery timer but fails to delete it when the driver is removed, leading to a Use-After-Free.
- [Medium] Clock prepare/enable leak on error path during probe.
- [Medium] PM runtime reference leak on error path during probe.
--

--- Patch [1]: [PATCH 1/3] hwrng: drivers - Remove redundant dev_err()/dev_err_probe() ---
commit 5b0e3802decb80a4bd7c81e612150dfe5af3cc8a
Author: Pan Chuang <panchuang@vivo.com>

hwrng: drivers - Remove redundant dev_err()/dev_err_probe()
    
This patch removes driver-specific dev_err() and dev_err_probe() calls
following devm_request_irq() since the core now automatically logs these
errors.

> diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
> --- a/drivers/char/hw_random/cctrng.c
> +++ b/drivers/char/hw_random/cctrng.c
> @@ -509,7 +509,7 @@ static int cctrng_probe(struct platform_device *pdev)
>  	/* register the driver isr function */
>  	rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED, "cctrng", drvdata);
>  	if (rc)
> -		return dev_err_probe(dev, rc, "Could not register to interrupt %d\n", irq);
> +		return rc;

[Severity: Medium]
This is a pre-existing issue, but does this error path leave the PM runtime
reference count elevated?

In cctrng_probe(), cc_trng_pm_get() increments the PM runtime usage count.
If devm_hwrng_register() subsequently fails, the code jumps to post_pm_err:

drivers/char/hw_random/cctrng.c:cctrng_probe() {
    ...
    rc = devm_hwrng_register(dev, &drvdata->rng);
    if (rc) {
        dev_err(dev, "Could not register hwrng device.\n");
        goto post_pm_err;
    }
    ...
post_pm_err:
    cc_trng_pm_fini(drvdata);

    return rc;
}

Since cc_trng_pm_fini() only disables PM runtime but doesn't drop the usage
count, does this permanently prevent the device's power domain from ever
suspending?

[Severity: High]
This is a pre-existing issue, but does the driver queue background works that
fail to cancel during driver removal?

The driver schedules asynchronous works (compwork and startwork) during
hardware operation. The teardown function only disables PM runtime:

drivers/char/hw_random/cctrng.c:cctrng_remove() {
    ...
    cc_trng_pm_fini(drvdata);

    dev_info(dev, "ARM cctrng device terminated\n");
}

Does this lead to a Use-After-Free if a work item is pending or executing
when the driver is unbound, since the devres framework will automatically
free the drvdata context memory?

> diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
> --- a/drivers/char/hw_random/imx-rngc.c
> +++ b/drivers/char/hw_random/imx-rngc.c
> @@ -297,7 +297,7 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
>  			irq, imx_rngc_irq, 0, pdev->name, (void *)rngc);
>  	if (ret) {
>  		clk_disable_unprepare(rngc->clk);
> -		return dev_err_probe(&pdev->dev, ret, "Can't get interrupt working.\n");
> +		return ret;
>  	}

[Severity: Medium]
This is a pre-existing issue, but does this error path fail to undo the
previous manual clock enablement?

In imx_rngc_probe(), rngc->clk is explicitly enabled via clk_prepare_enable().
If the subsequent call to devm_hwrng_register() fails, the function directly
returns the error:

drivers/char/hw_random/imx-rngc.c:imx_rngc_probe() {
    ...
    ret = devm_hwrng_register(&pdev->dev, &rngc->rng);
    if (ret)
        return dev_err_probe(&pdev->dev, ret, "hwrng registration failed\n");
    ...
}

Does this leave the hardware clock perpetually enabled without calling
clk_disable_unprepare(rngc->clk)?

> diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
> --- a/drivers/char/hw_random/jh7110-trng.c
> +++ b/drivers/char/hw_random/jh7110-trng.c
> @@ -303,8 +303,7 @@ static int starfive_trng_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(&pdev->dev, irq, starfive_trng_irq, 0, pdev->name,
>  			       (void *)trng);
>  	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "Failed to register interrupt handler\n");
> +		return ret;
>  
>  	trng->hclk = devm_clk_get(&pdev->dev, "hclk");

[Severity: High]
This is a pre-existing issue, but is it safe to request the IRQ before the
hardware clocks and resets are ready?

If a shared or pending interrupt triggers immediately upon registration,
starfive_trng_irq() could execute and read STARFIVE_ISTAT while the hardware
is unpowered and in reset.

Could this lead to a synchronous external abort when the unclocked device
registers are accessed?

[Severity: High]
This is a pre-existing issue, but does the platform driver leak resources
upon driver unbind?

The driver registers with module_platform_driver() without a remove callback:

drivers/char/hw_random/jh7110-trng.c {
static struct platform_driver starfive_trng_driver = {
    .probe = starfive_trng_probe,
    .driver = {
        .name = "jh7110-trng",
...
}

During starfive_trng_probe(), the driver manually enables clocks, deasserts
reset, and enables PM runtime.

Since there is no remove callback, does devm automatically unregister the
hwrng while leaving the manually acquired clocks, reset, and PM runtime
states permanently active?

> diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c
> --- a/drivers/char/hw_random/omap-rng.c
> +++ b/drivers/char/hw_random/omap-rng.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the immediate return on -EPROBE_DEFER
bypass manual resource cleanup?

In omap_rng_probe(), PM runtime is enabled and priv->clk is manually enabled.
The driver then attempts to get the second clock:

drivers/char/hw_random/omap-rng.c:omap_rng_probe() {
    ...
    priv->clk_reg = devm_clk_get(&pdev->dev, "reg");
    if (PTR_ERR(priv->clk_reg) == -EPROBE_DEFER)
        return -EPROBE_DEFER;
    ...
}

Does this early return bypass the err_register and err_ioremap cleanup labels,
leaving pm_runtime_put_sync(), pm_runtime_disable(), and
clk_disable_unprepare(priv->clk) uncalled?

> diff --git a/drivers/char/hw_random/xgene-rng.c b/drivers/char/hw_random/xgene-rng.c
> --- a/drivers/char/hw_random/xgene-rng.c
> +++ b/drivers/char/hw_random/xgene-rng.c
> @@ -337,7 +337,7 @@ static int xgene_rng_probe(struct platform_device *pdev)
>  	rc = devm_request_irq(&pdev->dev, ctx->irq, xgene_rng_irq_handler, 0,
>  				dev_name(&pdev->dev), ctx);
>  	if (rc)
> -		return dev_err_probe(&pdev->dev, rc, "Could not request RNG alarm IRQ\n");
> +		return rc;
>  
>  	/* Enable IP clock */
>  	clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);

[Severity: High]
This is a pre-existing issue, but is the IRQ requested before the IP clock is
enabled?

An early interrupt execution during this window could cause the handler to
read the RNG_INTR_STS_ACK register.

Could accessing an unclocked APB/AHB register lead to a system crash or bus
error?

[Severity: High]
This is a pre-existing issue, but does the driver start a failure recovery
timer that fails to get deleted when the driver is removed?

The teardown function returns without canceling the asynchronous
failure_timer:

drivers/char/hw_random/xgene-rng.c:xgene_rng_remove() {
    ...
    rc = device_init_wakeup(&pdev->dev, 0);
    if (rc)
        dev_err(&pdev->dev, "RNG init wakeup failed error %d\n", rc);
}

After remove() completes, the devres framework automatically frees the
xgene_rng_dev context. If the timer later expires, could
xgene_rng_expired_timer() execute and dereference the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710105318.376496-1-panchuang@vivo.com?part=1