Crash due to destroying TCP request sockets using SOCK_DESTROY

Crash due to destroying TCP request sockets using SOCK_DESTROY

[Subash Abhinov Kasiviswanathan]

We are seeing a crash on an ARM64 device with Android 4.14 based kernel.

 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.

The crash seems to have happened during a regression test where wifi
was toggled with some browser activity but it is not very easily
reproducible. I believe netd on Android tries to destroy all sockets in
a system on change of network.

  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.479895:   <6> CPU: 4 PID: 648 Comm: netd Tainted: G S      W  O   
  4.14.49+ #1
  13232.479897:   <6> task: fffffff5d6e28080 task.stack: ffffff801cf98000
  13232.479908:   <2> pc : refcount_sub_and_test+0x64/0x78
  13232.479910:   <2> lr : refcount_sub_and_test+0x64/0x78
  13232.479911:   <2> sp : ffffff801cf9ba40 pstate : 20400145
  13232.479911:   <2> x29: ffffff801cf9ba40 x28: fffffff5d6e28080
  13232.479914:   <2> x27: ffffff801cf9bd10 x26: fffffff4a1428f40
  13232.479915:   <2> x25: 0000000000000000 x24: ffffffffffffff91
  13232.479917:   <2> x23: 0000000000000015 x22: fffffff5b837c880
  13232.479919:   <2> x21: fffffff4a1428f40 x20: 0000000000000000
  13232.479920:   <2> x19: fffffff4c47c6088 x18: e7b13cd1ecbfea00
  13232.479922:   <2> x17: 00000008ec3bb553 x16: 011d8776aa792786
  13232.479924:   <2> x15: e7b13cd1ecbfea00 x14: 000000002bdb7692
  13232.479925:   <2> x13: 0000000000000000 x12: e7b13cd1ecbfea00
  13232.479927:   <2> x11: e7b13cd1ecbfea00 x10: 0000000000000000
  13232.479928:   <2> x9 : e7b13cd1ecbfea00 x8 : 0000000000000000
  13232.479929:   <2> x7 : 0000000000000001 x6 : 0000000000000001
  13232.479931:   <2> x5 : 0000000000000000 x4 : 00000c08ed425d69
  13232.479932:   <2> x3 : 00000066effb6000 x2 : ffffff8f09dc5000
  13232.479934:   <2> x1 : 0000000000000000 x0 : 0000000000000026
  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
  13232.480010:   <2>  inet_diag_cmd_exact+0x78/0xa0
  13232.480012:   <2>  inet_diag_handler_cmd+0xcc/0xf8
  13232.480018:   <2>  sock_diag_rcv_msg+0x130/0x158
  13232.480021:   <2>  netlink_rcv_skb+0xa4/0x11c
  13232.480023:   <2>  sock_diag_rcv+0x34/0x48
  13232.480025:   <2>  netlink_unicast+0x158/0x1f0
  13232.480026:   <2>  netlink_sendmsg+0x334/0x340
  13232.480028:   <2>  sock_sendmsg+0x44/0x60
  13232.480031:   <2>  sock_write_iter+0xac/0xf4
  13232.480034:   <2>  __vfs_write+0x124/0x154
  13232.480036:   <2>  vfs_write+0xcc/0x188
  13232.480038:   <2>  SyS_write+0x60/0xc0
  13232.480040:   <2>  el0_svc_naked+0x34/0x38
  13232.480042:   <6> Code: 910003fd f0008200 910fd000 97f4158c 
(d4210000)
  13232.480045:   <6> ---[ end trace 994bad5b8077e394 ]---
  13232.480061:   <6> Kernel panic - not syncing: Fatal exception

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: Crash due to destroying TCP request sockets using SOCK_DESTROY

[Lorenzo Colitti]

On Fri, Jul 6, 2018 at 11:37 AM Subash Abhinov Kasiviswanathan
<subashab@codeaurora.org> 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?

Re: Crash due to destroying TCP request sockets using SOCK_DESTROY

[Eric Dumazet]

On 07/05/2018 09:46 PM, Lorenzo Colitti wrote:
> On Fri, Jul 6, 2018 at 11:37 AM Subash Abhinov Kasiviswanathan
> <subashab@codeaurora.org> 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?
> 

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 <dsa@cumulusnetworks.com>

Thanks !

Re: Crash due to destroying TCP request sockets using SOCK_DESTROY

[Subash Abhinov Kasiviswanathan]

>> 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 <dsa@cumulusnetworks.com>
> 
> 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

Re: Crash due to destroying TCP request sockets using SOCK_DESTROY

[Lorenzo Colitti]

On Fri, Jul 6, 2018 at 7:24 PM Eric Dumazet <eric.dumazet@gmail.com> 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 <dsa@cumulusnetworks.com>

Submitted a patch against net: https://patchwork.ozlabs.org/patch/940757/ .

Cheers,
Lorenzo