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
