Re: [PATCH v5 2/2] 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] Race condition and broken teardown sequence between module unbind and active hwrng readers causes Deadlocks or Use-After-Free of the private data.
--

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

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

This commit implements configurable read sizes and data masking for the
timeriomem-rng driver. It adds support for 1, 2, or 4-byte reads and an
optional bitmask to filter output entropy, configurable via platform data
or device tree.

> diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
> index e61f063932090..42393409f22ad 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->reg_io_width) {
> +		case 1: {
> +			u8 val = readb(priv->io_base) & priv->mask;
> +			*(u8 *)data = val;
> +			break;
> +		}
> +		case 2: {
> +			u16 val = readw(priv->io_base) & priv->mask;
> +			*(u16 *)data = val;
> +			break;
> +		}
> +		case 4: {
> +			u32 val = readl(priv->io_base) & priv->mask;
> +			*(u32 *)data = val;
> +			break;
> +		}
> +		}
> +
> +		retval += chunk;
> +		data += chunk;
> +		max -= chunk;
> +	} while (wait && max > chunk);
>  
>  	/*
>  	 * Block any new callers until the RNG has had time to generate new

[Severity: High]
This is a pre-existing issue, but can this sequence lead to deadlocks or a 
use-after-free during module teardown?

Because the driver uses devm_hwrng_register(), devres unregisters the RNG
after the driver's remove function returns. The timeriomem_rng_remove function
explicitly calls hrtimer_cancel(&priv->timer), but concurrent readers might
still be active in timeriomem_rng_read.

If a reader is waiting earlier in this function at
wait_for_completion(&priv->completion) when the timer is cancelled, won't it
hang forever since the timer will never fire to wake it up?

Alternatively, if the reader is already past the wait, it will unconditionally
call hrtimer_restart(&priv->timer) slightly further down at the end of this
function. Doesn't this re-arm the timer after it was cancelled by the removal
thread?

If the timer is re-armed and devres subsequently frees the
timeriomem_rng_private data, won't the timer eventually fire and access freed
memory?

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