Re: [PATCH] hwrng: imx-rngc: propagate clock enable errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The IRQ handler accesses hardware registers without ensuring the device is in an active PM state, which causes synchronous external aborts (bus errors) if the clock is disabled.
- [High] Stale completion state from lack of reinit_completion causes invalid hardware state reads.
--

commit 2504f9e54a2f61f40b7332a04558df267648996f
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

hwrng: imx-rngc: propagate clock enable errors

imx_rngc_probe() reads hardware registers after enabling its clock without
checking the result, and the runtime resume callback always reports success
after the same unchecked operation. Propagate clock-enable errors before
accessing the RNG and to the runtime-PM core on resume.

> diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
> index 7693328f10564..00a21d62c79c3 100644
> --- a/drivers/char/hw_random/imx-rngc.c
> +++ b/drivers/char/hw_random/imx-rngc.c
> @@ -266,7 +266,9 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return irq;
>  
> -	clk_prepare_enable(rngc->clk);
> +	ret = clk_prepare_enable(rngc->clk);
> +	if (ret)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
clock management during probe failure.

If imx_rngc_self_test() fails later in imx_rngc_probe(), the driver manually
disables the clock:

    if (self_test) {
        ret = imx_rngc_self_test(rngc);
        if (ret) {
            clk_disable_unprepare(rngc->clk);
            return dev_err_probe(&pdev->dev, ret, "self test failed\n");
        }
    }

However, the IRQ is registered with devm_request_irq() just before the
self-test, so it remains active until devres unwinds.

If a spurious interrupt occurs, or if the hardware asserts an interrupt from
the failed self-test right after the clock is disabled, the IRQ handler will
execute:

drivers/char/hw_random/imx-rngc.c:imx_rngc_irq() {
    ...
    status = readl(rngc->base + RNGC_STATUS);
    rngc->err_reg = readl(rngc->base + RNGC_ERROR);
    ...
}

Could this cause a synchronous external abort due to the MMIO access without
the clock being enabled?

Should the IRQ be disabled or unregistered before disabling the clock in this
error path, or should the IRQ handler verify the PM state before accessing
registers?


[Severity: High]
This is also a pre-existing issue, but does this driver have a potential race
condition with its completion mechanism?

The driver initializes the completion once in imx_rngc_probe():

    init_completion(&rngc->rng_op_done);

It then waits on this completion in imx_rngc_init():

    ret = wait_for_completion_timeout(&rngc->rng_op_done,
                                      msecs_to_jiffies(RNGC_SEED_TIMEOUT));

And also in imx_rngc_self_test():

    ret = wait_for_completion_timeout(&rngc->rng_op_done,
                                      usecs_to_jiffies(RNGC_SELFTEST_TIMEOUT));

If a hardware operation times out, the function returns an error without
consuming a completion event.

If the hardware finishes the operation shortly after the timeout and triggers
the interrupt handler:

drivers/char/hw_random/imx-rngc.c:imx_rngc_irq() {
    ...
    if (status & (RNGC_STATUS_SEED_DONE | RNGC_STATUS_ST_DONE))
        complete(&rngc->rng_op_done);
    ...
}

This increments the completion counter. Because the driver never calls
reinit_completion(), could the next call to imx_rngc_init() or
imx_rngc_self_test() immediately return success from
wait_for_completion_timeout() without actually waiting for the new operation
to finish?

Could this lead to reading uninitialized or stale data, compromising the
random number generator's security?

[ ... ]

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