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]

Date: Wed, 13 Sep 2023 15:25:29 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Herbert Xu <herbert@...dor.apana.org.au>
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

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