* [PATCH net] tls: fix NULL pointer dereference on poll
@ 2018-06-11 21:22 Daniel Borkmann
2018-06-11 22:27 ` Dave Watson
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Daniel Borkmann @ 2018-06-11 21:22 UTC (permalink / raw)
To: davem; +Cc: davejwatson, netdev, ast, Daniel Borkmann, Christoph Hellwig
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(-)
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);
}
--
2.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] tls: fix NULL pointer dereference on poll
2018-06-11 21:22 [PATCH net] tls: fix NULL pointer dereference on poll Daniel Borkmann
@ 2018-06-11 22:27 ` Dave Watson
2018-06-11 23:30 ` David Miller
2018-06-12 5:37 ` Christoph Hellwig
2 siblings, 0 replies; 5+ messages in thread
From: Dave Watson @ 2018-06-11 22:27 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, ast, Christoph Hellwig
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tls: fix NULL pointer dereference on poll
2018-06-11 21:22 [PATCH net] tls: fix NULL pointer dereference on poll Daniel Borkmann
2018-06-11 22:27 ` Dave Watson
@ 2018-06-11 23:30 ` David Miller
2018-06-12 5:37 ` Christoph Hellwig
2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2018-06-11 23:30 UTC (permalink / raw)
To: daniel; +Cc: davejwatson, netdev, ast, hch
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tls: fix NULL pointer dereference on poll
2018-06-11 21:22 [PATCH net] tls: fix NULL pointer dereference on poll Daniel Borkmann
2018-06-11 22:27 ` Dave Watson
2018-06-11 23:30 ` David Miller
@ 2018-06-12 5:37 ` Christoph Hellwig
2018-06-12 10:43 ` Daniel Borkmann
2 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2018-06-12 5:37 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, davejwatson, netdev, ast, Christoph Hellwig
> 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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tls: fix NULL pointer dereference on poll
2018-06-12 5:37 ` Christoph Hellwig
@ 2018-06-12 10:43 ` Daniel Borkmann
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Borkmann @ 2018-06-12 10:43 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: davem, davejwatson, netdev, ast
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-06-12 10:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-11 21:22 [PATCH net] tls: fix NULL pointer dereference on poll Daniel Borkmann
2018-06-11 22:27 ` Dave Watson
2018-06-11 23:30 ` David Miller
2018-06-12 5:37 ` Christoph Hellwig
2018-06-12 10:43 ` Daniel Borkmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.