* [PATCH v1] net/handshake: remove fput() that causes use-after-free
@ 2023-06-13 8:32 Lin Ma
2023-06-13 14:00 ` Chuck Lever III
2023-06-13 18:03 ` Jakub Kicinski
0 siblings, 2 replies; 3+ messages in thread
From: Lin Ma @ 2023-06-13 8:32 UTC (permalink / raw)
To: chuck.lever, davem, edumazet, kuba, kernel-tls-handshake; +Cc: Lin Ma
A reference underflow is found in TLS handshake subsystem that causes a
direct use-after-free. Part of the crash log is like below:
[ 2.022114] ------------[ cut here ]------------
[ 2.022193] refcount_t: underflow; use-after-free.
[ 2.022288] WARNING: CPU: 0 PID: 60 at lib/refcount.c:28 refcount_warn_saturate+0xbe/0x110
[ 2.022432] Modules linked in:
[ 2.022848] RIP: 0010:refcount_warn_saturate+0xbe/0x110
[ 2.023231] RSP: 0018:ffffc900001bfe18 EFLAGS: 00000286
[ 2.023325] RAX: 0000000000000000 RBX: 0000000000000007 RCX: 00000000ffffdfff
[ 2.023438] RDX: 0000000000000000 RSI: 00000000ffffffea RDI: 0000000000000001
[ 2.023555] RBP: ffff888004c20098 R08: ffffffff82b392c8 R09: 00000000ffffdfff
[ 2.023693] R10: ffffffff82a592e0 R11: ffffffff82b092e0 R12: ffff888004c200d8
[ 2.023813] R13: 0000000000000000 R14: ffff888004c20000 R15: ffffc90000013ca8
[ 2.023930] FS: 0000000000000000(0000) GS:ffff88807dc00000(0000) knlGS:0000000000000000
[ 2.024062] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2.024161] CR2: ffff888003601000 CR3: 0000000002a2e000 CR4: 00000000000006f0
[ 2.024275] Call Trace:
[ 2.024322] <TASK>
[ 2.024367] ? __warn+0x7f/0x130
[ 2.024430] ? refcount_warn_saturate+0xbe/0x110
[ 2.024513] ? report_bug+0x199/0x1b0
[ 2.024585] ? handle_bug+0x3c/0x70
[ 2.024676] ? exc_invalid_op+0x18/0x70
[ 2.024750] ? asm_exc_invalid_op+0x1a/0x20
[ 2.024830] ? refcount_warn_saturate+0xbe/0x110
[ 2.024916] ? refcount_warn_saturate+0xbe/0x110
[ 2.024998] __tcp_close+0x2f4/0x3d0
[ 2.025065] ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10
[ 2.025168] tcp_close+0x1f/0x70
[ 2.025231] inet_release+0x33/0x60
[ 2.025297] sock_release+0x1f/0x80
[ 2.025361] handshake_req_cancel_test2+0x100/0x2d0
[ 2.025457] kunit_try_run_case+0x4c/0xa0
[ 2.025532] kunit_generic_run_threadfn_adapter+0x15/0x20
[ 2.025644] kthread+0xe1/0x110
[ 2.025708] ? __pfx_kthread+0x10/0x10
[ 2.025780] ret_from_fork+0x2c/0x50
One can enable CONFIG_NET_HANDSHAKE_KUNIT_TEST config to reproduce above
crash.
The root cause of this bug is that the commit 1ce77c998f04
("net/handshake: Unpin sock->file if a handshake is cancelled") adds one
additional fput() function. That patch claims that the fput() is used to
enable sock->file to be freed even when user space never calls DONE.
However, it seems that the intended DONE routine will never give an
additional fput() of ths sock->file. The existing two of them are just
used to balance the reference added in sockfd_lookup().
This patch revert the mentioned commit to avoid the use-after-free. The
patched kernel could successfully pass the KUNIT test and boot to shell.
[ 0.733613] # Subtest: Handshake API tests
[ 0.734029] 1..11
[ 0.734255] KTAP version 1
[ 0.734542] # Subtest: req_alloc API fuzzing
[ 0.736104] ok 1 handshake_req_alloc NULL proto
[ 0.736114] ok 2 handshake_req_alloc CLASS_NONE
[ 0.736559] ok 3 handshake_req_alloc CLASS_MAX
[ 0.737020] ok 4 handshake_req_alloc no callbacks
[ 0.737488] ok 5 handshake_req_alloc no done callback
[ 0.737988] ok 6 handshake_req_alloc excessive privsize
[ 0.738529] ok 7 handshake_req_alloc all good
[ 0.739036] # req_alloc API fuzzing: pass:7 fail:0 skip:0 total:7
[ 0.739444] ok 1 req_alloc API fuzzing
[ 0.740065] ok 2 req_submit NULL req arg
[ 0.740436] ok 3 req_submit NULL sock arg
[ 0.740834] ok 4 req_submit NULL sock->file
[ 0.741236] ok 5 req_lookup works
[ 0.741621] ok 6 req_submit max pending
[ 0.741974] ok 7 req_submit multiple
[ 0.742382] ok 8 req_cancel before accept
[ 0.742764] ok 9 req_cancel after accept
[ 0.743151] ok 10 req_cancel after done
[ 0.743510] ok 11 req_destroy works
[ 0.743882] # Handshake API tests: pass:11 fail:0 skip:0 total:11
[ 0.744205] # Totals: pass:17 fail:0 skip:0 total:17
Fixes: 1ce77c998f04 ("net/handshake: Unpin sock->file if a handshake is cancelled")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
net/handshake/handshake.h | 1 -
net/handshake/request.c | 4 ----
2 files changed, 5 deletions(-)
diff --git a/net/handshake/handshake.h b/net/handshake/handshake.h
index 8aeaadca844f..4dac965c99df 100644
--- a/net/handshake/handshake.h
+++ b/net/handshake/handshake.h
@@ -31,7 +31,6 @@ struct handshake_req {
struct list_head hr_list;
struct rhash_head hr_rhash;
unsigned long hr_flags;
- struct file *hr_file;
const struct handshake_proto *hr_proto;
struct sock *hr_sk;
void (*hr_odestruct)(struct sock *sk);
diff --git a/net/handshake/request.c b/net/handshake/request.c
index d78d41abb3d9..94d5cef3e048 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -239,7 +239,6 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
}
req->hr_odestruct = req->hr_sk->sk_destruct;
req->hr_sk->sk_destruct = handshake_sk_destruct;
- req->hr_file = sock->file;
ret = -EOPNOTSUPP;
net = sock_net(req->hr_sk);
@@ -335,9 +334,6 @@ bool handshake_req_cancel(struct sock *sk)
return false;
}
- /* Request accepted and waiting for DONE */
- fput(req->hr_file);
-
out_true:
trace_handshake_cancel(net, req, sk);
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1] net/handshake: remove fput() that causes use-after-free
2023-06-13 8:32 [PATCH v1] net/handshake: remove fput() that causes use-after-free Lin Ma
@ 2023-06-13 14:00 ` Chuck Lever III
2023-06-13 18:03 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: Chuck Lever III @ 2023-06-13 14:00 UTC (permalink / raw)
To: Lin Ma
Cc: David S. Miller, Eric Dumazet, kuba@kernel.org,
kernel-tls-handshake@lists.linux.dev
> On Jun 13, 2023, at 4:32 AM, Lin Ma <linma@zju.edu.cn> wrote:
>
> A reference underflow is found in TLS handshake subsystem that causes a
> direct use-after-free. Part of the crash log is like below:
>
> [ 2.022114] ------------[ cut here ]------------
> [ 2.022193] refcount_t: underflow; use-after-free.
> [ 2.022288] WARNING: CPU: 0 PID: 60 at lib/refcount.c:28 refcount_warn_saturate+0xbe/0x110
> [ 2.022432] Modules linked in:
> [ 2.022848] RIP: 0010:refcount_warn_saturate+0xbe/0x110
> [ 2.023231] RSP: 0018:ffffc900001bfe18 EFLAGS: 00000286
> [ 2.023325] RAX: 0000000000000000 RBX: 0000000000000007 RCX: 00000000ffffdfff
> [ 2.023438] RDX: 0000000000000000 RSI: 00000000ffffffea RDI: 0000000000000001
> [ 2.023555] RBP: ffff888004c20098 R08: ffffffff82b392c8 R09: 00000000ffffdfff
> [ 2.023693] R10: ffffffff82a592e0 R11: ffffffff82b092e0 R12: ffff888004c200d8
> [ 2.023813] R13: 0000000000000000 R14: ffff888004c20000 R15: ffffc90000013ca8
> [ 2.023930] FS: 0000000000000000(0000) GS:ffff88807dc00000(0000) knlGS:0000000000000000
> [ 2.024062] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 2.024161] CR2: ffff888003601000 CR3: 0000000002a2e000 CR4: 00000000000006f0
> [ 2.024275] Call Trace:
> [ 2.024322] <TASK>
> [ 2.024367] ? __warn+0x7f/0x130
> [ 2.024430] ? refcount_warn_saturate+0xbe/0x110
> [ 2.024513] ? report_bug+0x199/0x1b0
> [ 2.024585] ? handle_bug+0x3c/0x70
> [ 2.024676] ? exc_invalid_op+0x18/0x70
> [ 2.024750] ? asm_exc_invalid_op+0x1a/0x20
> [ 2.024830] ? refcount_warn_saturate+0xbe/0x110
> [ 2.024916] ? refcount_warn_saturate+0xbe/0x110
> [ 2.024998] __tcp_close+0x2f4/0x3d0
> [ 2.025065] ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10
> [ 2.025168] tcp_close+0x1f/0x70
> [ 2.025231] inet_release+0x33/0x60
> [ 2.025297] sock_release+0x1f/0x80
> [ 2.025361] handshake_req_cancel_test2+0x100/0x2d0
> [ 2.025457] kunit_try_run_case+0x4c/0xa0
> [ 2.025532] kunit_generic_run_threadfn_adapter+0x15/0x20
> [ 2.025644] kthread+0xe1/0x110
> [ 2.025708] ? __pfx_kthread+0x10/0x10
> [ 2.025780] ret_from_fork+0x2c/0x50
>
> One can enable CONFIG_NET_HANDSHAKE_KUNIT_TEST config to reproduce above
> crash.
>
> The root cause of this bug is that the commit 1ce77c998f04
> ("net/handshake: Unpin sock->file if a handshake is cancelled") adds one
> additional fput() function. That patch claims that the fput() is used to
> enable sock->file to be freed even when user space never calls DONE.
>
> However, it seems that the intended DONE routine will never give an
> additional fput() of ths sock->file. The existing two of them are just
> used to balance the reference added in sockfd_lookup().
>
> This patch revert the mentioned commit to avoid the use-after-free. The
> patched kernel could successfully pass the KUNIT test and boot to shell.
>
> [ 0.733613] # Subtest: Handshake API tests
> [ 0.734029] 1..11
> [ 0.734255] KTAP version 1
> [ 0.734542] # Subtest: req_alloc API fuzzing
> [ 0.736104] ok 1 handshake_req_alloc NULL proto
> [ 0.736114] ok 2 handshake_req_alloc CLASS_NONE
> [ 0.736559] ok 3 handshake_req_alloc CLASS_MAX
> [ 0.737020] ok 4 handshake_req_alloc no callbacks
> [ 0.737488] ok 5 handshake_req_alloc no done callback
> [ 0.737988] ok 6 handshake_req_alloc excessive privsize
> [ 0.738529] ok 7 handshake_req_alloc all good
> [ 0.739036] # req_alloc API fuzzing: pass:7 fail:0 skip:0 total:7
> [ 0.739444] ok 1 req_alloc API fuzzing
> [ 0.740065] ok 2 req_submit NULL req arg
> [ 0.740436] ok 3 req_submit NULL sock arg
> [ 0.740834] ok 4 req_submit NULL sock->file
> [ 0.741236] ok 5 req_lookup works
> [ 0.741621] ok 6 req_submit max pending
> [ 0.741974] ok 7 req_submit multiple
> [ 0.742382] ok 8 req_cancel before accept
> [ 0.742764] ok 9 req_cancel after accept
> [ 0.743151] ok 10 req_cancel after done
> [ 0.743510] ok 11 req_destroy works
> [ 0.743882] # Handshake API tests: pass:11 fail:0 skip:0 total:11
> [ 0.744205] # Totals: pass:17 fail:0 skip:0 total:17
>
>
> Fixes: 1ce77c998f04 ("net/handshake: Unpin sock->file if a handshake is cancelled")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>
Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
Thank you for the report and fix, Lin Ma.
It's becoming quite clear to me that running Kunit with the uml helper
is entirely different than enabling Kunit to run at boot time. The
former is an inadequate test.
> ---
> net/handshake/handshake.h | 1 -
> net/handshake/request.c | 4 ----
> 2 files changed, 5 deletions(-)
>
> diff --git a/net/handshake/handshake.h b/net/handshake/handshake.h
> index 8aeaadca844f..4dac965c99df 100644
> --- a/net/handshake/handshake.h
> +++ b/net/handshake/handshake.h
> @@ -31,7 +31,6 @@ struct handshake_req {
> struct list_head hr_list;
> struct rhash_head hr_rhash;
> unsigned long hr_flags;
> - struct file *hr_file;
> const struct handshake_proto *hr_proto;
> struct sock *hr_sk;
> void (*hr_odestruct)(struct sock *sk);
> diff --git a/net/handshake/request.c b/net/handshake/request.c
> index d78d41abb3d9..94d5cef3e048 100644
> --- a/net/handshake/request.c
> +++ b/net/handshake/request.c
> @@ -239,7 +239,6 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
> }
> req->hr_odestruct = req->hr_sk->sk_destruct;
> req->hr_sk->sk_destruct = handshake_sk_destruct;
> - req->hr_file = sock->file;
>
> ret = -EOPNOTSUPP;
> net = sock_net(req->hr_sk);
> @@ -335,9 +334,6 @@ bool handshake_req_cancel(struct sock *sk)
> return false;
> }
>
> - /* Request accepted and waiting for DONE */
> - fput(req->hr_file);
> -
> out_true:
> trace_handshake_cancel(net, req, sk);
>
> --
> 2.34.1
>
--
Chuck Lever
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1] net/handshake: remove fput() that causes use-after-free
2023-06-13 8:32 [PATCH v1] net/handshake: remove fput() that causes use-after-free Lin Ma
2023-06-13 14:00 ` Chuck Lever III
@ 2023-06-13 18:03 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: Jakub Kicinski @ 2023-06-13 18:03 UTC (permalink / raw)
To: Lin Ma; +Cc: chuck.lever, davem, edumazet, kernel-tls-handshake
On Tue, 13 Jun 2023 16:32:04 +0800 Lin Ma wrote:
> A reference underflow is found in TLS handshake subsystem that causes a
> direct use-after-free. Part of the crash log is like below:
Thanks for the patch, could you repost CCing netdev@ ?
We need that for the patch to make it to patchwork.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-13 18:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-13 8:32 [PATCH v1] net/handshake: remove fput() that causes use-after-free Lin Ma
2023-06-13 14:00 ` Chuck Lever III
2023-06-13 18:03 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox