Re: Re: [PATCH 5.4 39/78] Bluetooth: use correct lock to prevent UAF of hdev object

[Anand K. Mistry]

On Thu, 17 Jun 2021 at 22:37, LinMa <linma@xxxxxxxxxx> wrote:
>
>
> Oops, sorry for the delay here. I just forgot to check the mails.
>
> This comment is right, when I submit this patch I mentioned that the replacement of this lock can hang the detaching routine because it needs to wait the release of the lock_sock().
>
> But this does no harm in my testing. In fact, the relevant code can only be executed when removing the controller. I think it can wait for the lock. Moreover, this patch can fix the potential UAF indeed.
>
> > may need further discussion. (wrote in previous mail list
>
> Welcome the additional advise on this. Does this really broken the lock principle?

One more data point. I'm seeing this 100% of the time when trying the
suspend my system (on 5.10):

[  466.608970] BUG: sleeping function called from invalid context at
net/core/sock.c:3074
[  466.608975] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid:
5614, name: kworker/u4:4
[  466.608980] CPU: 1 PID: 5614 Comm: kworker/u4:4 Tainted: G        W
        5.10.43 #64
[  466.608983] Hardware name: HP Grunt/Grunt, BIOS
Google_Grunt.11031.104.0 09/05/2019
[  466.608991] Workqueue: events_unbound async_run_entry_fn
[  466.608995] Call Trace:
[  466.609003]  dump_stack+0x9c/0xe7
[  466.609009]  ___might_sleep+0x148/0x15e
[  466.609013]  lock_sock_nested+0x22/0x5d
[  466.609033]  hci_sock_dev_event+0x15a/0x1f0 [bluetooth]
[  466.609043]  hci_unregister_dev+0x15c/0x303 [bluetooth]
[  466.609049]  btusb_disconnect+0x77/0x127 [btusb]
[  466.609054]  usb_unbind_interface+0xa6/0x22e
[  466.609059]  ? usb_dev_suspend+0x14/0x14
[  466.609063]  device_release_driver_internal+0x100/0x1a1
[  466.609067]  unbind_marked_interfaces+0x4b/0x66
[  466.609071]  usb_resume+0x59/0x66
[  466.609075]  dpm_run_callback+0x8c/0x126
[  466.609078]  device_resume+0x1f1/0x25b
[  466.609082]  async_resume+0x1d/0x42
[  466.609085]  async_run_entry_fn+0x3d/0xd1
[  466.609089]  process_one_work+0x1b9/0x363
[  466.609093]  worker_thread+0x213/0x372
[  466.609097]  kthread+0x150/0x15f
[  466.609100]  ? pr_cont_work+0x58/0x58
[  466.609103]  ? kthread_blkcg+0x31/0x31
[  466.609106]  ret_from_fork+0x22/0x30


>
> Regards Lin Ma
>
> 在 2021-06-16 23:01:08，"Greg Kroah-Hartman" <gregkh@xxxxxxxxxxxxxxxxxxx> 写道：
>
> >On Mon, Jun 14, 2021 at 04:15:02PM +0200, Eric Dumazet wrote:
> >>
> >>
> >> On 6/8/21 8:27 PM, Greg Kroah-Hartman wrote:
> >> > From: Lin Ma <linma@xxxxxxxxxx>
> >> >
> >> > commit e305509e678b3a4af2b3cfd410f409f7cdaabb52 upstream.
> >> >
> >> > The hci_sock_dev_event() function will cleanup the hdev object for
> >> > sockets even if this object may still be in used within the
> >> > hci_sock_bound_ioctl() function, result in UAF vulnerability.
> >> >
> >> > This patch replace the BH context lock to serialize these affairs
> >> > and prevent the race condition.
> >> >
> >> > Signed-off-by: Lin Ma <linma@xxxxxxxxxx>
> >> > Signed-off-by: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> >> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> > ---
> >> >  net/bluetooth/hci_sock.c |    4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > --- a/net/bluetooth/hci_sock.c
> >> > +++ b/net/bluetooth/hci_sock.c
> >> > @@ -755,7 +755,7 @@ void hci_sock_dev_event(struct hci_dev *
> >> >            /* Detach sockets from device */
> >> >            read_lock(&hci_sk_list.lock);
> >> >            sk_for_each(sk, &hci_sk_list.head) {
> >> > -                  bh_lock_sock_nested(sk);
> >> > +                  lock_sock(sk);
> >> >                    if (hci_pi(sk)->hdev == hdev) {
> >> >                            hci_pi(sk)->hdev = NULL;
> >> >                            sk->sk_err = EPIPE;
> >> > @@ -764,7 +764,7 @@ void hci_sock_dev_event(struct hci_dev *
> >> >
> >> >                            hci_dev_put(hdev);
> >> >                    }
> >> > -                  bh_unlock_sock(sk);
> >> > +                  release_sock(sk);
> >> >            }
> >> >            read_unlock(&hci_sk_list.lock);
> >> >    }
> >> >
> >> >
> >>
> >>
> >> This patch is buggy.
> >>
> >> lock_sock() can sleep.
> >>
> >> But the read_lock(&hci_sk_list.lock) two lines before is not going to allow the sleep.
> >>
> >> Hmmm ?
> >>
> >>
> >
> >Odd, Lin, did you see any problems with your testing of this?
> >



-- 
Anand K. Mistry
Software Engineer
Google Australia