| Message ID | be8519564777b3a40eeb63002041576f9009a733.1694018970.git.sd@queasysnail.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tls: fix some issues with async encryption | expand |
On Wed, 6 Sep 2023 19:08:35 +0200 Sabrina Dubroca wrote: > If the next record is of a different type, we won't copy it to > userspace in this round, tls_record_content_type will stop us just > after decryption. Skip decryption until the next recvmsg() call. > > This fixes a use-after-free when a data record is decrypted > asynchronously but doesn't fill the userspace buffer, and the next > record is non-data, for example in the bad_cmsg selftest. What's the UAF on? > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index f80a2ea1dd7e..86b835b15872 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -2010,6 +2010,9 @@ int tls_sw_recvmsg(struct sock *sk, > else > darg.async = false; > > + if (ctx->async_capable && control && tlm->control != control) > + goto recv_end;
2023-09-06, 20:47:27 -0700, Jakub Kicinski wrote: > On Wed, 6 Sep 2023 19:08:35 +0200 Sabrina Dubroca wrote: > > If the next record is of a different type, we won't copy it to > > userspace in this round, tls_record_content_type will stop us just > > after decryption. Skip decryption until the next recvmsg() call. > > > > This fixes a use-after-free when a data record is decrypted > > asynchronously but doesn't fill the userspace buffer, and the next > > record is non-data, for example in the bad_cmsg selftest. > > What's the UAF on? It doesn't always happen unless I set cryptd_delay_ms from my debug patch (10 is enough): https://patchwork.kernel.org/project/linux-crypto/patch/9d664093b1bf7f47497b2c40b3a085b45f3274a2.1694021240.git.sd@queasysnail.net/ UAF is on the crypto_async_request (part of the aead_request, allocated in the big kmalloc in tls_decrypt_sg), mostly caught when cryptd_queue_worker calls crypto_request_complete, but sometimes a bit earlier (in crypto_dequeue_request). I'll admit this patch is papering over the issue a bit, but decrypting a record we know we won't read within this recv() call seems a bit pointless. I wonder if the way we're using ctx->async_wait here is correct. I'm observing crypto_wait_req return 0 even though the decryption hasn't run yet (and it should return -EBADMSG, not 0). I guess tls_decrypt_done calls the completion (since we only had one decrypt_pending), and then crypto_wait_req thinks everything is already done. Adding a fresh crypto_wait in tls_do_decryption (DECLARE_CRYPTO_WAIT) and using it in the !darg->async case also seems to fix the UAF (but makes the bad_cmsg test case fail in the same way as what I wrote in the cover letter for bad_in_large_read -- not decrypting the next message at all makes the selftest pass). Herbert, WDYT? We're calling tls_do_decryption twice from the same tls_sw_recvmsg invocation, first with darg->async = true, then with darg->async = false. Is it ok to use ctx->async_wait for both, or do we need a fresh one as in this patch? -------- 8< -------- diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 86b835b15872..ad51960f2864 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -246,6 +246,7 @@ static int tls_do_decryption(struct sock *sk, struct tls_context *tls_ctx = tls_get_ctx(sk); struct tls_prot_info *prot = &tls_ctx->prot_info; struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx); + DECLARE_CRYPTO_WAIT(wait); int ret; aead_request_set_tfm(aead_req, ctx->aead_recv); @@ -262,7 +263,7 @@ static int tls_do_decryption(struct sock *sk, } else { aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_BACKLOG, - crypto_req_done, &ctx->async_wait); + crypto_req_done, &wait); } ret = crypto_aead_decrypt(aead_req); @@ -270,7 +271,7 @@ static int tls_do_decryption(struct sock *sk, if (darg->async) return 0; - ret = crypto_wait_req(ret, &ctx->async_wait); + ret = crypto_wait_req(ret, &wait); } darg->async = false;
On Thu, 7 Sep 2023 14:21:59 +0200 Sabrina Dubroca wrote: > I wonder if the way we're using ctx->async_wait here is correct. I'm > observing crypto_wait_req return 0 even though the decryption hasn't > run yet (and it should return -EBADMSG, not 0). I guess > tls_decrypt_done calls the completion (since we only had one > decrypt_pending), and then crypto_wait_req thinks everything is > already done. > > Adding a fresh crypto_wait in tls_do_decryption (DECLARE_CRYPTO_WAIT) > and using it in the !darg->async case also seems to fix the UAF (but > makes the bad_cmsg test case fail in the same way as what I wrote in > the cover letter for bad_in_large_read -- not decrypting the next > message at all makes the selftest pass). > > Herbert, WDYT? We're calling tls_do_decryption twice from the same > tls_sw_recvmsg invocation, first with darg->async = true, then with > darg->async = false. Is it ok to use ctx->async_wait for both, or do > we need a fresh one as in this patch? I think you're right, we need a fresh one. The "non-async" call to tls_do_decryption() will see the completion that the "async" call queued and think it can progress. Then at the end of recv we check ->decrypt_pending and think we're good to exit. But the "non-async" call is still crypt'ing. All makes sense.
On Thu, Sep 07, 2023 at 02:21:59PM +0200, Sabrina Dubroca wrote: > > Herbert, WDYT? We're calling tls_do_decryption twice from the same > tls_sw_recvmsg invocation, first with darg->async = true, then with > darg->async = false. Is it ok to use ctx->async_wait for both, or do > we need a fresh one as in this patch? Yes I think your patch makes sense and the existing code could malfunction if two decryption requests occur during the same tls_sw_recvmsg call, with the first being async and the second being sync. However, I'm still unsure about the case where two async decryption requests occur during the same tls_sw_recvmsg call. Or perhaps this is not possible due to other constraints that are not obvious? Thanks,
2023-09-08, 14:06:12 +0800, Herbert Xu wrote: > On Thu, Sep 07, 2023 at 02:21:59PM +0200, Sabrina Dubroca wrote: > > > > Herbert, WDYT? We're calling tls_do_decryption twice from the same > > tls_sw_recvmsg invocation, first with darg->async = true, then with > > darg->async = false. Is it ok to use ctx->async_wait for both, or do > > we need a fresh one as in this patch? > > Yes I think your patch makes sense and the existing code could > malfunction if two decryption requests occur during the same > tls_sw_recvmsg call, with the first being async and the second > being sync. Thanks for confirming. I'll add it to v2 of this series. > However, I'm still unsure about the case where two async decryption > requests occur during the same tls_sw_recvmsg call. Or perhaps this > is not possible due to other constraints that are not obvious? tls_decrypt_done only runs the completion when decrypt_pending drops to 0, so this should be covered. I wonder if this situation could happen: tls_sw_recvmsg process first record decrypt_pending = 1 CB runs decrypt_pending = 0 complete(&ctx->async_wait.completion); process second record decrypt_pending = 1 tls_sw_recvmsg reaches "recv_end" decrypt_pending != 0 crypto_wait_req sees the first completion of ctx->async_wait and proceeds CB runs decrypt_pending = 0 complete(&ctx->async_wait.completion); With my force_async patch I've managed to run into situations where the CB runs before we reach the crypto_wait_req at the end of tls_sw_recvmsg (patch 4 of this series [1]). I don't know if it's a side-effect of my hack or if it's realistic. [1] https://patchwork.kernel.org/project/netdevbpf/patch/e094325019f7fd960470c10efda41c1b7f9bc54f.1694018970.git.sd@queasysnail.net/
On Fri, Sep 08, 2023 at 05:38:49PM +0200, Sabrina Dubroca wrote: > > tls_decrypt_done only runs the completion when decrypt_pending drops > to 0, so this should be covered. That doesn't look very safe. What if the first decrypt completes before the second decrypt even starts? Wouldn't that cause two complete calls on ctx->async_wait? > I wonder if this situation could happen: > > tls_sw_recvmsg > process first record > decrypt_pending = 1 > CB runs > decrypt_pending = 0 > complete(&ctx->async_wait.completion); > > process second record > decrypt_pending = 1 > tls_sw_recvmsg reaches "recv_end" > decrypt_pending != 0 > crypto_wait_req sees the first completion of ctx->async_wait and proceeds > > CB runs > decrypt_pending = 0 > complete(&ctx->async_wait.completion); Yes that's exactly what I was thinking of. I think this whole thing needs some rethinking and rewriting. Thanks,
2023-09-12, 12:38:35 +0800, Herbert Xu wrote: > On Fri, Sep 08, 2023 at 05:38:49PM +0200, Sabrina Dubroca wrote: > > > > tls_decrypt_done only runs the completion when decrypt_pending drops > > to 0, so this should be covered. > > That doesn't look very safe. What if the first decrypt completes > before the second decrypt even starts? Wouldn't that cause two > complete calls on ctx->async_wait? > > > I wonder if this situation could happen: > > > > tls_sw_recvmsg > > process first record > > decrypt_pending = 1 > > CB runs > > decrypt_pending = 0 > > complete(&ctx->async_wait.completion); > > > > process second record > > decrypt_pending = 1 > > tls_sw_recvmsg reaches "recv_end" > > decrypt_pending != 0 > > crypto_wait_req sees the first completion of ctx->async_wait and proceeds > > > > CB runs > > decrypt_pending = 0 > > complete(&ctx->async_wait.completion); > > Yes that's exactly what I was thinking of. > > I think this whole thing needs some rethinking and rewriting. I'm not sure there's a problem. In tls_sw_recvmsg, the code waiting for async decrypts does: /* Wait for all previously submitted records to be decrypted */ 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); ret = 0; if (pending) ret = crypto_wait_req(-EINPROGRESS, &ctx->async_wait); And the async callback finishes with: spin_lock_bh(&ctx->decrypt_compl_lock); if (!atomic_dec_return(&ctx->decrypt_pending)) complete(&ctx->async_wait.completion); spin_unlock_bh(&ctx->decrypt_compl_lock); Since we have the reinit_completion call, we'll ignore the previous complete() (for the first record), and still wait for the 2nd record's completion. Does that still look unsafe to you?
On Wed, Sep 13, 2023 at 03:25:29PM +0200, Sabrina Dubroca wrote: > > Does that still look unsafe to you? You're right. It does ssem to be safe but the use of a spin lock as well as a completion looks a bit iffy. Thanks,
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index f80a2ea1dd7e..86b835b15872 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -2010,6 +2010,9 @@ int tls_sw_recvmsg(struct sock *sk, else darg.async = false; + if (ctx->async_capable && control && tlm->control != control) + goto recv_end; + err = tls_rx_one_record(sk, msg, &darg); if (err < 0) { tls_err_abort(sk, -EBADMSG);
If the next record is of a different type, we won't copy it to userspace in this round, tls_record_content_type will stop us just after decryption. Skip decryption until the next recvmsg() call. This fixes a use-after-free when a data record is decrypted asynchronously but doesn't fill the userspace buffer, and the next record is non-data, for example in the bad_cmsg selftest. Fixes: c0ab4732d4c6 ("net/tls: Do not use async crypto for non-data records") Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- net/tls/tls_sw.c | 3 +++ 1 file changed, 3 insertions(+)