It seems that tipc_crypto_key_revoke() could be be invoked by
wokequeue tipc_crypto_work_rx() under process context and
timer/rx callback under softirq context, thus the lock acquisition
on &tx->lock seems better use spin_lock_bh() to prevent possible
deadlock.
This flaw was found by an experimental static analysis tool I am
developing for irq-related deadlock.
tipc_crypto_work_rx() <workqueue>
--> tipc_crypto_key_distr()
--> tipc_bcast_xmit()
--> tipc_bcbase_xmit()
--> tipc_bearer_bc_xmit()
--> tipc_crypto_xmit()
--> tipc_ehdr_build()
--> tipc_crypto_key_revoke()
--> spin_lock(&tx->lock)
<timer interrupt>
--> tipc_disc_timeout()
--> tipc_bearer_xmit_skb()
--> tipc_crypto_xmit()
--> tipc_ehdr_build()
--> tipc_crypto_key_revoke()
--> spin_lock(&tx->lock) <deadlock here>
Signed-off-by: Chengfeng Ye <[email protected]>
---
net/tipc/crypto.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 302fd749c424..43c3f1c971b8 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -1441,14 +1441,14 @@ static int tipc_crypto_key_revoke(struct net *net, u8 tx_key)
struct tipc_crypto *tx = tipc_net(net)->crypto_tx;
struct tipc_key key;
- spin_lock(&tx->lock);
+ spin_lock_bh(&tx->lock);
key = tx->key;
WARN_ON(!key.active || tx_key != key.active);
/* Free the active key */
tipc_crypto_key_set_state(tx, key.passive, 0, key.pending);
tipc_crypto_key_detach(tx->aead[key.active], &tx->lock);
- spin_unlock(&tx->lock);
+ spin_unlock_bh(&tx->lock);
pr_warn("%s: key is revoked\n", tx->name);
return -EKEYREVOKED;
--
2.17.1
On 9/27/2023 11:14 AM, Chengfeng Ye wrote:
> It seems that tipc_crypto_key_revoke() could be be invoked by
> wokequeue tipc_crypto_work_rx() under process context and
> timer/rx callback under softirq context, thus the lock acquisition
> on &tx->lock seems better use spin_lock_bh() to prevent possible
> deadlock.
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
>
Interesting.
> tipc_crypto_work_rx() <workqueue>
> --> tipc_crypto_key_distr()
> --> tipc_bcast_xmit()
> --> tipc_bcbase_xmit()
> --> tipc_bearer_bc_xmit()
> --> tipc_crypto_xmit()
> --> tipc_ehdr_build()
> --> tipc_crypto_key_revoke()
> --> spin_lock(&tx->lock)
> <timer interrupt>
> --> tipc_disc_timeout()
> --> tipc_bearer_xmit_skb()
> --> tipc_crypto_xmit()
> --> tipc_ehdr_build()
> --> tipc_crypto_key_revoke()
> --> spin_lock(&tx->lock) <deadlock here>
>
> Signed-off-by: Chengfeng Ye <[email protected]>
> ---
Reviewed-by: Jacob Keller <[email protected]>
> net/tipc/crypto.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index 302fd749c424..43c3f1c971b8 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -1441,14 +1441,14 @@ static int tipc_crypto_key_revoke(struct net *net, u8 tx_key)
> struct tipc_crypto *tx = tipc_net(net)->crypto_tx;
> struct tipc_key key;
>
> - spin_lock(&tx->lock);
> + spin_lock_bh(&tx->lock);
> key = tx->key;
> WARN_ON(!key.active || tx_key != key.active);
>
> /* Free the active key */
> tipc_crypto_key_set_state(tx, key.passive, 0, key.pending);
> tipc_crypto_key_detach(tx->aead[key.active], &tx->lock);
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
>
> pr_warn("%s: key is revoked\n", tx->name);
> return -EKEYREVOKED;
On Wed, Sep 27, 2023 at 06:14:14PM +0000, Chengfeng Ye wrote:
> It seems that tipc_crypto_key_revoke() could be be invoked by
> wokequeue tipc_crypto_work_rx() under process context and
> timer/rx callback under softirq context, thus the lock acquisition
> on &tx->lock seems better use spin_lock_bh() to prevent possible
> deadlock.
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
>
> tipc_crypto_work_rx() <workqueue>
> --> tipc_crypto_key_distr()
> --> tipc_bcast_xmit()
> --> tipc_bcbase_xmit()
> --> tipc_bearer_bc_xmit()
> --> tipc_crypto_xmit()
> --> tipc_ehdr_build()
> --> tipc_crypto_key_revoke()
> --> spin_lock(&tx->lock)
> <timer interrupt>
> --> tipc_disc_timeout()
> --> tipc_bearer_xmit_skb()
> --> tipc_crypto_xmit()
> --> tipc_ehdr_build()
> --> tipc_crypto_key_revoke()
> --> spin_lock(&tx->lock) <deadlock here>
>
> Signed-off-by: Chengfeng Ye <[email protected]>
Hi Chengfeng Ye,
thanks for your patch.
As a fix for Networking this should probably be targeted at the
'net' tree. Which should be denoted in the subject.
Subject: [PATCH net] ...
And as a fix this patch should probably have a Fixes tag.
This ones seem appropriate to me, but I could be wrong.
Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
I don't think it is necessary to repost just to address these issues,
but the Networking maintainers may think otherwise.
The code change itself looks good to me.
On 2023-09-27 14:14, Chengfeng Ye wrote:
> It seems that tipc_crypto_key_revoke() could be be invoked by
> wokequeue tipc_crypto_work_rx() under process context and
> timer/rx callback under softirq context, thus the lock acquisition
> on &tx->lock seems better use spin_lock_bh() to prevent possible
> deadlock.
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
>
> tipc_crypto_work_rx() <workqueue>
> --> tipc_crypto_key_distr()
> --> tipc_bcast_xmit()
> --> tipc_bcbase_xmit()
> --> tipc_bearer_bc_xmit()
> --> tipc_crypto_xmit()
> --> tipc_ehdr_build()
> --> tipc_crypto_key_revoke()
> --> spin_lock(&tx->lock)
> <timer interrupt>
> --> tipc_disc_timeout()
> --> tipc_bearer_xmit_skb()
> --> tipc_crypto_xmit()
> --> tipc_ehdr_build()
> --> tipc_crypto_key_revoke()
> --> spin_lock(&tx->lock) <deadlock here>
>
> Signed-off-by: Chengfeng Ye <[email protected]>
> ---
> net/tipc/crypto.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index 302fd749c424..43c3f1c971b8 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -1441,14 +1441,14 @@ static int tipc_crypto_key_revoke(struct net *net, u8 tx_key)
> struct tipc_crypto *tx = tipc_net(net)->crypto_tx;
> struct tipc_key key;
>
> - spin_lock(&tx->lock);
> + spin_lock_bh(&tx->lock);
> key = tx->key;
> WARN_ON(!key.active || tx_key != key.active);
>
> /* Free the active key */
> tipc_crypto_key_set_state(tx, key.passive, 0, key.pending);
> tipc_crypto_key_detach(tx->aead[key.active], &tx->lock);
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
>
> pr_warn("%s: key is revoked\n", tx->name);
> return -EKEYREVOKED;
Acked-by: Jon Maloy <[email protected]>
Thanks much for your review.
Regards,
Chengfeng
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
On Wed, 27 Sep 2023 18:14:14 +0000 you wrote:
> It seems that tipc_crypto_key_revoke() could be be invoked by
> wokequeue tipc_crypto_work_rx() under process context and
> timer/rx callback under softirq context, thus the lock acquisition
> on &tx->lock seems better use spin_lock_bh() to prevent possible
> deadlock.
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
>
> [...]
Here is the summary with links:
- tipc: fix a potential deadlock on &tx->lock
https://git.kernel.org/netdev/net/c/08e50cf07184
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html