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

[Jakub Kicinski <kuba@kernel.org>]

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.