Re: KASAN: use-after-free Read in hci_chan_del
From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-06-07 07:55:37
Also in:
lkml, netdev
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:quoted
On Sun, Jun 06, 2021 at 04:50:04PM +0800, Hillf Danton wrote:quoted
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.
quoted hunk
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.
quoted hunk
+ +#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.
quoted hunk
+ 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