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

[Lothar Rubusch]

Hi Thorsten, thanks for the feedback. Pls, find my comment down below.

On Mon, Jun 1, 2026 at 11:42 PM Thorsten Blum <thorsten.blum@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > ---
> >  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.

Argh... I see your point. The "ready" now is transparent to the
i2c_client_list usage and serves for nothing, that's nonsense. Going
some overengineering-steps back, my original idea (to satisfy a
sashiko complaint), in my own words:

Thread A:
1. probe()
  V
2. probe(): add i2c_priv to i2c_client_list <-------------- Thread B requests
  V
3. probe(): registers kpp
  V
4. probe(): say, register kpp fails
  V
5. probe(): remove i2c_priv from i2c_client_list <-----

Thread B:
Now if a crypto request/TFM comes in (thread B) and requests a client
from the i2c_client_list.
(Note, this is a case where the device must be, say, the second such
device so that kpp is already registered for any atmel driver).

If this happens before step 2 or after step 5, it's fine. This
instance is still not on the list. If it happens at step 2 through
step 4 this is problematic. A i2c_priv could be returned which is
actually (still) not ready. In the meanwhile i2c_priv will be removed,
but the TFM continues refering to this instance.

Question:
- Do you see the issue here, too? Or, is my understanding wrong? Can
this be problematic / lead to UAF?

- If true, my first idea was to set a "ready" state initially to
false, after kpp registered successfully, set it to true. The flag is
checked then, as in this patch. Then I probably messed it up. So,
could this approach solve the situation?

If you not answer I'll present this in the next days.

Best,
L

>
> Thanks,
> Thorsten