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
