From: Greg KH <gregkh@linuxfoundation.org>
To: Hillf Danton <hdanton@sina.com>
Cc: Leon Romanovsky <leon@kernel.org>, SyzScope <syzscope@gmail.com>,
davem@davemloft.net, johan.hedberg@gmail.com, kuba@kernel.org,
linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
marcel@holtmann.org, netdev@vger.kernel.org,
syzkaller-bugs@googlegroups.com
Subject: Re: KASAN: use-after-free Read in hci_chan_del
Date: Mon, 7 Jun 2021 09:55:31 +0200 [thread overview]
Message-ID: <YL3Q848EVIdkUrF4@kroah.com> (raw)
In-Reply-To: <20210607074828.3259-1-hdanton@sina.com>
On Mon, Jun 07, 2021 at 03:48:28PM +0800, Hillf Danton wrote:
> On Sun, 6 Jun 2021 11:54:22 +0200 Greg KH wrote:
> >On Sun, Jun 06, 2021 at 04:50:04PM +0800, Hillf Danton wrote:
> >>
> >> To fix the uaf reported, add reference count to hci channel to track users.
> >> Then only channels with zero users will be released.
> >>
> >> It is now only for thoughts.
> >>
> >> +++ x/include/net/bluetooth/hci_core.h
> >> @@ -704,6 +704,7 @@ struct hci_chan {
> >> struct sk_buff_head data_q;
> >> unsigned int sent;
> >> __u8 state;
> >> + atomic_t ref;
> >
> >Please no, never use "raw" atomic variables. Especially for something
> >like this, use a kref.
>
> Fair, thanks for taking a look at it.
>
> Spin with care for the race the added ref fails to cut.
I do not understand what you mean here.
> To ease review the full syzreport is also attached.
>
> To fix uaf, add user track to hci channel and we will only release channel if
> its user hits zero. And a dryrun mechanism is also added to take care of the
> race user track fails to cut.
>
> CPU0 CPU1
> ---- ----
> hci_chan_del l2cap_conn_del
> chan->user = 0;
>
> if (chan->user != 0)
> return;
> synchronize_rcu();
> kfree(chan);
>
> hci_chan_del();
>
> It is now only for thoughts.
>
> +++ x/include/net/bluetooth/hci_core.h
> @@ -704,6 +704,10 @@ struct hci_chan {
> struct sk_buff_head data_q;
> unsigned int sent;
> __u8 state;
> + __u8 user;
No.
> + __u8 release;
No please no.
> +
> +#define HCHAN_RELEASE_DRYRUN 1
> };
>
> struct hci_conn_params {
> +++ x/net/bluetooth/l2cap_core.c
> @@ -1903,6 +1903,12 @@ static void l2cap_conn_del(struct hci_co
>
> mutex_unlock(&conn->chan_lock);
>
> + /* see comment in hci_chan_del() */
> + conn->hchan->release = HCHAN_RELEASE_DRYRUN;
> + smp_wmb();
> + conn->hchan->user--;
And the reason you are open-coding a kref is why???
Please again no.
> + hci_chan_del(conn->hchan);
> + conn->hchan->release = 0;
> hci_chan_del(conn->hchan);
>
> if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> @@ -7716,6 +7722,8 @@ static struct l2cap_conn *l2cap_conn_add
> kref_init(&conn->ref);
> hcon->l2cap_data = conn;
> conn->hcon = hci_conn_get(hcon);
> + /* dec in l2cap_conn_del() */
> + hchan->user++;
{sigh}
No, there is a reason we wrote kref many _decades_ ago. Please use it,
your original attempt with an atomic was just fine, just use the proper
data structures the kernel provides you as this is obviously a reference
counted object.
thanks,
greg k-h
next prev parent reply other threads:[~2021-06-07 7:55 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-02 20:45 KASAN: use-after-free Read in hci_chan_del syzbot
2020-08-03 17:08 ` syzbot
2021-05-04 21:50 ` ETenal
2021-05-06 6:01 ` Dan Carpenter
2021-05-06 6:42 ` SyzScope
2021-06-04 9:48 ` Greg KH
2021-06-04 17:11 ` SyzScope
2021-06-05 7:43 ` Greg KH
2021-06-05 18:12 ` SyzScope
2021-06-06 5:16 ` Greg KH
2021-06-06 5:29 ` Leon Romanovsky
2021-06-06 5:06 ` Leon Romanovsky
[not found] ` <20210606085004.12212-1-hdanton@sina.com>
2021-06-06 9:54 ` Greg KH
[not found] ` <20210607074828.3259-1-hdanton@sina.com>
2021-06-07 7:55 ` Greg KH [this message]
[not found] ` <20210607100201.3345-1-hdanton@sina.com>
2021-06-07 10:31 ` Greg KH
[not found] ` <20210608081800.3484-1-hdanton@sina.com>
2021-06-08 8:40 ` Greg KH
2021-05-28 21:12 ` SyzScope
2021-06-03 18:30 ` SyzScope
2021-06-03 18:36 ` Greg KH
2021-06-07 10:21 ` Jason A. Donenfeld
2021-06-07 10:28 ` Dmitry Vyukov
2021-06-07 11:20 ` Greg KH
2021-06-07 18:26 ` SyzScope
2021-06-08 8:46 ` Greg KH
2021-06-08 8:53 ` Dan Carpenter
2021-06-07 22:25 ` [syzbot] " syzbot
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=YL3Q848EVIdkUrF4@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=davem@davemloft.net \
--cc=hdanton@sina.com \
--cc=johan.hedberg@gmail.com \
--cc=kuba@kernel.org \
--cc=leon@kernel.org \
--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=syzkaller-bugs@googlegroups.com \
--cc=syzscope@gmail.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).