Re: [PATCH 1/1] crypto: atmel-sha204a - fix heap info leak on I2C transfer failure

[Thorsten Blum]

On Fri, May 29, 2026 at 09:43:36AM +0000, Lothar Rubusch wrote:
> When a non-blocking read operation is requested, the driver dynamically
> allocates memory to track asynchronous transfer status. If the underlying
> I2C transmission fails, atmel_sha204a_rng_done() logs a rate-limited
> warning but incorrectly proceeds to cache the pointer to this uninitialized
> buffer inside the rng->priv data field anyway.

The buffer is not necessarily uninitialized. cmd.data could also contain
a device error/status response or stale data from a previous request. An
uninitialized buffer is only one possibility.

Also, "rng->priv data field" is a bit confusing; maybe say that the
callback caches the work_data pointer in rng->priv instead?

> On subsequent execution passes, atmel_sha204a_rng_read_nonblocking()
> detects the stale rng->priv value, skips executing a hardware data read,

The cache-hit path still queues a new async read request and copies
invalid/stale data, but "skips a hardware data read" is probably not
correct. Did you mean the returned bytes aren't from a successful read?

> and copies up to 32 bytes of uninitialized kernel heap data from this
> garbage memory pool straight back into the system's hwrng data stream.

Same as above: it's not necessarily garbage or uninitialized data.

> Fix this information disclosure vector by immediately releasing the
> allocated asynchronous work data buffer and explicitly clearing the
> tracking pointer context whenever an I2C transaction returns a non-zero
> error status.
> 
> Additionally, duplicate the tfm counter decrement within the new error
> path to ensure the reference counter is properly released before executing
> the early return, maintaining the driver's availability for subsequent
> requests.
> 
> Fixes: da001fb651b0 ("crypto: atmel-i2c - add support for SHA204A random number generator")
> Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> ---
>  drivers/crypto/atmel-sha204a.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/atmel-sha204a.c b/drivers/crypto/atmel-sha204a.c
> index 4c9af737b33a..20cd915ea8a3 100644
> --- a/drivers/crypto/atmel-sha204a.c
> +++ b/drivers/crypto/atmel-sha204a.c
> @@ -31,10 +31,15 @@ static void atmel_sha204a_rng_done(struct atmel_i2c_work_data *work_data,
>  	struct atmel_i2c_client_priv *i2c_priv = work_data->ctx;
>  	struct hwrng *rng = areq;
>  
> -	if (status)
> +	if (status) {
>  		dev_warn_ratelimited(&i2c_priv->client->dev,
>  				     "i2c transaction failed (%d)\n",
>  				     status);
> +		kfree(work_data);
> +		rng->priv = 0;

Setting rng->priv = 0 is redundant here. rng_read_nonblocking() already
clears it before enqueuing new work, and the ->tfm_count gate prevents
others from setting it.

> +		atomic_dec(&i2c_priv->tfm_count);
> +		return;
> +	}
>  
>  	rng->priv = (unsigned long)work_data;
>  	atomic_dec(&i2c_priv->tfm_count);
> 
> base-commit: 5624ea54f3ba5c83d2e5503411a31a8be0278c1e

The fix itself looks good to me.

For a v2, could you please reword the commit message to be more precise,
and ideally drop the redundant rng->priv assignment? And feel free to
add a stable tag, as this should probably be backported.

With the above addressed, feel free to add:

Reviewed-by: Thorsten Blum <thorsten.blum@xxxxxxxxx>

Thanks,
Thorsten