| Message ID | 20180611212204.24002-1-daniel@iogearbox.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] tls: fix NULL pointer dereference on poll | expand |
On 06/11/18 11:22 PM, Daniel Borkmann wrote: > While hacking on kTLS, I ran into the following panic from an > unprivileged netserver / netperf TCP session: > > [...] > Looks like the recent conversion from poll to poll_mask callback started > in 152524231023 ("net: add support for ->poll_mask in proto_ops") missed > to eventually convert kTLS, too: TCP's ->poll was converted over to the > ->poll_mask in commit 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") > and therefore kTLS wrongly saved the ->poll old one which is now NULL. > > Convert kTLS over to use ->poll_mask instead. Also instead of POLLIN | > POLLRDNORM use the proper EPOLLIN | EPOLLRDNORM bits as the case in > tcp_poll_mask() as well that is mangled here. Thanks, was just trying to bisect this myself. Works for me. Tested-by: Dave Watson <davejwatson@fb.com> > Fixes: 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Dave Watson <davejwatson@fb.com>
From: Daniel Borkmann <daniel@iogearbox.net> Date: Mon, 11 Jun 2018 23:22:04 +0200 > While hacking on kTLS, I ran into the following panic from an > unprivileged netserver / netperf TCP session: ... > Debugging further, it turns out that calling into ctx->sk_poll() is > invalid since sk_poll itself is NULL which was saved from the original > TCP socket in order for tls_sw_poll() to invoke it. > > Looks like the recent conversion from poll to poll_mask callback started > in 152524231023 ("net: add support for ->poll_mask in proto_ops") missed > to eventually convert kTLS, too: TCP's ->poll was converted over to the > ->poll_mask in commit 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") > and therefore kTLS wrongly saved the ->poll old one which is now NULL. > > Convert kTLS over to use ->poll_mask instead. Also instead of POLLIN | > POLLRDNORM use the proper EPOLLIN | EPOLLRDNORM bits as the case in > tcp_poll_mask() as well that is mangled here. > > Fixes: 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Applied, thanks Daniel.
> Looks like the recent conversion from poll to poll_mask callback started > in 152524231023 ("net: add support for ->poll_mask in proto_ops") missed > to eventually convert kTLS, too: TCP's ->poll was converted over to the > ->poll_mask in commit 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") > and therefore kTLS wrongly saved the ->poll old one which is now NULL. Looks like this TLS code was added in the same cycle. > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c > index 301f224..a127d61 100644 > --- a/net/tls/tls_main.c > +++ b/net/tls/tls_main.c > @@ -712,7 +712,7 @@ static int __init tls_register(void) > build_protos(tls_prots[TLSV4], &tcp_prot); > > tls_sw_proto_ops = inet_stream_ops; > - tls_sw_proto_ops.poll = tls_sw_poll; > + tls_sw_proto_ops.poll_mask = tls_sw_poll_mask; > tls_sw_proto_ops.splice_read = tls_sw_splice_read; Not new in this patch, but copying ops vectors is a very bad idea, not only because your new instance can't be marked const and you thus open up exploit vectors. I would suggest to clean this up eventually. > +__poll_t tls_sw_poll_mask(struct socket *sock, __poll_t events) > { > struct sock *sk = sock->sk; > struct tls_context *tls_ctx = tls_get_ctx(sk); > struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx); > + __poll_t mask; > > + /* Grab EPOLLOUT and EPOLLHUP from the underlying socket */ > + mask = ctx->sk_poll_mask(sock, events); > > + /* Clear EPOLLIN bits, and set based on recv_pkt */ > + mask &= ~(EPOLLIN | EPOLLRDNORM); > if (ctx->recv_pkt) > + mask |= EPOLLIN | EPOLLRDNORM; > > + return mask; So you call the underlying protocol method on the struct sock of the TLS code? Again not reall new in this patch, but how is this even supposed to work?
On 06/12/2018 07:37 AM, Christoph Hellwig wrote: >> Looks like the recent conversion from poll to poll_mask callback started >> in 152524231023 ("net: add support for ->poll_mask in proto_ops") missed >> to eventually convert kTLS, too: TCP's ->poll was converted over to the >> ->poll_mask in commit 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") >> and therefore kTLS wrongly saved the ->poll old one which is now NULL. > > Looks like this TLS code was added in the same cycle. > >> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c >> index 301f224..a127d61 100644 >> --- a/net/tls/tls_main.c >> +++ b/net/tls/tls_main.c >> @@ -712,7 +712,7 @@ static int __init tls_register(void) >> build_protos(tls_prots[TLSV4], &tcp_prot); >> >> tls_sw_proto_ops = inet_stream_ops; >> - tls_sw_proto_ops.poll = tls_sw_poll; >> + tls_sw_proto_ops.poll_mask = tls_sw_poll_mask; >> tls_sw_proto_ops.splice_read = tls_sw_splice_read; > > Not new in this patch, but copying ops vectors is a very bad idea, not > only because your new instance can't be marked const and you thus open > up exploit vectors. I would suggest to clean this up eventually. Generally, agree with you. It could at minimum also be a __ro_after_init candidate, at least the TLSV4 ops which wouldn't change. In v6 case though it could be loaded as a module after TLS was initialized. >> +__poll_t tls_sw_poll_mask(struct socket *sock, __poll_t events) >> { >> struct sock *sk = sock->sk; >> struct tls_context *tls_ctx = tls_get_ctx(sk); >> struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx); >> + __poll_t mask; >> >> + /* Grab EPOLLOUT and EPOLLHUP from the underlying socket */ >> + mask = ctx->sk_poll_mask(sock, events); >> >> + /* Clear EPOLLIN bits, and set based on recv_pkt */ >> + mask &= ~(EPOLLIN | EPOLLRDNORM); >> if (ctx->recv_pkt) >> + mask |= EPOLLIN | EPOLLRDNORM; >> >> + return mask; > > So you call the underlying protocol method on the struct sock of > the TLS code? Again not reall new in this patch, but how is this > even supposed to work? Yeah, patch doesn't change it, but reason is that TLS relies on kernel's stream parser to determine TLS message boundary on ingress, so once a full message got received only then we want to signal this to the user space application. Latter skb is then held in ctx->recv_pkt via stream parser. Thanks, Daniel
diff --git a/include/net/tls.h b/include/net/tls.h index 70c2737..7f84ea3 100644 --- a/include/net/tls.h +++ b/include/net/tls.h @@ -109,8 +109,7 @@ struct tls_sw_context_rx { struct strparser strp; void (*saved_data_ready)(struct sock *sk); - unsigned int (*sk_poll)(struct file *file, struct socket *sock, - struct poll_table_struct *wait); + __poll_t (*sk_poll_mask)(struct socket *sock, __poll_t events); struct sk_buff *recv_pkt; u8 control; bool decrypted; @@ -225,8 +224,7 @@ void tls_sw_free_resources_tx(struct sock *sk); void tls_sw_free_resources_rx(struct sock *sk); int tls_sw_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock, int flags, int *addr_len); -unsigned int tls_sw_poll(struct file *file, struct socket *sock, - struct poll_table_struct *wait); +__poll_t tls_sw_poll_mask(struct socket *sock, __poll_t events); ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos, struct pipe_inode_info *pipe, size_t len, unsigned int flags); diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c index 301f224..a127d61 100644 --- a/net/tls/tls_main.c +++ b/net/tls/tls_main.c @@ -712,7 +712,7 @@ static int __init tls_register(void) build_protos(tls_prots[TLSV4], &tcp_prot); tls_sw_proto_ops = inet_stream_ops; - tls_sw_proto_ops.poll = tls_sw_poll; + tls_sw_proto_ops.poll_mask = tls_sw_poll_mask; tls_sw_proto_ops.splice_read = tls_sw_splice_read; #ifdef CONFIG_TLS_DEVICE diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 8ca57d0..34895b7 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -915,23 +915,22 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos, return copied ? : err; } -unsigned int tls_sw_poll(struct file *file, struct socket *sock, - struct poll_table_struct *wait) +__poll_t tls_sw_poll_mask(struct socket *sock, __poll_t events) { - unsigned int ret; struct sock *sk = sock->sk; struct tls_context *tls_ctx = tls_get_ctx(sk); struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx); + __poll_t mask; - /* Grab POLLOUT and POLLHUP from the underlying socket */ - ret = ctx->sk_poll(file, sock, wait); + /* Grab EPOLLOUT and EPOLLHUP from the underlying socket */ + mask = ctx->sk_poll_mask(sock, events); - /* Clear POLLIN bits, and set based on recv_pkt */ - ret &= ~(POLLIN | POLLRDNORM); + /* Clear EPOLLIN bits, and set based on recv_pkt */ + mask &= ~(EPOLLIN | EPOLLRDNORM); if (ctx->recv_pkt) - ret |= POLLIN | POLLRDNORM; + mask |= EPOLLIN | EPOLLRDNORM; - return ret; + return mask; } static int tls_read_size(struct strparser *strp, struct sk_buff *skb) @@ -1188,7 +1187,7 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx) sk->sk_data_ready = tls_data_ready; write_unlock_bh(&sk->sk_callback_lock); - sw_ctx_rx->sk_poll = sk->sk_socket->ops->poll; + sw_ctx_rx->sk_poll_mask = sk->sk_socket->ops->poll_mask; strp_check_rcv(&sw_ctx_rx->strp); }
While hacking on kTLS, I ran into the following panic from an unprivileged netserver / netperf TCP session: BUG: unable to handle kernel NULL pointer dereference at 0000000000000000 PGD 800000037f378067 P4D 800000037f378067 PUD 3c0e61067 PMD 0 Oops: 0010 [#1] SMP KASAN PTI CPU: 1 PID: 2289 Comm: netserver Not tainted 4.17.0+ #139 Hardware name: LENOVO 20FBCTO1WW/20FBCTO1WW, BIOS N1FET47W (1.21 ) 11/28/2016 RIP: 0010: (null) Code: Bad RIP value. RSP: 0018:ffff88036abcf740 EFLAGS: 00010246 RAX: dffffc0000000000 RBX: ffff88036f5f6800 RCX: 1ffff1006debed26 RDX: ffff88036abcf920 RSI: ffff8803cb1a4f00 RDI: ffff8803c258c280 RBP: ffff8803c258c280 R08: ffff8803c258c280 R09: ffffed006f559d48 R10: ffff88037aacea43 R11: ffffed006f559d49 R12: ffff8803c258c280 R13: ffff8803cb1a4f20 R14: 00000000000000db R15: ffffffffc168a350 FS: 00007f7e631f4700(0000) GS:ffff8803d1c80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffffffffffffd6 CR3: 00000003ccf64005 CR4: 00000000003606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: ? tls_sw_poll+0xa4/0x160 [tls] ? sock_poll+0x20a/0x680 ? do_select+0x77b/0x11a0 ? poll_schedule_timeout.constprop.12+0x130/0x130 ? pick_link+0xb00/0xb00 ? read_word_at_a_time+0x13/0x20 ? vfs_poll+0x270/0x270 ? deref_stack_reg+0xad/0xe0 ? __read_once_size_nocheck.constprop.6+0x10/0x10 [...] Debugging further, it turns out that calling into ctx->sk_poll() is invalid since sk_poll itself is NULL which was saved from the original TCP socket in order for tls_sw_poll() to invoke it. Looks like the recent conversion from poll to poll_mask callback started in 152524231023 ("net: add support for ->poll_mask in proto_ops") missed to eventually convert kTLS, too: TCP's ->poll was converted over to the ->poll_mask in commit 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") and therefore kTLS wrongly saved the ->poll old one which is now NULL. Convert kTLS over to use ->poll_mask instead. Also instead of POLLIN | POLLRDNORM use the proper EPOLLIN | EPOLLRDNORM bits as the case in tcp_poll_mask() as well that is mangled here. Fixes: 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Cc: Christoph Hellwig <hch@lst.de> Cc: Dave Watson <davejwatson@fb.com> --- include/net/tls.h | 6 ++---- net/tls/tls_main.c | 2 +- net/tls/tls_sw.c | 19 +++++++++---------- 3 files changed, 12 insertions(+), 15 deletions(-)