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

[Sabrina Dubroca <sd@queasysnail.net>]

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