From: "Wangshaobo (bobo)" <bobo.shaobowang@huawei.com>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: <cj.chengjian@huawei.com>, Wei Yongjun <weiyongjun1@huawei.com>,
<yuehaibing@huawei.com>, <huawei.libin@huawei.com>,
Marcel Holtmann <marcel@holtmann.org>,
Johan Hedberg <johan.hedberg@gmail.com>,
"linux-bluetooth@vger.kernel.org"
<linux-bluetooth@vger.kernel.org>,
"open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] Bluetooth: fix use-after-free error in lock_sock_nested()
Date: Mon, 19 Jul 2021 10:09:41 +0800 [thread overview]
Message-ID: <50a139c2-6252-1881-c253-cc548bca1947@huawei.com> (raw)
In-Reply-To: <CABBYNZKy28hfo811zMB6Z=TEXrUn_JCkpehE7n_a7Cx10qBa8g@mail.gmail.com>
>>> 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
prev parent reply other threads:[~2021-07-19 2:09 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-14 3:17 [PATCH] Bluetooth: fix use-after-free error in lock_sock_nested() Wang ShaoBo
2021-07-14 4:10 ` bluez.test.bot
2021-07-14 21:50 ` [PATCH] " Luiz Augusto von Dentz
2021-07-15 4:57 ` Wangshaobo (bobo)
2021-07-16 1:37 ` Wangshaobo (bobo)
2021-07-16 17:22 ` Luiz Augusto von Dentz
2021-07-19 2:09 ` Wangshaobo (bobo) [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50a139c2-6252-1881-c253-cc548bca1947@huawei.com \
--to=bobo.shaobowang@huawei.com \
--cc=cj.chengjian@huawei.com \
--cc=huawei.libin@huawei.com \
--cc=johan.hedberg@gmail.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=netdev@vger.kernel.org \
--cc=weiyongjun1@huawei.com \
--cc=yuehaibing@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).