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

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.