From: Lorenzo Colitti <hidden> Date: 2018-07-06 04:46:48
On Fri, Jul 6, 2018 at 11:37 AM Subash Abhinov Kasiviswanathan
[off-list ref] wrote:
From the call stack, a TCP socket is being destroyed using netlink_diag.
The memory dump showed that the socket was an inet request socket (in
state TCP_NEW_SYN_RECV) with refcount of 0.
[...]
13232.479820: <2> refcount_t: underflow; use-after-free.
13232.479838: <6> ------------[ cut here ]------------
13232.479843: <6> kernel BUG at kernel/msm-4.14/lib/refcount.c:204!
13232.479849: <6> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[...]
13232.479996: <6> Process netd (pid: 648, stack limit =
0xffffff801cf98000)
13232.479998: <2> Call trace:
13232.480000: <2> refcount_sub_and_test+0x64/0x78
13232.480002: <2> refcount_dec_and_test+0x18/0x24
13232.480005: <2> sock_gen_put+0x1c/0xb0
13232.480009: <2> tcp_diag_destroy+0x54/0x68
[...]
Looks like for a TCP_NEW_SYN_RECV socket, sock_diag_destroy
essentially ends up doing:
struct request_sock *req = inet_reqsk(sk);
local_bh_disable();
inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
req);
local_bh_enable();
...
sock_gen_put(sk);
It looks like inet_csk_reqsk_queue_drop_and_put calls reqsk_put(req),
which frees the socket, and at that point sock_gen_put is a UAF. Do we
just need:
- inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
- req);
+ inet_csk_reqsk_queue_drop(req->rsk_listener, req);
since sock_gen_put will also end up calling reqsk_put() for a
TCP_SYN_RECV socket?
Alastair - you're able to reproduce this UAF using net_test on qemu,
right? If so, could you try that two-line patch above?
From: Eric Dumazet <hidden> Date: 2018-07-06 10:24:16
On 07/05/2018 09:46 PM, Lorenzo Colitti wrote:
On Fri, Jul 6, 2018 at 11:37 AM Subash Abhinov Kasiviswanathan
[off-list ref] wrote:
quoted
From the call stack, a TCP socket is being destroyed using netlink_diag.
The memory dump showed that the socket was an inet request socket (in
state TCP_NEW_SYN_RECV) with refcount of 0.
[...]
13232.479820: <2> refcount_t: underflow; use-after-free.
13232.479838: <6> ------------[ cut here ]------------
13232.479843: <6> kernel BUG at kernel/msm-4.14/lib/refcount.c:204!
13232.479849: <6> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[...]
13232.479996: <6> Process netd (pid: 648, stack limit =
0xffffff801cf98000)
13232.479998: <2> Call trace:
13232.480000: <2> refcount_sub_and_test+0x64/0x78
13232.480002: <2> refcount_dec_and_test+0x18/0x24
13232.480005: <2> sock_gen_put+0x1c/0xb0
13232.480009: <2> tcp_diag_destroy+0x54/0x68
[...]
Looks like for a TCP_NEW_SYN_RECV socket, sock_diag_destroy
essentially ends up doing:
struct request_sock *req = inet_reqsk(sk);
local_bh_disable();
inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
req);
local_bh_enable();
...
sock_gen_put(sk);
It looks like inet_csk_reqsk_queue_drop_and_put calls reqsk_put(req),
which frees the socket, and at that point sock_gen_put is a UAF. Do we
just need:
- inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
- req);
+ inet_csk_reqsk_queue_drop(req->rsk_listener, req);
since sock_gen_put will also end up calling reqsk_put() for a
TCP_SYN_RECV socket?
Alastair - you're able to reproduce this UAF using net_test on qemu,
right? If so, could you try that two-line patch above?
Hi Lorenzo
Your patch makes sense to me, please submit it formally with :
Fixes: d7226c7a4dd1 ("net: diag: Fix refcnt leak in error path destroying socket")
Cc: David Ahern <redacted>
Thanks !
Looks like for a TCP_NEW_SYN_RECV socket, sock_diag_destroy
essentially ends up doing:
struct request_sock *req = inet_reqsk(sk);
local_bh_disable();
inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
req);
local_bh_enable();
...
sock_gen_put(sk);
It looks like inet_csk_reqsk_queue_drop_and_put calls reqsk_put(req),
which frees the socket, and at that point sock_gen_put is a UAF. Do we
just need:
-
inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
- req);
+ inet_csk_reqsk_queue_drop(req->rsk_listener,
req);
since sock_gen_put will also end up calling reqsk_put() for a
TCP_SYN_RECV socket?
Alastair - you're able to reproduce this UAF using net_test on qemu,
right? If so, could you try that two-line patch above?
Hi Lorenzo
Your patch makes sense to me, please submit it formally with :
Fixes: d7226c7a4dd1 ("net: diag: Fix refcnt leak in error path
destroying socket")
Cc: David Ahern <redacted>
Thanks !
Thanks Lorenzo and Eric. I will try it out locally.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Lorenzo Colitti <hidden> Date: 2018-07-07 07:34:43
On Fri, Jul 6, 2018 at 7:24 PM Eric Dumazet [off-list ref] wrote:
Your patch makes sense to me, please submit it formally with :
Fixes: d7226c7a4dd1 ("net: diag: Fix refcnt leak in error path destroying socket")
Cc: David Ahern <redacted>