Re: [PATCH v2] hw_random: timeriomem-rng: add configurable read width and data mask

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but an improper teardown sequence allows a concurrent reader to restart the timer after it was cancelled, leading to a Use-After-Free and potential Deadlock.
--

commit f3fa66cce2c0ad3af71293c86473cf060b400645
Author: Jad Keskes <inasj268@gmail.com>

hw_random: timeriomem-rng: add configurable read width and data mask

This commit adds support for reading 8, 16, or 32 bits from the hardware
random number generator. It applies an optional mask to the data and
configures these properties via device tree or platform data.

> diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
> index e61f063932090..4557326618c90 100644
> --- a/drivers/char/hw_random/timeriomem-rng.c
> +++ b/drivers/char/hw_random/timeriomem-rng.c

[ ... ]

> @@ -71,11 +76,28 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
>  			usleep_range(period_us,
>  					period_us + max(1, period_us / 100));
>  
> -		*(u32 *)data = readl(priv->io_base);
> -		retval += sizeof(u32);
> -		data += sizeof(u32);
> -		max -= sizeof(u32);
> -	} while (wait && max > sizeof(u32));
> +		switch (priv->width) {
> +		case 8: {
> +			u8 val = readb(priv->io_base) & priv->mask;
> +			*(u8 *)data = val;
> +			break;
> +		}

[ ... ]

> +		}
> +
> +		retval += chunk;
> +		data += chunk;
> +		max -= chunk;
> +	} while (wait && max > chunk);

[Severity: High]
This isn't a bug introduced by this patch, but does the current driver
teardown sequence allow a concurrent reader to cause a use-after-free
or deadlock?

Because the driver uses devm_hwrng_register(), the hwrng is unregistered by
devres after timeriomem_rng_remove() completes.

If timeriomem_rng_remove() calls hrtimer_cancel(&priv->timer) while a reader
is inside timeriomem_rng_read() (for example, sleeping between reads), the
devres cleanup will subsequently call hwrng_unregister() which blocks
waiting for the active reader to finish.

However, the reader will eventually finish its loop and unconditionally run
the following at the end of timeriomem_rng_read():

	hrtimer_forward_now(&priv->timer, priv->period);
	hrtimer_restart(&priv->timer);

Can this sequence restart the timer after it was cancelled by the remove
function, allowing the timer to eventually fire and dereference the freed
priv pointer?

Additionally, if a reader is waiting in wait_for_completion(&priv->completion)
when the timer is cancelled, could this prevent the timer from firing,
leaving the reader hanging in TASK_UNINTERRUPTIBLE and causing the
subsequent hwrng_unregister() to deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615201339.1264676-1-inasj268@gmail.com?part=1