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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZP/rS+NtSbJ3EuWc@gondor.apana.org.au>
Date: Tue, 12 Sep 2023 12:38:35 +0800
From: Herbert Xu <herbert@...dor.apana.org.au>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	Dave Watson <davejwatson@...com>, Vakul Garg <vakul.garg@....com>,
	Boris Pismenny <borisp@...dia.com>,
	John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a
 different type

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