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:

quoted

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?

quoted

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