* [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