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

On Tue, Jun 08, 2021 at 04:18:00PM +0800, Hillf Danton wrote:

On Mon, 7 Jun 2021 12:31:39 +0200 Greg KH wrote:

quoted

On Mon, Jun 07, 2021 at 06:02:01PM +0800, Hillf Danton wrote:

quoted

After taking another look at the added user track, I realised that it serves
no more than a one-off state word that prevents channel from being released.
Then the race behind the uaf can be fixed by adding a state on top of the
dryrun introduced even without tracking users.

The state machine works as the following,
1) it is initialised to be backoff that means channel cannot be released
   at the moment.
2) it is changed to be dryrun on releasing to cut the race that survived
   backoff.
3) it is finally set to zero for release after cutting the chance for race.

Adding another state on top of this feels rough, does it really solve
the race here?

No, frankly, given the list_del_rcu() in hci_chan_del().

quoted

Normally a reference count should be enough to properly
tear things down when needed, rolling back from a "can I try this now"
state still seems racy without the needed lock somewhere.

The rollback is added only for making sure that the channel released in
l2cap_conn_del() would not be freed in the other pathes. That exclusiveness
adds more barriers than thought to fixing the rare race with kref and spinlock
in the usual and simple manner.

If OTOH channel is created with the exclusiveness taken into account by adding
the exclusive create and delete methods for l2cap, then the race can be fixed
by checking the exclusive mark in addition to aquiring the hdev lock at release
time.

One would think that the state machine for the channel would fix this
whole mess, why do we need an "additional" state here in the first
place?

Would be nice if one of the bluetooth maintainers weighed in on this...

thanks,

greg k-h