Re: [PATCH 1/1] crypto: atmel-ecc - fix use after free situation

[Thorsten Blum <thorsten.blum@linux.dev>]

On Fri, May 29, 2026 at 09:27:03AM +0000, Lothar Rubusch wrote:
> Fixes a possible race condition, when having multiple of such devices
> attached (identified by sashiko feedback).
> 
> The Scenario:
>     Thread A (Device 1 Probe): Successfully adds i2c_priv to the global
>              list (Line 324). The lock is released.
>     Thread B (An active crypto request): Concurrently calls
>               atmel_ecc_i2c_client_alloc(). It scans the global list, sees
>               Device 1, and assigns a crypto job to it.
>     Thread A: Moves to line 332. crypto_register_kpp() fails (e.g., out of
>               memory or name clash).
>     Thread A: Enters the error path. It removes Device 1 from the list and
>               frees the i2c_priv memory.
>     Thread B: Is still actively trying to talk to the I2C hardware using
>               the i2c_priv pointer it grabbed in Step 2. The memory is now
>               gone. Result: Kernel crash (Use-After-Free).
> 
> Fixes: 11105693fa05 ("crypto: atmel-ecc - introduce Microchip / Atmel ECC driver")
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/crypto/atmel-ecc.c | 10 ++++++++++
>  drivers/crypto/atmel-i2c.h |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index 0ca02995a1de..d391fe1462f6 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -218,6 +218,8 @@ static struct i2c_client *atmel_ecc_i2c_client_alloc(void)
>  
>  	list_for_each_entry(i2c_priv, &driver_data.i2c_client_list,
>  			    i2c_client_list_node) {
> +		if (!i2c_priv->ready)
> +			continue;
>  		tfm_cnt = atomic_read(&i2c_priv->tfm_count);
>  		if (tfm_cnt < min_tfm_cnt) {
>  			min_tfm_cnt = tfm_cnt;
> @@ -322,20 +324,24 @@ static int atmel_ecc_probe(struct i2c_client *client)
>  		return ret;
>  
>  	i2c_priv = i2c_get_clientdata(client);
> +	i2c_priv->ready = false;
>  
>  	spin_lock(&driver_data.i2c_list_lock);
>  	list_add_tail(&i2c_priv->i2c_client_list_node,
>  		      &driver_data.i2c_client_list);
> +	i2c_priv->ready = true;
>  	spin_unlock(&driver_data.i2c_list_lock);
>  
>  	ret = crypto_register_kpp(&atmel_ecdh_nist_p256);
>  	if (ret) {
>  		spin_lock(&driver_data.i2c_list_lock);
> +		i2c_priv->ready = false;
>  		list_del(&i2c_priv->i2c_client_list_node);
>  		spin_unlock(&driver_data.i2c_list_lock);
>  
>  		dev_err(&client->dev, "%s alg registration failed\n",
>  			atmel_ecdh_nist_p256.base.cra_driver_name);
> +		return ret;
>  	} else {
>  		dev_info(&client->dev, "atmel ecc algorithms registered in /proc/crypto\n");
>  	}
> @@ -347,6 +353,10 @@ static void atmel_ecc_remove(struct i2c_client *client)
>  {
>  	struct atmel_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);
>  
> +	spin_lock(&driver_data.i2c_list_lock);
> +	i2c_priv->ready = false;
> +	spin_unlock(&driver_data.i2c_list_lock);
> +
>  	/* Return EBUSY if i2c client already allocated. */
>  	if (atomic_read(&i2c_priv->tfm_count)) {
>  		/*
> diff --git a/drivers/crypto/atmel-i2c.h b/drivers/crypto/atmel-i2c.h
> index 72f04c15682f..e3b12030f9c4 100644
> --- a/drivers/crypto/atmel-i2c.h
> +++ b/drivers/crypto/atmel-i2c.h
> @@ -129,6 +129,7 @@ struct atmel_ecc_driver_data {
>   * @wake_token_sz       : size in bytes of the wake_token
>   * @tfm_count           : number of active crypto transformations on i2c client
>   * @hwrng               : hold the hardware generated rng
> + * @ready               : hw client is ready to use
>   *
>   * Reads and writes from/to the i2c client are sequential. The first byte
>   * transmitted to the device is treated as the byte size. Any attempt to send
> @@ -145,6 +146,7 @@ struct atmel_i2c_client_priv {
>  	size_t wake_token_sz;
>  	atomic_t tfm_count ____cacheline_aligned;
>  	struct hwrng hwrng;
> +	bool ready;
>  };

I don't think the ready flag fixes the race. A concurrent tfm can still
bind to the shared I2C client after atmel_ecc_probe() adds it to the
global list and marks it as ready, but before crypto_register_kpp()
fails.

Thanks,
Thorsten