Re: [PATCH v8 2/2] hwrng: starfive: rework clk/reset teardown order for JHB100

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] PM runtime reference leak in starfive_trng_read() on lock contention.
- [High] TOCTOU race in starfive_trng_release leading to double clock disable or bus error.
- [High] Prematurely dropping PM runtime reference during asynchronous hardware reseed.
- [High] Pending workqueue task is not cancelled on module teardown or probe error path, leading to Use-After-Free.
- [High] Shared reset control is left permanently deasserted if unbound while runtime-suspended and uninitialized.
- [High] Missing interrupt mask in starfive_trng_suspend causes an interrupt storm.
--

commit 7c6643a978a103f1c37f18a5471c5dac7b74eeb6
Author: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
Subject: hwrng: starfive: rework clk/reset teardown order for JHB100

This commit reworks the clock and reset teardown sequence to address
hardware-specific requirements for the JHB100 SoC. It introduces a per-compatible
match data structure to dictate reset ordering and migrates the SEU-triggered
hardware reseed path to a workqueue. The driver also switches to devm actions
for teardown management.

> diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
> index aee12caab5780..ecc7d5bd7bf51 100644
> --- a/drivers/char/hw_random/jh7110-trng.c
> +++ b/drivers/char/hw_random/jh7110-trng.c
[ ... ]
> @@ -138,6 +160,28 @@ static inline void starfive_trng_irq_mask_clear(struct starfive_trng *trng)
>  	writel(data, trng->base + STARFIVE_ISTAT);
>  }
>  
> +static void starfive_trng_release(void *data)
> +{
> +	struct starfive_trng *trng = data;
> +

[Severity: High]
Could this lead to a use-after-free if probe fails? 

The trng->work is not cancelled in this release action. If 
starfive_trng_probe() fails after registering the IRQ, an SEU interrupt could
schedule trng->work right before the trng structure is freed, leaving a
pending workqueue item pointing to freed memory.

> +	if (!pm_runtime_status_suspended(trng->dev)) {

[Severity: High]
Is there a race condition here with asynchronous autosuspend? 

The PM status is checked without ensuring PM transitions are halted. If the
device is active, this proceeds to manually disable clocks. 

If a pending autosuspend timer fires concurrently, starfive_trng_suspend()
might execute on another CPU and also attempt to disable clocks, potentially
leading to clock reference underflows or bus errors.

[Severity: High]
Does skipping this block when suspended cause the shared reset control to
remain permanently deasserted? 

If probe fails after deasserting the reset, and the device autosuspends before
starfive_trng_release() executes, the reset won't be re-asserted here.
starfive_trng_suspend() also skips asserting the reset if trng->cleanup is 0.

> +		writel(0, trng->base + STARFIVE_IE);
> +		starfive_trng_irq_clear(trng);
> +
> +		if (trng->irq >= 0)
> +			synchronize_irq(trng->irq);
> +
[ ... ]
> +static void starfive_trng_randreseed_work(struct work_struct *work)
> +{
> +	struct starfive_trng *trng = work_to_trng(work);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(trng->dev);
> +	if (ret < 0) {
> +		dev_warn(trng->dev, "Failed to wake device for reseed: %d\n", ret);
> +		return;
> +	}
> +
> +	mutex_lock(&trng->lock);
> +
> +	if (READ_ONCE(trng->cleanup))
> +		goto unlock;
> +
> +	reinit_completion(&trng->reseed_done);
> +	writel(STARFIVE_CTRL_EXEC_RANDRESEED, trng->base + STARFIVE_CTRL);
> +
> +unlock:
> +	mutex_unlock(&trng->lock);
> +
> +	pm_runtime_put_autosuspend(trng->dev);
> +}

[Severity: High]
Does this prematurely drop the PM runtime reference while the hardware is
still operating? 

Writing STARFIVE_CTRL_EXEC_RANDRESEED initiates a hardware reseed. Unlike
starfive_trng_cmd() which waits for completion, this workqueue function does
not wait for the reseed_done completion. 

Dropping the reference immediately could allow the device to suspend and
clocks to gate while the TRNG is actively reseeding, potentially corrupting
the IP state.

[ ... ]
> @@ -247,7 +358,18 @@ static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wai
>  	struct starfive_trng *trng = to_trng(rng);
>  	int ret;
>  
> -	pm_runtime_get_sync(trng->dev);
> +	ret = pm_runtime_resume_and_get(trng->dev);
> +	if (ret < 0) {
> +		dev_warn(trng->dev, "Failed to wake device for read: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (wait) {
> +		mutex_lock(&trng->lock);
> +	} else {
> +		if (!mutex_trylock(&trng->lock))
> +			return -EAGAIN;
> +	}

[Severity: High]
Does this early return leak the PM usage counter? 

The counter is incremented via pm_runtime_resume_and_get() at the top of
starfive_trng_read(). 

Returning -EAGAIN here completely bypasses the pm_runtime_put_autosuspend()
cleanup call at the bottom of the function. If called repeatedly by the hwrng
core, could this overflow the usage counter?

[ ... ]
>  static int __maybe_unused starfive_trng_suspend(struct device *dev)
>  {
>  	struct starfive_trng *trng = dev_get_drvdata(dev);
> +	bool cleanup = READ_ONCE(trng->cleanup);
> +
> +	if (cleanup && trng->data->seq_rst_clk == SEQ_RST_FIRST)
> +		reset_control_assert(trng->rst);
>  
> -	clk_disable_unprepare(trng->hclk);
>  	clk_disable_unprepare(trng->ahb);
> +	clk_disable_unprepare(trng->hclk);

[Severity: High]
Could this cause an interrupt storm when background auto-reseeding is active?

The clocks are disabled here without writing 0 to STARFIVE_IE to mask the
interrupt source. If an interrupt like SEED_DONE fires right as the clocks are
gated, the IRQ line might remain latched high. 

When starfive_trng_irq() runs, it checks pm_runtime_get_if_active() and
returns IRQ_NONE without clearing the hardware interrupt, causing the IRQ to
fire repeatedly and hang the CPU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907031603.188483-1-lianfeng.ouyang@starfivetech.com?part=2