[PATCH net v2] tipc: protect received keys from concurrent flush

[PATCH net v2] tipc: protect received keys from concurrent flush

[Jérémy Jean]

tipc_crypto_key_synch() can queue the RX worker again while it is
still using the session key rx->skey. If tipc_crypto_key_flush()
cancels that queued work, it frees that key without waiting for the
running worker. The worker can then read freed memory or free the key
a second time.

Track the worker session key rx->skey under rx->lock and clear it on
flush. Check for a flush under the same lock before attaching or
retrying the key. Let the worker free its own key without touching a
replacement.

Fixes: 1ef6f7c9390f ("tipc: add automatic session key exchange")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---

Changes in v2 to address a review by Sashiko:
* Ensure flush revokes received keys.
* Preserve keys received after a flush.

v1: https://lore.kernel.org/all/20260930115708.349540-2-Jeremy.Jean@oss.cyber.gouv.fr/

 net/tipc/crypto.c | 65 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 16f1ed1f6b1b..a315ff3d7f80 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -184,6 +184,7 @@ struct tipc_crypto_stats {
  * @key: the key states
  * @skey_mode: session key's mode
  * @skey: received session key
+ * @skey_in_use: received session key owned by the RX worker
  * @wq: common workqueue on TX crypto
  * @work: delayed work sched for TX/RX
  * @key_distr: key distributing state
@@ -209,6 +210,7 @@ struct tipc_crypto {
 	struct tipc_key key;
 	u8 skey_mode;
 	struct tipc_aead_key *skey;
+	struct tipc_aead_key *skey_in_use;
 	struct workqueue_struct *wq;
 	struct delayed_work work;
 #define KEY_DISTR_SCHED		1
@@ -1136,7 +1138,12 @@ int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
 
 	/* Attach it to the crypto */
 	if (likely(!rc)) {
-		rc = tipc_crypto_key_attach(c, aead, 0, master_key);
+		spin_lock_bh(&c->lock);
+		if (ukey == c->skey_in_use && c->skey != ukey)
+			rc = -ECANCELED;
+		else
+			rc = tipc_crypto_key_attach(c, aead, 0, master_key);
+		spin_unlock_bh(&c->lock);
 		if (rc < 0)
 			tipc_aead_free(&aead->rcu);
 	}
@@ -1151,6 +1158,8 @@ int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
  * @pos: desired slot in the crypto key array, = 0 if any!
  * @master_key: specify this is a cluster master key
  *
+ * The caller must hold c->lock.
+ *
  * Return: new key id in case of success, otherwise: -EBUSY
  */
 static int tipc_crypto_key_attach(struct tipc_crypto *c,
@@ -1158,20 +1167,19 @@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
 				  bool master_key)
 {
 	struct tipc_key key;
-	int rc = -EBUSY;
 	u8 new_key;
 
-	spin_lock_bh(&c->lock);
+	lockdep_assert_held(&c->lock);
 	key = c->key;
 	if (master_key) {
 		new_key = KEY_MASTER;
 		goto attach;
 	}
 	if (key.active && key.passive)
-		goto exit;
+		return -EBUSY;
 	if (key.pending) {
 		if (tipc_aead_users(c->aead[key.pending]) > 0)
-			goto exit;
+			return -EBUSY;
 		/* if (pos): ok with replacing, will be aligned when needed */
 		/* Replace it */
 		new_key = key.pending;
@@ -1201,11 +1209,7 @@ attach:
 	c->working = 1;
 	c->nokey = 0;
 	c->key_master |= master_key;
-	rc = new_key;
-
-exit:
-	spin_unlock_bh(&c->lock);
-	return rc;
+	return new_key;
 }
 
 void tipc_crypto_key_flush(struct tipc_crypto *c)
@@ -1219,11 +1223,13 @@ void tipc_crypto_key_flush(struct tipc_crypto *c)
 		rx = c;
 		tx = tipc_net(rx->net)->crypto_tx;
 		if (cancel_delayed_work(&rx->work)) {
-			kfree_sensitive(rx->skey);
-			rx->skey = NULL;
 			atomic_xchg(&rx->key_distr, 0);
 			tipc_node_put(rx->node);
 		}
+		/* Leave an in-flight key to its worker, but revoke it now. */
+		if (rx->skey != rx->skey_in_use)
+			kfree_sensitive(rx->skey);
+		rx->skey = NULL;
 		/* RX stopping => decrease TX key users if any */
 		k = atomic_xchg(&rx->peer_rx_active, 0);
 		if (k) {
@@ -1940,10 +1946,13 @@ static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 		if (tipc_aead_clone(&tmp, aead) < 0)
 			goto rcv;
 		WARN_ON(!refcount_inc_not_zero(&tmp->refcnt));
+		spin_lock_bh(&rx->lock);
 		if (tipc_crypto_key_attach(rx, tmp, ehdr->tx_key, false) < 0) {
+			spin_unlock_bh(&rx->lock);
 			tipc_aead_free(&tmp->rcu);
 			goto rcv;
 		}
+		spin_unlock_bh(&rx->lock);
 		tipc_aead_put(aead);
 		aead = tmp;
 	}
@@ -2381,27 +2390,35 @@ static void tipc_crypto_work_rx(struct work_struct *work)
 	}
 
 	/* Case 2: Attach a pending received session key from peer if any */
+	spin_lock_bh(&rx->lock);
 	if (rx->skey) {
-		rc = tipc_crypto_key_init(rx, rx->skey, rx->skey_mode, false);
-		if (unlikely(rc < 0))
+		rx->skey_in_use = rx->skey;
+		spin_unlock_bh(&rx->lock);
+		rc = tipc_crypto_key_init(rx, rx->skey_in_use,
+					  READ_ONCE(rx->skey_mode), false);
+		if (unlikely(rc < 0 && rc != -ECANCELED))
 			pr_warn("%s: unable to attach received skey, err %d\n",
 				rx->name, rc);
-		switch (rc) {
-		case -EBUSY:
-		case -ENOMEM:
+		if (rc != -EBUSY && rc != -ENOMEM)
+			synchronize_rcu();
+		spin_lock_bh(&rx->lock);
+		if (rx->skey == rx->skey_in_use &&
+		    (rc == -EBUSY || rc == -ENOMEM)) {
 			/* Resched the key attaching */
 			resched = true;
-			break;
-		default:
-			synchronize_rcu();
-			kfree_sensitive(rx->skey);
-			rx->skey = NULL;
-			break;
+		} else {
+			if (rx->skey == rx->skey_in_use)
+				rx->skey = NULL;
+			kfree_sensitive(rx->skey_in_use);
 		}
+		rx->skey_in_use = NULL;
 	}
 
-	if (resched && queue_delayed_work(tx->wq, &rx->work, delay))
+	if (resched && queue_delayed_work(tx->wq, &rx->work, delay)) {
+		spin_unlock_bh(&rx->lock);
 		return;
+	}
+	spin_unlock_bh(&rx->lock);
 
 	tipc_node_put(rx->node);
 }

base-commit: 551c722f40809618230001baccf219193e22fc5a
-- 
2.47.3

Re: [PATCH net v2] tipc: protect received keys from concurrent flush

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

RE: [PATCH net v2] tipc: protect received keys from concurrent flush

[Tung Quang Nguyen]

>Subject: [PATCH net v2] tipc: protect received keys from concurrent flush
>
>tipc_crypto_key_synch() can queue the RX worker again while it is still using
>the session key rx->skey. If tipc_crypto_key_flush() cancels that queued work,
>it frees that key without waiting for the running worker. The worker can then
>read freed memory or free the key a second time.
>
>Track the worker session key rx->skey under rx->lock and clear it on flush.
>Check for a flush under the same lock before attaching or retrying the key. Let
>the worker free its own key without touching a replacement.
>
>Fixes: 1ef6f7c9390f ("tipc: add automatic session key exchange")
>Cc: stable@vger.kernel.org
>Assisted-by: LLM
>Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
>---
>
>Changes in v2 to address a review by Sashiko:
>* Ensure flush revokes received keys.
>* Preserve keys received after a flush.
>
>v1: https://lore.kernel.org/all/20260930115708.349540-2-
>Jeremy.Jean@oss.cyber.gouv.fr/
>
> net/tipc/crypto.c | 65 ++++++++++++++++++++++++++++++-----------------
> 1 file changed, 41 insertions(+), 24 deletions(-)
>
>diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c index
>16f1ed1f6b1b..a315ff3d7f80 100644
>--- a/net/tipc/crypto.c
>+++ b/net/tipc/crypto.c
>@@ -184,6 +184,7 @@ struct tipc_crypto_stats {
>  * @key: the key states
>  * @skey_mode: session key's mode
>  * @skey: received session key
>+ * @skey_in_use: received session key owned by the RX worker
>  * @wq: common workqueue on TX crypto
>  * @work: delayed work sched for TX/RX
>  * @key_distr: key distributing state
>@@ -209,6 +210,7 @@ struct tipc_crypto {
> 	struct tipc_key key;
> 	u8 skey_mode;
> 	struct tipc_aead_key *skey;
>+	struct tipc_aead_key *skey_in_use;
> 	struct workqueue_struct *wq;
> 	struct delayed_work work;
> #define KEY_DISTR_SCHED		1
>@@ -1136,7 +1138,12 @@ int tipc_crypto_key_init(struct tipc_crypto *c, struct
>tipc_aead_key *ukey,
>
> 	/* Attach it to the crypto */
> 	if (likely(!rc)) {
>-		rc = tipc_crypto_key_attach(c, aead, 0, master_key);
>+		spin_lock_bh(&c->lock);
>+		if (ukey == c->skey_in_use && c->skey != ukey)
>+			rc = -ECANCELED;

This check should be done earlier, before calling tipc_ahead_init() ?

>+		else
>+			rc = tipc_crypto_key_attach(c, aead, 0, master_key);
>+		spin_unlock_bh(&c->lock);
> 		if (rc < 0)
> 			tipc_aead_free(&aead->rcu);
> 	}
>@@ -1151,6 +1158,8 @@ int tipc_crypto_key_init(struct tipc_crypto *c, struct
>tipc_aead_key *ukey,
>  * @pos: desired slot in the crypto key array, = 0 if any!
>  * @master_key: specify this is a cluster master key
>  *
>+ * The caller must hold c->lock.
>+ *
>  * Return: new key id in case of success, otherwise: -EBUSY
>  */
> static int tipc_crypto_key_attach(struct tipc_crypto *c, @@ -1158,20 +1167,19
>@@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
> 				  bool master_key)
> {
> 	struct tipc_key key;
>-	int rc = -EBUSY;
> 	u8 new_key;
>
>-	spin_lock_bh(&c->lock);
>+	lockdep_assert_held(&c->lock);
> 	key = c->key;
> 	if (master_key) {
> 		new_key = KEY_MASTER;
> 		goto attach;
> 	}
> 	if (key.active && key.passive)
>-		goto exit;
>+		return -EBUSY;
> 	if (key.pending) {
> 		if (tipc_aead_users(c->aead[key.pending]) > 0)
>-			goto exit;
>+			return -EBUSY;
> 		/* if (pos): ok with replacing, will be aligned when needed */
> 		/* Replace it */
> 		new_key = key.pending;
>@@ -1201,11 +1209,7 @@ attach:
> 	c->working = 1;
> 	c->nokey = 0;
> 	c->key_master |= master_key;
>-	rc = new_key;
>-
>-exit:
>-	spin_unlock_bh(&c->lock);
>-	return rc;
>+	return new_key;

Remove lock from tipc_crypto_key_attach() is enough. Why adding many irrelevant changes that do not seem necessary ?

> }
>
> void tipc_crypto_key_flush(struct tipc_crypto *c) @@ -1219,11 +1223,13 @@
>void tipc_crypto_key_flush(struct tipc_crypto *c)
> 		rx = c;
> 		tx = tipc_net(rx->net)->crypto_tx;
> 		if (cancel_delayed_work(&rx->work)) {
>-			kfree_sensitive(rx->skey);
>-			rx->skey = NULL;
> 			atomic_xchg(&rx->key_distr, 0);
> 			tipc_node_put(rx->node);
> 		}
>+		/* Leave an in-flight key to its worker, but revoke it now. */
>+		if (rx->skey != rx->skey_in_use)
>+			kfree_sensitive(rx->skey);
>+		rx->skey = NULL;
> 		/* RX stopping => decrease TX key users if any */
> 		k = atomic_xchg(&rx->peer_rx_active, 0);
> 		if (k) {
>@@ -1940,10 +1946,13 @@ static void tipc_crypto_rcv_complete(struct net
>*net, struct tipc_aead *aead,
> 		if (tipc_aead_clone(&tmp, aead) < 0)
> 			goto rcv;
> 		WARN_ON(!refcount_inc_not_zero(&tmp->refcnt));
>+		spin_lock_bh(&rx->lock);
> 		if (tipc_crypto_key_attach(rx, tmp, ehdr->tx_key, false) < 0) {
>+			spin_unlock_bh(&rx->lock);
> 			tipc_aead_free(&tmp->rcu);
> 			goto rcv;
> 		}
>+		spin_unlock_bh(&rx->lock);
> 		tipc_aead_put(aead);
> 		aead = tmp;
> 	}
>@@ -2381,27 +2390,35 @@ static void tipc_crypto_work_rx(struct work_struct
>*work)
> 	}
>
> 	/* Case 2: Attach a pending received session key from peer if any */
>+	spin_lock_bh(&rx->lock);
> 	if (rx->skey) {
>-		rc = tipc_crypto_key_init(rx, rx->skey, rx->skey_mode, false);
>-		if (unlikely(rc < 0))
>+		rx->skey_in_use = rx->skey;
>+		spin_unlock_bh(&rx->lock);
>+		rc = tipc_crypto_key_init(rx, rx->skey_in_use,
>+					  READ_ONCE(rx->skey_mode), false);
>+		if (unlikely(rc < 0 && rc != -ECANCELED))
> 			pr_warn("%s: unable to attach received skey, err
>%d\n",
> 				rx->name, rc);
>-		switch (rc) {
>-		case -EBUSY:
>-		case -ENOMEM:
>+		if (rc != -EBUSY && rc != -ENOMEM)
>+			synchronize_rcu();
>+		spin_lock_bh(&rx->lock);
>+		if (rx->skey == rx->skey_in_use &&
>+		    (rc == -EBUSY || rc == -ENOMEM)) {
> 			/* Resched the key attaching */
> 			resched = true;
>-			break;
>-		default:
>-			synchronize_rcu();
>-			kfree_sensitive(rx->skey);
>-			rx->skey = NULL;
>-			break;
>+		} else {
>+			if (rx->skey == rx->skey_in_use)
>+				rx->skey = NULL;
>+			kfree_sensitive(rx->skey_in_use);
> 		}
>+		rx->skey_in_use = NULL;
> 	}
>
>-	if (resched && queue_delayed_work(tx->wq, &rx->work, delay))
>+	if (resched && queue_delayed_work(tx->wq, &rx->work, delay)) {
>+		spin_unlock_bh(&rx->lock);
> 		return;
>+	}
>+	spin_unlock_bh(&rx->lock);
>
> 	tipc_node_put(rx->node);
> }
>
>base-commit: 551c722f40809618230001baccf219193e22fc5a
>--
>2.47.3

pw-bot: cr

Re: [PATCH net v2] tipc: protect received keys from concurrent flush

[Jérémy Jean]

Hello,

On 2026-10-05 04:14, Tung Quang Nguyen wrote:
>> 	/* Attach it to the crypto */
>> 	if (likely(!rc)) {
>> -		rc = tipc_crypto_key_attach(c, aead, 0, master_key);
>> +		spin_lock_bh(&c->lock);
>> +		if (ukey == c->skey_in_use && c->skey != ukey)
>> +			rc = -ECANCELED;
> 
> This check should be done earlier, before calling tipc_ahead_init() ?

Would moving the check before tipc_aead_init() be enough, given that a
flush can still happen while AEAD setup is running and before the key
is attached?

Indeed, the race goes something like:
1. Worker starts tipc_aead_init()
2. Flush clears rx->skey
3. Worker finishes AEAD setup

AFAIU, we still need the check under c->lock, immediately before
tipc_crypto_key_attach(), so that a revoked key cannot be published
after flush.

> 
>> +		else
>> +			rc = tipc_crypto_key_attach(c, aead, 0, master_key);
>> +		spin_unlock_bh(&c->lock);
>> 		if (rc < 0)
>> 			tipc_aead_free(&aead->rcu);
>> 	}
>> @@ -1151,6 +1158,8 @@ int tipc_crypto_key_init(struct tipc_crypto *c, 
>> struct
>> tipc_aead_key *ukey,
>>  * @pos: desired slot in the crypto key array, = 0 if any!
>>  * @master_key: specify this is a cluster master key
>>  *
>> + * The caller must hold c->lock.
>> + *
>>  * Return: new key id in case of success, otherwise: -EBUSY
>>  */
>> static int tipc_crypto_key_attach(struct tipc_crypto *c, @@ -1158,20 
>> +1167,19
>> @@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
>> 				  bool master_key)
>> {
>> 	struct tipc_key key;
>> -	int rc = -EBUSY;
>> 	u8 new_key;
>> 
>> -	spin_lock_bh(&c->lock);
>> +	lockdep_assert_held(&c->lock);
>> 	key = c->key;
>> 	if (master_key) {
>> 		new_key = KEY_MASTER;
>> 		goto attach;
>> 	}
>> 	if (key.active && key.passive)
>> -		goto exit;
>> +		return -EBUSY;
>> 	if (key.pending) {
>> 		if (tipc_aead_users(c->aead[key.pending]) > 0)
>> -			goto exit;
>> +			return -EBUSY;
>> 		/* if (pos): ok with replacing, will be aligned when needed */
>> 		/* Replace it */
>> 		new_key = key.pending;
>> @@ -1201,11 +1209,7 @@ attach:
>> 	c->working = 1;
>> 	c->nokey = 0;
>> 	c->key_master |= master_key;
>> -	rc = new_key;
>> -
>> -exit:
>> -	spin_unlock_bh(&c->lock);
>> -	return rc;
>> +	return new_key;
> 
> Remove lock from tipc_crypto_key_attach() is enough. Why adding many 
> irrelevant changes that do not seem necessary ?

Those extra changes in tipc_crypto_key_attach() are more cleanup than
anything else; I preferred the direct returns since the function no
longer owns the lock, but I can reduce the patch and keep the exit
label.

Thanks,
Jérémy