Re: KASAN: use-after-free Read in hci_chan_del

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