[PATCH net] tls: fix NULL deref on tls_sw_splice_eof() with empty record

[PATCH net] tls: fix NULL deref on tls_sw_splice_eof() with empty record

[Jann Horn]

syzkaller discovered that if tls_sw_splice_eof() is executed as part of
sendfile() when the plaintext/ciphertext sk_msg are empty, the send path
gets confused because the empty ciphertext buffer does not have enough
space for the encryption overhead. This causes tls_push_record() to go on
the `split = true` path (which is only supposed to be used when interacting
with an attached BPF program), and then get further confused and hit the
tls_merge_open_record() path, which then assumes that there must be at
least one populated buffer element, leading to a NULL deref.

It is possible to have empty plaintext/ciphertext buffers if we previously
bailed from tls_sw_sendmsg_locked() via the tls_trim_both_msgs() path.
tls_sw_push_pending_record() already handles this case correctly; let's do
the same check in tls_sw_splice_eof().

Fixes: df720d288dbb ("tls/sw: Use splice_eof() to flush")
Cc: stable@vger.kernel.org
Reported-by: syzbot+40d43509a099ea756317@syzkaller.appspotmail.com
Signed-off-by: Jann Horn <jannh@google.com>
---
Note: syzkaller did not find a reliable reproducer for this; I wrote
my own reproducer that reliably hits the crash on an unpatched kernel,
and I've verified that the crash goes away with the patch applied.

 net/tls/tls_sw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index a78e8e722409..316f76187962 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1232,11 +1232,14 @@ void tls_sw_splice_eof(struct socket *sock)
 	lock_sock(sk);
 
 retry:
+	/* same checks as in tls_sw_push_pending_record() */
 	rec = ctx->open_rec;
 	if (!rec)
 		goto unlock;
 
 	msg_pl = &rec->msg_plaintext;
+	if (msg_pl->sg.size == 0)
+		goto unlock;
 
 	/* Check the BPF advisor and perform transmission. */
 	ret = bpf_exec_tx_verdict(msg_pl, sk, false, TLS_RECORD_TYPE_DATA,

base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
-- 
2.43.0.rc1.413.gea7ed67945-goog

Re: [PATCH net] tls: fix NULL deref on tls_sw_splice_eof() with empty record

[Jann Horn]

On Wed, Nov 22, 2023 at 10:44 PM Jann Horn <jannh@google.com> wrote:
> syzkaller discovered that if tls_sw_splice_eof() is executed as part of
> sendfile() when the plaintext/ciphertext sk_msg are empty, the send path
> gets confused because the empty ciphertext buffer does not have enough
> space for the encryption overhead. This causes tls_push_record() to go on
> the `split = true` path (which is only supposed to be used when interacting
> with an attached BPF program), and then get further confused and hit the
> tls_merge_open_record() path, which then assumes that there must be at
> least one populated buffer element, leading to a NULL deref.

Ah, and in case you're looking for the corresponding syzkaller report,
you can find that at
<https://lore.kernel.org/all/000000000000347a250608e8a4d1@google.com/T/>.

Re: [PATCH net] tls: fix NULL deref on tls_sw_splice_eof() with empty record

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 22 Nov 2023 22:44:47 +0100 you wrote:
> syzkaller discovered that if tls_sw_splice_eof() is executed as part of
> sendfile() when the plaintext/ciphertext sk_msg are empty, the send path
> gets confused because the empty ciphertext buffer does not have enough
> space for the encryption overhead. This causes tls_push_record() to go on
> the `split = true` path (which is only supposed to be used when interacting
> with an attached BPF program), and then get further confused and hit the
> tls_merge_open_record() path, which then assumes that there must be at
> least one populated buffer element, leading to a NULL deref.
> 
> [...]

Here is the summary with links:
  - [net] tls: fix NULL deref on tls_sw_splice_eof() with empty record
    https://git.kernel.org/netdev/net/c/53f2cb491b50

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] tls: fix NULL deref on tls_sw_splice_eof() with empty record

[John Fastabend]

Jann Horn wrote:
> On Wed, Nov 22, 2023 at 10:44 PM Jann Horn <jannh@google.com> wrote:
> > syzkaller discovered that if tls_sw_splice_eof() is executed as part of
> > sendfile() when the plaintext/ciphertext sk_msg are empty, the send path
> > gets confused because the empty ciphertext buffer does not have enough
> > space for the encryption overhead. This causes tls_push_record() to go on
> > the `split = true` path (which is only supposed to be used when interacting
> > with an attached BPF program), and then get further confused and hit the
> > tls_merge_open_record() path, which then assumes that there must be at
> > least one populated buffer element, leading to a NULL deref.
> 
> Ah, and in case you're looking for the corresponding syzkaller report,
> you can find that at
> <https://lore.kernel.org/all/000000000000347a250608e8a4d1@google.com/T/>.

I'm a bit slow, but looks good to me as well. Thanks a lot.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH net] tls: fix NULL deref on tls_sw_splice_eof() with empty record

[David Howells]

Jann Horn <jannh@google.com> wrote:

> +	/* same checks as in tls_sw_push_pending_record() */

Wouldn't it be better to say what you're checking rather than referring off to
another function that might one day disappear or be renamed?

David

Re: [PATCH net] tls: fix NULL deref on tls_sw_splice_eof() with empty record

[Jann Horn]

On Mon, Nov 27, 2023 at 10:04 AM David Howells <dhowells@redhat.com> wrote:
> Jann Horn <jannh@google.com> wrote:
>
> > +     /* same checks as in tls_sw_push_pending_record() */
>
> Wouldn't it be better to say what you're checking rather than referring off to
> another function that might one day disappear or be renamed?

Hm, maybe? My thought was that since this is kind of a special version
of what tls_sw_push_pending_record() does, it's clearer to refer to
sort of the canonical version of these checks. And if that ever
disappears or gets renamed or whatever, and someone misses the
comment, you'll still have git history to look at.

And if, in the future, someone decides to add more checks to
tls_sw_push_pending_record() for whatever reason, commenting it this
way will make it clearer that tls_sw_splice_eof() could potentially
require the same checks.