netdev - Re: [PATCH] Bluetooth: fix use-after-free error in lock_sock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 19 Jul 2021 10:09:41 +0800
From:   "Wangshaobo (bobo)" <bobo.shaobowang@...wei.com>
To:     Luiz Augusto von Dentz <luiz.dentz@...il.com>
CC:     <cj.chengjian@...wei.com>, Wei Yongjun <weiyongjun1@...wei.com>,
        <yuehaibing@...wei.com>, <huawei.libin@...wei.com>,
        Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Bluetooth: fix use-after-free error in lock_sock_nested()


>>> Hi,
>>>
>>> In my case it looks OK, this is the diff:
>>>
>>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>>> index f1b1edd0b697..32ef3328ab49 100644
>>> --- a/net/bluetooth/l2cap_sock.c
>>> +++ b/net/bluetooth/l2cap_sock.c
>>> @@ -1500,6 +1500,9 @@ static void l2cap_sock_close_cb(struct
>>> l2cap_chan *chan)
>>>   {
>>>          struct sock *sk = chan->data;
>>>
>>> +       if (!sk)
>>> +               return;
>>> +
>>>          l2cap_sock_kill(sk);
>>>   }
>>>
>>> @@ -1508,6 +1511,9 @@ static void l2cap_sock_teardown_cb(struct
>>> l2cap_chan *chan, int err)
>>>          struct sock *sk = chan->data;
>>>          struct sock *parent;
>>>
>>> +       if (!sk)
>>> +               return;
>>> +
>>>          BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
>>>
>>>          /* This callback can be called both for server (BT_LISTEN)
>>> @@ -1700,6 +1706,7 @@ static void l2cap_sock_destruct(struct sock *sk)
>>>          BT_DBG("sk %p", sk);
>>>
>>>          if (l2cap_pi(sk)->chan)
>>> +              l2cap_pi(sk)->chan->data = NULL;
>>>                   l2cap_chan_put(l2cap_pi(sk)->chan);
>>>
>>> But if it has potential risk if l2cap_sock_destruct() can not be
>>> excuted in time ?
>>>
>>> sk_free():
>>>
>>>          if (refcount_dec_and_test(&sk->sk_wmem_alloc)) //is possible
>>> this condition false ?
>>>
>>>                __sk_free(sk)   -> ... l2cap_sock_destruct()
>>>
>> Dear Luiz,
>>
>> Not only that, if l2cap_sock_kill() has put 'l2cap_pi(sk)->chan', how
>> does we avoid re-puting 'l2cap_pi(sk)->chan' if l2cap_sock_destruct()
>> work postponed? this will cause underflow of chan->refcount; this PATCH
>> 4e1a720d0312 ("Bluetooth: avoid killing an already killed socket") also
>> may not work in any case because only sock_orphan() has excuted can this
>> sock be killed, but if sco_sock_release() excute first, for this sock
>> has been marked as SOCK_DEAD, this sock can never be killed. So should
>> we think put chan->data = NULL in xx_sock_kill() is a better choice ?
> Not sure what do you mean by postponed? Interrupted perhaps? Even in
> that case what are trying to prevent is use after free so if the
> callback has not run yet that means the sk has not been freed. Anyway
> I think we could do it inconditionally in l2cap_sock_kill since we
> will be releasing the reference owned by l2cap_pi(sk)->chan->data that
> should be reset to NULL immediatelly.

Dear  Luiz,

yes, that's right, if sk can be accessed, it also means that chan has 
not been destroyed, thanks very much.

-- Wang ShaoBo