On Thu, 17 Jun 2021 at 22:37, LinMa linma@zju.edu.cn 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@linuxfoundation.org 写道：
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@zju.edu.cn
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@zju.edu.cn Signed-off-by: Marcel Holtmann marcel@holtmann.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
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?