Re: [PATCH net] tls: fix NULL pointer dereference on poll
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], _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
Re: [PATCH net] tls: fix NULL pointer dereference on poll
> 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], _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?
Re: [PATCH net] tls: fix NULL pointer dereference on poll
From: Daniel Borkmann 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 Applied, thanks Daniel.
Re: [PATCH net] tls: fix NULL pointer dereference on poll
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 > Fixes: 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") > Signed-off-by: Daniel Borkmann > Cc: Christoph Hellwig > Cc: Dave Watson
[PATCH net] tls: fix NULL pointer dereference on poll
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 PGD 80037f378067 P4D 80037f378067 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:88036abcf740 EFLAGS: 00010246 RAX: dc00 RBX: 88036f5f6800 RCX: 11006debed26 RDX: 88036abcf920 RSI: 8803cb1a4f00 RDI: 8803c258c280 RBP: 8803c258c280 R08: 8803c258c280 R09: ed006f559d48 R10: 88037aacea43 R11: ed006f559d49 R12: 8803c258c280 R13: 8803cb1a4f20 R14: 00db R15: c168a350 FS: 7f7e631f4700() GS:8803d1c8() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: ffd6 CR3: 0003ccf64005 CR4: 003606e0 DR0: DR1: DR2: DR3: DR6: fffe0ff0 DR7: 0400 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 Cc: Christoph Hellwig Cc: Dave Watson --- include/net/tls.h | 6 ++ net/tls/tls_main.c | 2 +- net/tls/tls_sw.c | 19 +-- 3 files changed, 12 insertions(+), 15 deletions(-) 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], _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