Re: [PATCH v3 1/3] crypto: atmel-sha204a - fix memory leak at non-blocking RNG work_data

["Ard Biesheuvel" <ardb@kernel.org>]

Hi Lothar,

On Wed, 22 Apr 2026, at 23:09, Lothar Rubusch wrote:
> The driver allocated memory for work_data in the non-blocking read
> path but never free'd it again. After first read-out the memory pointer
> seemed to be recycled and never was allocated again, due to some errors
> in the logic, so that the leak was not growing.
>

Why can't we just reuse the work_data, instead of alloc/freeing it every time?

> Add kfree(work_data) in the completion callback on error. then add
> kfree(work_data) after the data is consumed in the subsequent read
> call. Finally ensure atomic_dec() is called only after the data has
> been consumed or an error occurred to prevent race conditions.
>
> Fixes: da001fb651b0 ("crypto: atmel-i2c - add support for SHA204A 
> random number generator")
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/crypto/atmel-sha204a.c | 43 ++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/crypto/atmel-sha204a.c 
> b/drivers/crypto/atmel-sha204a.c
> index dbb39ed0cea1..19720bdd446d 100644
> --- a/drivers/crypto/atmel-sha204a.c
> +++ b/drivers/crypto/atmel-sha204a.c
> @@ -25,13 +25,17 @@ 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;
> +		atomic_dec(&i2c_priv->tfm_count);
> +		return;
> +	}
> 
>  	rng->priv = (unsigned long)work_data;
> -	atomic_dec(&i2c_priv->tfm_count);
>  }
> 
>  static int atmel_sha204a_rng_read_nonblocking(struct hwrng *rng, void 
> *data,
> @@ -42,31 +46,36 @@ static int 
> atmel_sha204a_rng_read_nonblocking(struct hwrng *rng, void *data,
> 
>  	i2c_priv = container_of(rng, struct atmel_i2c_client_priv, hwrng);
> 
> -	/* keep maximum 1 asynchronous read in flight at any time */
> -	if (!atomic_add_unless(&i2c_priv->tfm_count, 1, 1))
> -		return 0;
> -
> +	/* Verify if data available from last run */
>  	if (rng->priv) {
>  		work_data = (struct atmel_i2c_work_data *)rng->priv;
>  		max = min(sizeof(work_data->cmd.data), max);
>  		memcpy(data, &work_data->cmd.data, max);
> -		rng->priv = 0;
> -	} else {
> -		work_data = kmalloc_obj(*work_data, GFP_ATOMIC);
> -		if (!work_data) {
> -			atomic_dec(&i2c_priv->tfm_count);
> -			return -ENOMEM;
> -		}
> -		work_data->ctx = i2c_priv;
> -		work_data->client = i2c_priv->client;
> 
> -		max = 0;
> +		/* Now, free memory */
> +		kfree(work_data);
> +		rng->priv = 0;
> +		atomic_dec(&i2c_priv->tfm_count);
> +		return max;
>  	}
> 
> +	/* When a request is still in-flight but not processed */
> +	if (atomic_read(&i2c_priv->tfm_count) > 0)
> +		return 0;
> +
> +	/* Start a new request */
> +	work_data = kmalloc_obj(*work_data, GFP_ATOMIC);
> +	if (!work_data)
> +		return -ENOMEM;
> +
> +	atomic_inc(&i2c_priv->tfm_count);
> +	work_data->ctx = i2c_priv;
> +	work_data->client = i2c_priv->client;
> +
>  	atmel_i2c_init_random_cmd(&work_data->cmd);
>  	atmel_i2c_enqueue(work_data, atmel_sha204a_rng_done, rng);
> 
> -	return max;
> +	return 0;
>  }
> 
>  static int atmel_sha204a_rng_read(struct hwrng *rng, void *data, size_t max,
> -- 
> 2.53.0