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

Ard Biesheuvel
2026-04-23T07:44:11Z

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