[PATCH net 0/5] tls: fix some issues with async encryption

[PATCH net 0/5] tls: fix some issues with async encryption

[Sabrina Dubroca]

I've been playing with a few hacks in the crypto code (forcing async
crypto for every request, smaller cryptd queue), and that has revealed
some bugs while running the selftests.

With this setup and those patches applied, the bad_in_large_read test
case still fails. With all-async crypto, we don't know which record
threw the EBADMSG, so we can't keep the first couple of records that
were decrypted correctly. We have to throw away the whole batch.

Liu Jian has also found a bug with async crypto and wrapping record
numbers:
https://lore.kernel.org/netdev/20230906065237.2180187-1-liujian56@huawei.com/
It will get fixed separately and I'll submit a selftest for this case.

Sabrina Dubroca (5):
  net: tls: handle -EBUSY on async encrypt/decrypt requests
  tls: fix use-after-free with partial reads and async decrypt
  tls: fix returned read length with async !zc decrypt
  tls: fix race condition in async decryption of corrupted records
  tls: don't decrypt the next record if it's of a different type

 net/tls/tls_sw.c | 45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

-- 
2.40.1

[PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Sabrina Dubroca]

Since we're setting the CRYPTO_TFM_REQ_MAY_BACKLOG flag on our
requests to the crypto API, crypto_aead_{encrypt,decrypt} can return
 -EBUSY instead of -EINPROGRESS in valid situations. For example, when
the cryptd queue for AESNI is full (easy to trigger with an
artifically low cryptd.cryptd_max_cpu_qlen), requests will be enqueued
to the backlog but still processed. In that case, the async callback
will also be called twice: first with err == -EINPROGRESS, which it
seems we can just ignore, then with err == 0.

I've only tested this on AESNI with cryptd.

Fixes: a54667f6728c ("tls: Add support for encryption using async offload accelerator")
Fixes: 94524d8fc965 ("net/tls: Add support for async decryption of tls records")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/tls/tls_sw.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 1ed4a611631f..4f3dd0403efb 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -196,6 +196,9 @@ static void tls_decrypt_done(void *data, int err)
 	struct sock *sk;
 	int aead_size;
 
+	if (err == -EINPROGRESS)
+		return;
+
 	aead_size = sizeof(*aead_req) + crypto_aead_reqsize(aead);
 	aead_size = ALIGN(aead_size, __alignof__(*dctx));
 	dctx = (void *)((u8 *)aead_req + aead_size);
@@ -261,7 +264,7 @@ static int tls_do_decryption(struct sock *sk,
 	}
 
 	ret = crypto_aead_decrypt(aead_req);
-	if (ret == -EINPROGRESS) {
+	if (ret == -EINPROGRESS || ret == -EBUSY) {
 		if (darg->async)
 			return 0;
 
@@ -443,6 +446,9 @@ static void tls_encrypt_done(void *data, int err)
 	struct sock *sk;
 	int pending;
 
+	if (err == -EINPROGRESS)
+		return;
+
 	msg_en = &rec->msg_encrypted;
 
 	sk = rec->sk;
@@ -544,7 +550,7 @@ static int tls_do_encryption(struct sock *sk,
 	atomic_inc(&ctx->encrypt_pending);
 
 	rc = crypto_aead_encrypt(aead_req);
-	if (!rc || rc != -EINPROGRESS) {
+	if (!rc || (rc != -EINPROGRESS && rc != -EBUSY)) {
 		atomic_dec(&ctx->encrypt_pending);
 		sge->offset -= prot->prepend_size;
 		sge->length += prot->prepend_size;
@@ -552,7 +558,7 @@ static int tls_do_encryption(struct sock *sk,
 
 	if (!rc) {
 		WRITE_ONCE(rec->tx_ready, true);
-	} else if (rc != -EINPROGRESS) {
+	} else if (rc != -EINPROGRESS && rc != -EBUSY) {
 		list_del(&rec->list);
 		return rc;
 	}
@@ -779,7 +785,7 @@ static int tls_push_record(struct sock *sk, int flags,
 	rc = tls_do_encryption(sk, tls_ctx, ctx, req,
 			       msg_pl->sg.size + prot->tail_size, i);
 	if (rc < 0) {
-		if (rc != -EINPROGRESS) {
+		if (rc != -EINPROGRESS && rc != -EBUSY) {
 			tls_err_abort(sk, -EBADMSG);
 			if (split) {
 				tls_ctx->pending_open_record_frags = true;
@@ -990,7 +996,7 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
 	if (unlikely(msg->msg_controllen)) {
 		ret = tls_process_cmsg(sk, msg, &record_type);
 		if (ret) {
-			if (ret == -EINPROGRESS)
+			if (ret == -EINPROGRESS || ret == -EBUSY)
 				num_async++;
 			else if (ret != -EAGAIN)
 				goto send_end;
@@ -1071,7 +1077,7 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
 						  record_type, &copied,
 						  msg->msg_flags);
 			if (ret) {
-				if (ret == -EINPROGRESS)
+				if (ret == -EINPROGRESS || ret == -EBUSY)
 					num_async++;
 				else if (ret == -ENOMEM)
 					goto wait_for_memory;
@@ -1125,7 +1131,7 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
 						  record_type, &copied,
 						  msg->msg_flags);
 			if (ret) {
-				if (ret == -EINPROGRESS)
+				if (ret == -EINPROGRESS || ret == -EBUSY)
 					num_async++;
 				else if (ret == -ENOMEM)
 					goto wait_for_memory;
@@ -1248,6 +1254,7 @@ void tls_sw_splice_eof(struct socket *sock)
 			goto unlock;
 		retrying = true;
 		goto retry;
+	case -EBUSY:
 	case -EINPROGRESS:
 		break;
 	default:
@@ -2106,7 +2113,7 @@ int tls_sw_recvmsg(struct sock *sk,
 		__skb_queue_purge(&ctx->async_hold);
 
 		if (ret) {
-			if (err >= 0 || err == -EINPROGRESS)
+			if (err >= 0 || err == -EINPROGRESS || err == -EBUSY)
 				err = ret;
 			decrypted = 0;
 			goto end;
-- 
2.40.1

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Jakub Kicinski]

On Wed,  6 Sep 2023 19:08:31 +0200 Sabrina Dubroca wrote:
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -196,6 +196,9 @@ static void tls_decrypt_done(void *data, int err)
>  	struct sock *sk;
>  	int aead_size;
>  
> +	if (err == -EINPROGRESS)
> +		return;

Maybe a comment here clarifying that caller got -EBUSY and the callback
will fire again without an error? The flow is slightly counter-
-intuitive.

> @@ -443,6 +446,9 @@ static void tls_encrypt_done(void *data, int err)
>  	struct sock *sk;
>  	int pending;
>  
> +	if (err == -EINPROGRESS)
> +		return;

Same here? 

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Simon Horman]

On Wed, Sep 06, 2023 at 07:08:31PM +0200, Sabrina Dubroca wrote:
> Since we're setting the CRYPTO_TFM_REQ_MAY_BACKLOG flag on our
> requests to the crypto API, crypto_aead_{encrypt,decrypt} can return
>  -EBUSY instead of -EINPROGRESS in valid situations. For example, when
> the cryptd queue for AESNI is full (easy to trigger with an
> artifically low cryptd.cryptd_max_cpu_qlen), requests will be enqueued

Hi Sabrina,

as it looks like there will be a v2, checkpatch --codespell, asked
me to suggest:

 artifically -> artificially

...

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Herbert Xu]

Sabrina Dubroca <sd@queasysnail.net> wrote:
> Since we're setting the CRYPTO_TFM_REQ_MAY_BACKLOG flag on our
> requests to the crypto API, crypto_aead_{encrypt,decrypt} can return
> -EBUSY instead of -EINPROGRESS in valid situations. For example, when
> the cryptd queue for AESNI is full (easy to trigger with an
> artifically low cryptd.cryptd_max_cpu_qlen), requests will be enqueued
> to the backlog but still processed. In that case, the async callback
> will also be called twice: first with err == -EINPROGRESS, which it
> seems we can just ignore, then with err == 0.
> 
> I've only tested this on AESNI with cryptd.
> 
> Fixes: a54667f6728c ("tls: Add support for encryption using async offload accelerator")
> Fixes: 94524d8fc965 ("net/tls: Add support for async decryption of tls records")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
> net/tls/tls_sw.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)

You should only use MAY_BACKLOG if you can actually back off and
stop issuing new requests.  In that case you can only restart
issuing new requests when the EINPROGRESS notification comes in.

If that's not the case here you should drop MAY_BACKLOG altogether.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Sabrina Dubroca]

Thanks for looking at this patch. In retrospect I should have cc'd you
on it.

2023-09-08, 14:10:05 +0800, Herbert Xu wrote:
> Sabrina Dubroca <sd@queasysnail.net> wrote:
> > Since we're setting the CRYPTO_TFM_REQ_MAY_BACKLOG flag on our
> > requests to the crypto API, crypto_aead_{encrypt,decrypt} can return
> > -EBUSY instead of -EINPROGRESS in valid situations. For example, when
> > the cryptd queue for AESNI is full (easy to trigger with an
> > artifically low cryptd.cryptd_max_cpu_qlen), requests will be enqueued
> > to the backlog but still processed. In that case, the async callback
> > will also be called twice: first with err == -EINPROGRESS, which it
> > seems we can just ignore, then with err == 0.
> > 
> > I've only tested this on AESNI with cryptd.
> > 
> > Fixes: a54667f6728c ("tls: Add support for encryption using async offload accelerator")
> > Fixes: 94524d8fc965 ("net/tls: Add support for async decryption of tls records")
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> > net/tls/tls_sw.c | 23 +++++++++++++++--------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> 
> You should only use MAY_BACKLOG if you can actually back off and
> stop issuing new requests.  In that case you can only restart
> issuing new requests when the EINPROGRESS notification comes in.
> 
> If that's not the case here you should drop MAY_BACKLOG altogether.

Uh, ok, I didn't know that, thanks for explaining. When I was fixing
this code I couldn't find a mention of what the expectations for
MAY_BACKLOG are. Could you add a comment describing this in the
headers (either for #define CRYPTO_TFM_REQ_MAY_BACKLOG or
aead_request_set_callback, wherever is more appropriate). MAY_BACKLOG
is used by both tls and tipc (talking only about networking) and
neither seem to respect this need to back off.

Jakub, I guess we should drop the CRYPTO_TFM_REQ_MAY_BACKLOG for net,
and maybe consider adding it back (with the back off) in
net-next. Probably not urgent considering that nobody seems to have
run into this bug so far.

But then we have to handle ENOSPC a bit more gracefully, because right
now it looks like
 - on TX, we break the socket (tls_err_abort when tls_do_encryption returns
   an error)
 - on RX, we also break the socket, and we don't decrement
   decrypt_pending so the recv() call gets stuck

Not sure how complex the changes would be, the sendmsg and recvmsg
code is already a bit hard to follow.

-- 
Sabrina

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Jakub Kicinski]

On Fri, 8 Sep 2023 17:55:59 +0200 Sabrina Dubroca wrote:
> Jakub, I guess we should drop the CRYPTO_TFM_REQ_MAY_BACKLOG for net,
> and maybe consider adding it back (with the back off) in
> net-next. Probably not urgent considering that nobody seems to have
> run into this bug so far.

Someone did mention it long time ago, but I don't recall the context :S
I think it was something about the device queue filling up..

> But then we have to handle ENOSPC a bit more gracefully, because right
> now it looks like
>  - on TX, we break the socket (tls_err_abort when tls_do_encryption returns
>    an error)
>  - on RX, we also break the socket, and we don't decrement
>    decrypt_pending so the recv() call gets stuck
> 
> Not sure how complex the changes would be, the sendmsg and recvmsg
> code is already a bit hard to follow.

To keep it simple we can wait for all in-flight requests to drain if we
hit EBUSY? Basically factor this bit out:

		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 call it after we get EBUSY? We'll drain the pending queue all the
way to empty, which may not be too great for throughput, but who cares
- right now we don't handle EBUSY at all, so it must be extremely rare.

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Herbert Xu]

On Fri, Sep 08, 2023 at 02:26:02PM -0700, Jakub Kicinski wrote:
>
> and call it after we get EBUSY? We'll drain the pending queue all the
> way to empty, which may not be too great for throughput, but who cares
> - right now we don't handle EBUSY at all, so it must be extremely rare.

EBUSY means that you're sending requests to the Crypto API faster
than it can process them.  If left unhandled you will eventually
exhaust all memory.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Herbert Xu]

On Fri, Sep 08, 2023 at 05:55:59PM +0200, Sabrina Dubroca wrote:
>
> Uh, ok, I didn't know that, thanks for explaining. When I was fixing
> this code I couldn't find a mention of what the expectations for
> MAY_BACKLOG are. Could you add a comment describing this in the
> headers (either for #define CRYPTO_TFM_REQ_MAY_BACKLOG or
> aead_request_set_callback, wherever is more appropriate). MAY_BACKLOG
> is used by both tls and tipc (talking only about networking) and
> neither seem to respect this need to back off.

Patches are welcome :)

A bit of history: at the beginning we always dropped requests
that we couldn't queue because the only user was IPsec so this
is the expected behaviour.

When storage crypto support was added there was a need for reliable
handling of resource constraints so that's why MAY_BACKLOG was added.
However, the expectation is obviously that you must stop sending new
requests once you run into the resource constraint.

> Jakub, I guess we should drop the CRYPTO_TFM_REQ_MAY_BACKLOG for net,
> and maybe consider adding it back (with the back off) in
> net-next. Probably not urgent considering that nobody seems to have
> run into this bug so far.

I think that would be the prudent action.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Sabrina Dubroca]

2023-09-12, 12:43:32 +0800, Herbert Xu wrote:
> On Fri, Sep 08, 2023 at 05:55:59PM +0200, Sabrina Dubroca wrote:
> >
> > Uh, ok, I didn't know that, thanks for explaining. When I was fixing
> > this code I couldn't find a mention of what the expectations for
> > MAY_BACKLOG are. Could you add a comment describing this in the
> > headers (either for #define CRYPTO_TFM_REQ_MAY_BACKLOG or
> > aead_request_set_callback, wherever is more appropriate). MAY_BACKLOG
> > is used by both tls and tipc (talking only about networking) and
> > neither seem to respect this need to back off.
> 
> Patches are welcome :)

Ok. I thought it'd be better if you wrote that patch since if I write
it, it'll be a c/p (or rephrase) of what you wrote. But fine, I'll go
ahead and do that :)

> A bit of history: at the beginning we always dropped requests
> that we couldn't queue because the only user was IPsec so this
> is the expected behaviour.
> 
> When storage crypto support was added there was a need for reliable
> handling of resource constraints so that's why MAY_BACKLOG was added.
> However, the expectation is obviously that you must stop sending new
> requests once you run into the resource constraint.
> 
> > Jakub, I guess we should drop the CRYPTO_TFM_REQ_MAY_BACKLOG for net,
> > and maybe consider adding it back (with the back off) in
> > net-next. Probably not urgent considering that nobody seems to have
> > run into this bug so far.
> 
> I think that would be the prudent action.

We'd have to do pretty much what Jakub suggested [1] (handle ENOSPC by
waiting for all current requests) and then resubmit the failed
request. So I think keeping the MAY_BACKLOG flag and handling EBUSY
this way is simpler. With this, we send one request to the backlog,
then we wait until the queue drains.

[1] https://lore.kernel.org/netdev/20230908142602.2ced0631@kernel.org/

-- 
Sabrina

Re: [PATCH net 1/5] net: tls: handle -EBUSY on async encrypt/decrypt requests

[Herbert Xu]

On Tue, Sep 12, 2023 at 05:37:23PM +0200, Sabrina Dubroca wrote:
>
> We'd have to do pretty much what Jakub suggested [1] (handle ENOSPC by
> waiting for all current requests) and then resubmit the failed
> request. So I think keeping the MAY_BACKLOG flag and handling EBUSY
> this way is simpler. With this, we send one request to the backlog,
> then we wait until the queue drains.
> 
> [1] https://lore.kernel.org/netdev/20230908142602.2ced0631@kernel.org/

You need to handle something like ENOSPC anyhow because the underlying
driver may experience a real failure (e.g., someone unplugged the
hardware) which would have pretty much the same effect of ENOSPC
(i.e., an error with no retries).  So if the tls code can't cope with
that then it has to be fixed.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH net 2/5] tls: fix use-after-free with partial reads and async decrypt

[Sabrina Dubroca]

tls_decrypt_sg doesn't take a reference on the pages from clear_skb,
so the put_page() in tls_decrypt_done releases them, and we trigger a
use-after-free in process_rx_list when we try to read from the
partially-read skb.

This can be seen with the recv_and_splice test case.

Fixes: fd31f3996af2 ("tls: rx: decrypt into a fresh skb")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/tls/tls_sw.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 4f3dd0403efb..f23cceaceb36 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -63,6 +63,7 @@ struct tls_decrypt_ctx {
 	u8 iv[MAX_IV_SIZE];
 	u8 aad[TLS_MAX_AAD_SIZE];
 	u8 tail;
+	bool put_outsg;
 	struct scatterlist sg[];
 };
 
@@ -221,7 +222,8 @@ static void tls_decrypt_done(void *data, int err)
 		for_each_sg(sg_next(sgout), sg, UINT_MAX, pages) {
 			if (!sg)
 				break;
-			put_page(sg_page(sg));
+			if (dctx->put_outsg)
+				put_page(sg_page(sg));
 		}
 	}
 
@@ -1549,6 +1551,8 @@ static int tls_decrypt_sg(struct sock *sk, struct iov_iter *out_iov,
 	if (err < 0)
 		goto exit_free;
 
+	dctx->put_outsg = false;
+
 	if (clear_skb) {
 		sg_init_table(sgout, n_sgout);
 		sg_set_buf(&sgout[0], dctx->aad, prot->aad_size);
@@ -1558,6 +1562,8 @@ static int tls_decrypt_sg(struct sock *sk, struct iov_iter *out_iov,
 		if (err < 0)
 			goto exit_free;
 	} else if (out_iov) {
+		dctx->put_outsg = true;
+
 		sg_init_table(sgout, n_sgout);
 		sg_set_buf(&sgout[0], dctx->aad, prot->aad_size);
 
-- 
2.40.1

Re: [PATCH net 2/5] tls: fix use-after-free with partial reads and async decrypt

[Jakub Kicinski]

On Wed,  6 Sep 2023 19:08:32 +0200 Sabrina Dubroca wrote:
> @@ -221,7 +222,8 @@ static void tls_decrypt_done(void *data, int err)
> 	/* Free the destination pages if skb was not decrypted inplace */
> 	if (sgout != sgin) {

This check is always true now, right?
Should we replace it with dctx->put_outsg?

> 		/* Skip the first S/G entry as it points to AAD */
>  		for_each_sg(sg_next(sgout), sg, UINT_MAX, pages) {
>  			if (!sg)
>  				break;
> -			put_page(sg_page(sg));
> +			if (dctx->put_outsg)
> +				put_page(sg_page(sg));
>  		}
>  	}
-- 
pw-bot: cr

Re: [PATCH net 2/5] tls: fix use-after-free with partial reads and async decrypt

[Sabrina Dubroca]

2023-09-06, 19:05:29 -0700, Jakub Kicinski wrote:
> On Wed,  6 Sep 2023 19:08:32 +0200 Sabrina Dubroca wrote:
> > @@ -221,7 +222,8 @@ static void tls_decrypt_done(void *data, int err)
> > 	/* Free the destination pages if skb was not decrypted inplace */
> > 	if (sgout != sgin) {
> 
> This check is always true now, right?
> Should we replace it with dctx->put_outsg?

Ugh, I noticed that when I was debugging, and didn't think about
replacing the test. Yeah, I'll do that in v2.

-- 
Sabrina

[PATCH net 3/5] tls: fix returned read length with async !zc decrypt

[Sabrina Dubroca]

We can double-count the chunk size:
 - once via decrypted
 - once in async_copy_bytes, which then becomes part of
   process_rx_list's return value

Subtract it from decrypted before adding in process_rx_list's return
value.

Fixes: 4d42cd6bc2ac ("tls: rx: fix return value for async crypto")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
This logic is so complex, in this series I'm just trying to make all
the selftests pass at the same time :/

 net/tls/tls_sw.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index f23cceaceb36..babbd43d41ed 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2132,7 +2132,13 @@ int tls_sw_recvmsg(struct sock *sk,
 		else
 			err = process_rx_list(ctx, msg, &control, 0,
 					      async_copy_bytes, is_peek);
-		decrypted += max(err, 0);
+
+		if (err > 0) {
+			/* decrypted already accounts for async_copy_bytes,
+			 * we don't want to double-count
+			 */
+			decrypted += err - async_copy_bytes;
+		}
 	}
 
 	copied += decrypted;
-- 
2.40.1

[PATCH net 4/5] tls: fix race condition in async decryption of corrupted records

[Sabrina Dubroca]

In case a corrupted record is decrypted asynchronously, the error can
be reported in two ways:
 - via the completion's error mechanism
 - via sk->sk_err

If all asynchronous decrypts finish before we reach the end of
tls_sw_recvmsg, decrypt_pending will be 0 and we don't check the
completion for errors. We should still check sk->sk_err, otherwise
we'll miss the error and return garbage to userspace. This is visible
in the bad_auth test case.

Fixes: 94524d8fc965 ("net/tls: Add support for async decryption of tls records")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/tls/tls_sw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index babbd43d41ed..f80a2ea1dd7e 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2116,6 +2116,9 @@ int tls_sw_recvmsg(struct sock *sk,
 		ret = 0;
 		if (pending)
 			ret = crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
+		/* Crypto completion could have run before us, check sk_err */
+		if (ret == 0)
+			ret = -sk->sk_err;
 		__skb_queue_purge(&ctx->async_hold);
 
 		if (ret) {
-- 
2.40.1

[PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Sabrina Dubroca]

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(+)

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);
-- 
2.40.1

Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Jakub Kicinski]

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;

Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Sabrina Dubroca]

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;
 
-- 
Sabrina

Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Jakub Kicinski]

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.

Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Herbert Xu]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Sabrina Dubroca]

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/

-- 
Sabrina

Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Herbert Xu]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Sabrina Dubroca]

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?

-- 
Sabrina

Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

[Herbert Xu]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt