public inbox for linux-arm-kernel@lists.infradead.org 
 help / color / mirror / Atom feed

* [PATCH 1/1] crypto: atmel-ecc - fix use after free situation
@ 2026-05-29  9:27 Lothar Rubusch
  2026-06-01 21:42 ` Thorsten Blum
  0 siblings, 1 reply; 3+ messages in thread
From: Lothar Rubusch @ 2026-05-29  9:27 UTC (permalink / raw)
  To: thorsten.blum, herbert, davem, nicolas.ferre, alexandre.belloni,
	claudiu.beznea, tudor.ambarus, krzk+dt
  Cc: linux-crypto, linux-arm-kernel, linux-kernel, l.rubusch

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;
 };
 
 /**

base-commit: 5624ea54f3ba5c83d2e5503411a31a8be0278c1e
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 3+ messages in thread

• * Re: [PATCH 1/1] crypto: atmel-ecc - fix use after free situation
    2026-05-29  9:27 [PATCH 1/1] crypto: atmel-ecc - fix use after free situation Lothar Rubusch
  @ 2026-06-01 21:42 ` Thorsten Blum
    2026-06-04  7:56   ` Lothar Rubusch
    0 siblings, 1 reply; 3+ messages in thread
  From: Thorsten Blum @ 2026-06-01 21:42 UTC (permalink / raw)
    To: Lothar Rubusch
    Cc: herbert, davem, nicolas.ferre, alexandre.belloni, claudiu.beznea,
  	tudor.ambarus, krzk+dt, linux-crypto, linux-arm-kernel,
  	linux-kernel
  
  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
  
  
  ^ permalink raw reply	[flat|nested] 3+ messages in thread

• • * Re: [PATCH 1/1] crypto: atmel-ecc - fix use after free situation
      2026-06-01 21:42 ` Thorsten Blum
    @ 2026-06-04  7:56   ` Lothar Rubusch
      0 siblings, 0 replies; 3+ messages in thread
    From: Lothar Rubusch @ 2026-06-04  7:56 UTC (permalink / raw)
      To: Thorsten Blum
      Cc: herbert, davem, nicolas.ferre, alexandre.belloni, claudiu.beznea,
    	tudor.ambarus, krzk+dt, linux-crypto, linux-arm-kernel,
    	linux-kernel
    
    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@linux•dev> 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@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.
    
    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
    
    
    ^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-06-04  7:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-29  9:27 [PATCH 1/1] crypto: atmel-ecc - fix use after free situation Lothar Rubusch
2026-06-01 21:42 ` Thorsten Blum
2026-06-04  7:56   ` Lothar Rubusch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox