From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Dumazet <eric.dumazet@gmail.com>
Subject: Re: Crash due to destroying TCP request sockets using SOCK_DESTROY
Date: Fri, 6 Jul 2018 03:24:13 -0700
Message-ID: <af52b285-e97b-f610-acc6-cb1361609034@gmail.com>
References: <95aaa3d59cb3c9cc11b6b83880d92fec@codeaurora.org>
 <CAKD1Yr0aVtWt_oFaeMPO_p1VAZNNQ2zdEQx-BDH_JNUTk38-ww@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, Eric Dumazet <eric.dumazet@gmail.com>,
        Alistair Strachan <astrachan@google.com>,
        David Ahern <dsa@cumulusnetworks.com>
To: Lorenzo Colitti <lorenzo@google.com>,
        Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pf0-f193.google.com ([209.85.192.193]:34344 "EHLO
        mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753428AbeGFKYQ (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 6 Jul 2018 06:24:16 -0400
Received: by mail-pf0-f193.google.com with SMTP id e10-v6so8088867pfn.1
        for <netdev@vger.kernel.org>; Fri, 06 Jul 2018 03:24:15 -0700 (PDT)
In-Reply-To: <CAKD1Yr0aVtWt_oFaeMPO_p1VAZNNQ2zdEQx-BDH_JNUTk38-ww@mail.gmail.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>



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 !