From: Jakub Kicinski <kuba@kernel•org>
To: davem@davemloft•net
Cc: netdev@vger•kernel.org, edumazet@google•com, pabeni@redhat•com,
sd@queasysnail•net, vadim.fedorenko@linux•dev,
Jakub Kicinski <kuba@kernel•org>, valis <sec@valis•email>,
borisp@nvidia•com, john.fastabend@gmail•com,
vinay.yadav@chelsio•com
Subject: [PATCH net 2/7] tls: fix race between async notify and socket close
Date: Tue, 6 Feb 2024 17:18:19 -0800 [thread overview]
Message-ID: <20240207011824.2609030-3-kuba@kernel.org> (raw)
In-Reply-To: <20240207011824.2609030-1-kuba@kernel.org>
The submitting thread (one which called recvmsg/sendmsg)
may exit as soon as the async crypto handler calls complete()
so any code past that point risks touching already freed data.
Try to avoid the locking and extra flags altogether.
Have the main thread hold an extra reference, this way
we can depend solely on the atomic ref counter for
synchronization.
Don't futz with reiniting the completion, either, we are now
tightly controlling when completion fires.
Reported-by: valis <sec@valis•email>
Fixes: 0cada33241d9 ("net/tls: fix race condition causing kernel panic")
Signed-off-by: Jakub Kicinski <kuba@kernel•org>
---
CC: borisp@nvidia•com
CC: john.fastabend@gmail•com
CC: vinay.yadav@chelsio•com
---
include/net/tls.h | 5 -----
net/tls/tls_sw.c | 43 ++++++++++---------------------------------
2 files changed, 10 insertions(+), 38 deletions(-)
diff --git a/include/net/tls.h b/include/net/tls.h
index 962f0c501111..340ad43971e4 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -97,9 +97,6 @@ struct tls_sw_context_tx {
struct tls_rec *open_rec;
struct list_head tx_list;
atomic_t encrypt_pending;
- /* protect crypto_wait with encrypt_pending */
- spinlock_t encrypt_compl_lock;
- int async_notify;
u8 async_capable:1;
#define BIT_TX_SCHEDULED 0
@@ -136,8 +133,6 @@ struct tls_sw_context_rx {
struct tls_strparser strp;
atomic_t decrypt_pending;
- /* protect crypto_wait with decrypt_pending*/
- spinlock_t decrypt_compl_lock;
struct sk_buff_head async_hold;
struct wait_queue_head wq;
};
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 6a73714f34cc..635305bebfef 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -224,22 +224,15 @@ static void tls_decrypt_done(void *data, int err)
kfree(aead_req);
- spin_lock_bh(&ctx->decrypt_compl_lock);
- if (!atomic_dec_return(&ctx->decrypt_pending))
+ if (atomic_dec_and_test(&ctx->decrypt_pending))
complete(&ctx->async_wait.completion);
- spin_unlock_bh(&ctx->decrypt_compl_lock);
}
static int tls_decrypt_async_wait(struct tls_sw_context_rx *ctx)
{
- int pending;
-
- spin_lock_bh(&ctx->decrypt_compl_lock);
- reinit_completion(&ctx->async_wait.completion);
- pending = atomic_read(&ctx->decrypt_pending);
- spin_unlock_bh(&ctx->decrypt_compl_lock);
- if (pending)
+ if (!atomic_dec_and_test(&ctx->decrypt_pending))
crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
+ atomic_inc(&ctx->decrypt_pending);
return ctx->async_wait.err;
}
@@ -267,6 +260,7 @@ static int tls_do_decryption(struct sock *sk,
aead_request_set_callback(aead_req,
CRYPTO_TFM_REQ_MAY_BACKLOG,
tls_decrypt_done, aead_req);
+ DEBUG_NET_WARN_ON_ONCE(atomic_read(&ctx->decrypt_pending) < 1);
atomic_inc(&ctx->decrypt_pending);
} else {
aead_request_set_callback(aead_req,
@@ -455,7 +449,6 @@ static void tls_encrypt_done(void *data, int err)
struct sk_msg *msg_en;
bool ready = false;
struct sock *sk;
- int pending;
msg_en = &rec->msg_encrypted;
@@ -494,12 +487,8 @@ static void tls_encrypt_done(void *data, int err)
ready = true;
}
- spin_lock_bh(&ctx->encrypt_compl_lock);
- pending = atomic_dec_return(&ctx->encrypt_pending);
-
- if (!pending && ctx->async_notify)
+ if (atomic_dec_and_test(&ctx->encrypt_pending))
complete(&ctx->async_wait.completion);
- spin_unlock_bh(&ctx->encrypt_compl_lock);
if (!ready)
return;
@@ -511,22 +500,9 @@ static void tls_encrypt_done(void *data, int err)
static int tls_encrypt_async_wait(struct tls_sw_context_tx *ctx)
{
- int pending;
-
- spin_lock_bh(&ctx->encrypt_compl_lock);
- ctx->async_notify = true;
-
- pending = atomic_read(&ctx->encrypt_pending);
- spin_unlock_bh(&ctx->encrypt_compl_lock);
- if (pending)
+ if (!atomic_dec_and_test(&ctx->encrypt_pending))
crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
- else
- reinit_completion(&ctx->async_wait.completion);
-
- /* There can be no concurrent accesses, since we have no
- * pending encrypt operations
- */
- WRITE_ONCE(ctx->async_notify, false);
+ atomic_inc(&ctx->encrypt_pending);
return ctx->async_wait.err;
}
@@ -577,6 +553,7 @@ static int tls_do_encryption(struct sock *sk,
/* Add the record in tx_list */
list_add_tail((struct list_head *)&rec->list, &ctx->tx_list);
+ DEBUG_NET_WARN_ON_ONCE(atomic_read(&ctx->encrypt_pending) < 1);
atomic_inc(&ctx->encrypt_pending);
rc = crypto_aead_encrypt(aead_req);
@@ -2601,7 +2578,7 @@ static struct tls_sw_context_tx *init_ctx_tx(struct tls_context *ctx, struct soc
}
crypto_init_wait(&sw_ctx_tx->async_wait);
- spin_lock_init(&sw_ctx_tx->encrypt_compl_lock);
+ atomic_set(&sw_ctx_tx->encrypt_pending, 1);
INIT_LIST_HEAD(&sw_ctx_tx->tx_list);
INIT_DELAYED_WORK(&sw_ctx_tx->tx_work.work, tx_work_handler);
sw_ctx_tx->tx_work.sk = sk;
@@ -2622,7 +2599,7 @@ static struct tls_sw_context_rx *init_ctx_rx(struct tls_context *ctx)
}
crypto_init_wait(&sw_ctx_rx->async_wait);
- spin_lock_init(&sw_ctx_rx->decrypt_compl_lock);
+ atomic_set(&sw_ctx_rx->decrypt_pending, 1);
init_waitqueue_head(&sw_ctx_rx->wq);
skb_queue_head_init(&sw_ctx_rx->rx_list);
skb_queue_head_init(&sw_ctx_rx->async_hold);
--
2.43.0
next prev parent reply other threads:[~2024-02-07 1:19 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-07 1:18 [PATCH net 0/7] net: tls: fix some issues with async encryption Jakub Kicinski
2024-02-07 1:18 ` [PATCH net 1/7] net: tls: factor out tls_*crypt_async_wait() Jakub Kicinski
2024-02-09 9:23 ` Simon Horman
2024-02-10 9:08 ` Sabrina Dubroca
2024-02-07 1:18 ` Jakub Kicinski [this message]
2024-02-09 9:24 ` [PATCH net 2/7] tls: fix race between async notify and socket close Simon Horman
2024-02-09 9:47 ` Eric Dumazet
2024-02-10 9:11 ` Sabrina Dubroca
2024-02-07 1:18 ` [PATCH net 3/7] tls: fix race between tx work scheduling " Jakub Kicinski
2024-02-09 9:24 ` Simon Horman
2024-02-10 9:12 ` Sabrina Dubroca
2024-02-07 1:18 ` [PATCH net 4/7] net: tls: handle backlogging of crypto requests Jakub Kicinski
2024-02-09 9:25 ` Simon Horman
2024-02-07 1:18 ` [PATCH net 5/7] net: tls: fix use-after-free with partial reads and async decrypt Jakub Kicinski
2024-02-09 9:25 ` Simon Horman
2024-02-07 1:18 ` [PATCH net 6/7] selftests: tls: use exact comparison in recv_partial Jakub Kicinski
2024-02-09 9:25 ` Simon Horman
2024-02-07 1:18 ` [PATCH net 7/7] net: tls: fix returned read length with async decrypt Jakub Kicinski
2024-02-09 9:22 ` Simon Horman
2024-02-10 9:02 ` Sabrina Dubroca
2024-02-12 17:11 ` Jakub Kicinski
2024-02-10 9:05 ` [PATCH net 0/7] net: tls: fix some issues with async encryption Sabrina Dubroca
2024-02-10 21:40 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240207011824.2609030-3-kuba@kernel.org \
--to=kuba@kernel$(echo .)org \
--cc=borisp@nvidia$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=john.fastabend@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=sd@queasysnail$(echo .)net \
--cc=sec@valis$(echo .)email \
--cc=vadim.fedorenko@linux$(echo .)dev \
--cc=vinay.yadav@chelsio$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox