| Message ID | 20231122214447.675768-1-jannh@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 53f2cb491b500897a619ff6abd72f565933760f0 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tls: fix NULL deref on tls_sw_splice_eof() with empty record | expand |
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/>.
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!
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>
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
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.
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,
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(+) base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263