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
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(-)
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(-)
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(-)
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(+)
@@ -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){
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(+)
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-09-07 02:05:30
On Wed, 6 Sep 2023 19:08:32 +0200 Sabrina Dubroca wrote:
quoted hunk
@@ -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));
}
}
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-09-07 03:47:28
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.
From: Simon Horman <horms@kernel.org> Date: 2023-09-07 15:11:51
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
...
On Wed, 6 Sep 2023 19:08:35 +0200 Sabrina Dubroca wrote:
quoted
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< --------
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-09-07 17:08:28
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.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2023-09-08 06:10:24
Sabrina Dubroca [off-list ref] 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 [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2023-09-08 06:26:49
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 [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Sep 07, 2023 at 02:21:59PM +0200, Sabrina Dubroca wrote:
quoted
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
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 [off-list ref] wrote:
quoted
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-09-08 21:26:03
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.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2023-09-09 01:26:56
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.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2023-09-12 04:38:46
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);
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2023-09-12 04:43:37
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.
On Fri, Sep 08, 2023 at 05:55:59PM +0200, Sabrina Dubroca wrote:
quoted
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.
quoted
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
On Fri, Sep 08, 2023 at 05:38:49PM +0200, Sabrina Dubroca wrote:
quoted
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?
quoted
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
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2023-09-14 09:00:53
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 [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt