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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch introduces a new device tree binding schema file but leaves the old schema intact, causing validation conflicts.
- [Low] Using the modulo operator `%` on a `resource_size_t` variable causes an undefined reference link error on 32-bit platforms.

Pre-existing issues:
- [High] The driver cancels the timer in `remove()` but relies on `devm` cleanup to unregister the hwrng device, creating a teardown race condition that leads to use-after-free or deadlock.
--

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

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

The TODO for supporting read sizes other than 32 bits and masking has
been sitting in this driver since 2009.  Implement it.

> diff --git a/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml b/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml
> new file mode 100644
> index 0000000000000..0d8460e9f9163
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rng/timeriomem-rng.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Timer IOMEM Hardware Random Number Generator

[Severity: Medium]
Will this new device tree binding schema file conflict with the existing
schema?

The patch introduces this new schema but does not appear to replace or remove
the existing timeriomem_rng.yaml file. Since both schemas match the same
compatible = "timeriomem_rng" string and the old schema specifies
additionalProperties: false, any device tree node including the newly added
width or mask properties might pass this new schema but fail the old schema's
validation, which would break dt_binding_check.

> diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
> index e61f063932090..9308988521473 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;
> +		}
> +		case 16: {
> +			u16 val = readw(priv->io_base) & priv->mask;
> +			*(u16 *)data = val;
> +			break;
> +		}
> +		case 32: {
> +			u32 val = readl(priv->io_base) & priv->mask;
> +			*(u32 *)data = val;
> +			break;
> +		}
> +		}
> +
> +		retval += chunk;
> +		data += chunk;
> +		max -= chunk;
> +	} while (wait && max > chunk);

[Severity: High]
This is a pre-existing issue, but does the driver have a teardown race
condition between remove() and devres cleanup?

In timeriomem_rng_remove(), hrtimer_cancel() is explicitly called to stop the
timer. However, because the RNG device was registered with
devm_hwrng_register() during probe, it remains accessible to userspace and the
hwrng core until the devres cleanups run strictly after remove() returns.

If a concurrent read is blocked on wait_for_completion() when the timer is
cancelled, it could hang indefinitely, deadlocking the reader thread and the
subsequent hwrng_unregister() call in devres cleanup.

Alternatively, if a reader completes its wait and unconditionally calls
hrtimer_restart() in timeriomem_rng_read() after the cancellation, the timer
might be re-armed:

drivers/char/hw_random/timeriomem-rng.c:timeriomem_rng_read() {
    ...
	priv->present = 0;
	reinit_completion(&priv->completion);
	hrtimer_forward_now(&priv->timer, priv->period);
	hrtimer_restart(&priv->timer);
    ...
}

When the devres cleanup later unregisters the RNG and frees the priv
structure, this re-armed timer will eventually fire, potentially dereferencing
a freed pointer.

[ ... ]
> @@ -145,9 +164,42 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
>  		if (!of_property_read_u32(pdev->dev.of_node,
>  						"quality", &i))
>  			priv->rng_ops.quality = i;
> +
> +		of_property_read_u32(pdev->dev.of_node,
> +				     "width", &priv->width);
> +		of_property_read_u32(pdev->dev.of_node,
> +				     "mask", &priv->mask);
>  	} else {
>  		period = pdata->period;
>  		priv->rng_ops.quality = pdata->quality;
> +
> +		if (pdata->width_set)
> +			priv->width = pdata->width;
> +		if (pdata->mask_set)
> +			priv->mask = pdata->mask;
> +	}
> +
> +	if (priv->width == 0)
> +		priv->width = 32;
> +
> +	switch (priv->width) {
> +	case 8:
> +	case 16:
> +	case 32:
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "invalid width %u, must be 8, 16, or 32\n",
> +			priv->width);
> +		return -EINVAL;
> +	}
> +
> +	if (res->start % (priv->width / 8) != 0 ||
> +	    resource_size(res) < priv->width / 8) {

[Severity: Low]
Does using the modulo operator here cause an undefined reference to __umoddi3
on 32-bit platforms?

The variable res->start is of type resource_size_t, which can be a 64-bit
integer on 32-bit platforms with PAE enabled (CONFIG_PHYS_ADDR_T_64BIT=y).

Since the divisor (priv->width / 8) is a runtime value rather than a
compile-time constant, the compiler might emit a call to __umoddi3 for the
64-bit division, which leads to a linker error on 32-bit architectures.
Could IS_ALIGNED() or div_u64() be used instead?

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