[PATCH] Bluetooth: call lock_sock() outside of spinlock section

syzbot is hitting might_sleep() warning at hci_sock_dev_event() due to
calling lock_sock() with rw spinlock held [1]. Defer calling lock_sock()
via sock_hold().

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Reported-by: syzbot <redacted>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <redacted>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
---
 net/bluetooth/hci_sock.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index eed0dd066e12..64e54ab0892f 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -759,19 +759,39 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 	if (event == HCI_DEV_UNREG) {
 		struct sock *sk;
 
+restart:
 		/* Detach sockets from device */
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
+			/* hci_sk_list.lock is preventing hci_sock_release()
+			 * from calling bt_sock_unlink().
+			 */
+			if (hci_pi(sk)->hdev != hdev || sk_unhashed(sk))
+				continue;
+			/* Take a ref because we can't call lock_sock() with
+			 * hci_sk_list.lock held.
+			 */
+			sock_hold(sk);
+			read_unlock(&hci_sk_list.lock);
 			lock_sock(sk);
-			if (hci_pi(sk)->hdev == hdev) {
+			/* Since hci_sock_release() might have already called
+			 * bt_sock_unlink() while waiting for lock_sock(),
+			 * use sk_hashed(sk) for checking that bt_sock_unlink()
+			 * is not yet called.
+			 */
+			if (sk_hashed(sk) && hci_pi(sk)->hdev == hdev) {
 				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
 				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
-
 				hci_dev_put(hdev);
 			}
 			release_sock(sk);
+			sock_put(sk);
+			/* Restarting is safe, for hci_pi(sk)->hdev is now NULL
+			 * if condition met and will "continue;" otherwise.
+			 */
+			goto restart;
 		}
 		read_unlock(&hci_sk_list.lock);
 	}

-- 
2.18.4

syzbot is hitting might_sleep() warning at hci_sock_dev_event() due to
calling lock_sock() with rw spinlock held [1]. Defer calling lock_sock()
via sock_hold().

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Reported-by: syzbot <redacted>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <redacted>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
---
Changes in v2:
  Take hci_sk_list.lock for write in case bt_sock_unlink() is called after
  sk_hashed(sk) test, and defer hci_dev_put(hdev) till schedulable context.

 net/bluetooth/hci_sock.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..d8e1ac1ae10d 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -758,20 +758,46 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 
 	if (event == HCI_DEV_UNREG) {
 		struct sock *sk;
+		bool put_dev;
 
+restart:
+		put_dev = false;
 		/* Detach sockets from device */
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
+			/* hci_sk_list.lock is preventing hci_sock_release()
+			 * from calling bt_sock_unlink().
+			 */
+			if (hci_pi(sk)->hdev != hdev || sk_unhashed(sk))
+				continue;
+			/* Take a ref because we can't call lock_sock() with
+			 * hci_sk_list.lock held.
+			 */
+			sock_hold(sk);
+			read_unlock(&hci_sk_list.lock);
 			lock_sock(sk);
-			if (hci_pi(sk)->hdev == hdev) {
+			/* Since hci_sock_release() might have already called
+			 * bt_sock_unlink() while waiting for lock_sock(),
+			 * use sk_hashed(sk) for checking that bt_sock_unlink()
+			 * is not yet called.
+			 */
+			write_lock(&hci_sk_list.lock);
+			if (sk_hashed(sk) && hci_pi(sk)->hdev == hdev) {
 				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
 				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
-
-				hci_dev_put(hdev);
+				put_dev = true;
 			}
+			write_unlock(&hci_sk_list.lock);
 			release_sock(sk);
+			sock_put(sk);
+			if (put_dev)
+				hci_dev_put(hdev);
+			/* Restarting is safe, for hci_pi(sk)->hdev != hdev if
+			 * condition met and sk_unhashed(sk) == true otherwise.
+			 */
+			goto restart;
 		}
 		read_unlock(&hci_sk_list.lock);
 	}

-- 
2.18.4

Hi Tetsuo,

On Wed, Jul 7, 2021 at 2:43 AM Tetsuo Handa
[off-list ref] wrote:

This sounds a little too complicated, afaik backward goto is not even
consider a good practice either, since it appears we don't unlink the
sockets here we could perhaps don't release the reference to hdev
either and leave hci_sock_release to deal with it and then perhaps we
can take away the backward goto, actually why are you restarting to
begin with? It is also weird that this only manifests in the Bluetooth
HCI sockets or other subsystems don't use such locking mechanism
anymore?

                }
                read_unlock(&hci_sk_list.lock);
        }
--
2.18.4

-- 
Luiz Augusto von Dentz

On 2021/07/08 3:20, Luiz Augusto von Dentz wrote:

quoted

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..d8e1ac1ae10d 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -758,20 +758,46 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)

        if (event == HCI_DEV_UNREG) {
                struct sock *sk;
+               bool put_dev;

+restart:
+               put_dev = false;
                /* Detach sockets from device */
                read_lock(&hci_sk_list.lock);
                sk_for_each(sk, &hci_sk_list.head) {
+                       /* hci_sk_list.lock is preventing hci_sock_release()
+                        * from calling bt_sock_unlink().
+                        */
+                       if (hci_pi(sk)->hdev != hdev || sk_unhashed(sk))
+                               continue;
+                       /* Take a ref because we can't call lock_sock() with
+                        * hci_sk_list.lock held.
+                        */
+                       sock_hold(sk);
+                       read_unlock(&hci_sk_list.lock);
                        lock_sock(sk);
-                       if (hci_pi(sk)->hdev == hdev) {
+                       /* Since hci_sock_release() might have already called
+                        * bt_sock_unlink() while waiting for lock_sock(),
+                        * use sk_hashed(sk) for checking that bt_sock_unlink()
+                        * is not yet called.
+                        */
+                       write_lock(&hci_sk_list.lock);
+                       if (sk_hashed(sk) && hci_pi(sk)->hdev == hdev) {
                                hci_pi(sk)->hdev = NULL;
                                sk->sk_err = EPIPE;
                                sk->sk_state = BT_OPEN;
                                sk->sk_state_change(sk);
-
-                               hci_dev_put(hdev);
+                               put_dev = true;
                        }
+                       write_unlock(&hci_sk_list.lock);
                        release_sock(sk);
+                       sock_put(sk);
+                       if (put_dev)
+                               hci_dev_put(hdev);
+                       /* Restarting is safe, for hci_pi(sk)->hdev != hdev if
+                        * condition met and sk_unhashed(sk) == true otherwise.
+                        */
+                       goto restart;

This sounds a little too complicated, afaik backward goto is not even
consider a good practice either, since it appears we don't unlink the
sockets here

Because hci_sock_release() might be concurrently called while
hci_sock_dev_event() from hci_unregister_dev() from vhci_release() is running.

While hci_sock_dev_event() itself does not unlink the sockets from hci_sk_list.head,
bt_sock_unlink() from hci_sock_release() unlinks a socket from hci_sk_list.head.

Therefore, as long as there is possibility that hci_sk_list is modified by other thread
when current thread is traversing this list, we need to be prepared for such race.

             we could perhaps don't release the reference to hdev
either and leave hci_sock_release to deal with it and then perhaps we
can take away the backward goto, actually why are you restarting to
begin with?

Do you mean something like

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..0525883f4639 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -759,19 +759,14 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 	if (event == HCI_DEV_UNREG) {
 		struct sock *sk;
 
-		/* Detach sockets from device */
+		/* Change socket state and notify */
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
-			lock_sock(sk);
 			if (hci_pi(sk)->hdev == hdev) {
-				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
 				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
-
-				hci_dev_put(hdev);
 			}
-			release_sock(sk);
 		}
 		read_unlock(&hci_sk_list.lock);
 	}

? I can't judge because I don't know how this works. I worry that
without lock_sock()/release_sock(), this races with e.g. hci_sock_bind().

We could take away the backward goto if we can do something like below.

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..1ca03769badf 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -43,6 +43,8 @@ static DEFINE_IDA(sock_cookie_ida);
 
 static atomic_t monitor_promisc = ATOMIC_INIT(0);
 
+static DEFINE_MUTEX(sock_list_lock);
+
 /* ----- HCI socket interface ----- */
 
 /* Socket info */

@@ -760,7 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 		struct sock *sk;
 
 		/* Detach sockets from device */
-		read_lock(&hci_sk_list.lock);
+		mutex_lock(&sock_list_lock);
 		sk_for_each(sk, &hci_sk_list.head) {
 			lock_sock(sk);
 			if (hci_pi(sk)->hdev == hdev) {

@@ -773,7 +775,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 			}
 			release_sock(sk);
 		}
-		read_unlock(&hci_sk_list.lock);
+		mutex_unlock(&sock_list_lock);
 	}
 }
 

@@ -838,6 +840,7 @@ static int hci_sock_release(struct socket *sock)
 	if (!sk)
 		return 0;
 
+	mutex_lock(&sock_list_lock);
 	lock_sock(sk);
 
 	switch (hci_pi(sk)->channel) {

@@ -860,6 +863,7 @@ static int hci_sock_release(struct socket *sock)
 	}
 
 	bt_sock_unlink(&hci_sk_list, sk);
+	mutex_unlock(&sock_list_lock);
 
 	hdev = hci_pi(sk)->hdev;
 	if (hdev) {

@@ -2049,7 +2053,9 @@ static int hci_sock_create(struct net *net, struct socket *sock, int protocol,
 	sock->state = SS_UNCONNECTED;
 	sk->sk_state = BT_OPEN;
 
+	mutex_lock(&sock_list_lock);
 	bt_sock_link(&hci_sk_list, sk);
+	mutex_unlock(&sock_list_lock);
 	return 0;
 }
 

            It is also weird that this only manifests in the Bluetooth
HCI sockets or other subsystems don't use such locking mechanism
anymore?

If other subsystems have similar problem, that should be handled by different
patches. This patch fixes a regression introduced when fixing CVE-2021-3573,
and I think that Linux distributors are waiting for this regression to be fixed
so that they can backport commit e305509e678b3a4a ("Bluetooth: use correct lock
to prevent UAF of hdev object"). Also, this regression is currently 7th top
crashers for syzbot, and I'd like to apply this patch as soon as possible.

I think that this patch can serve as a response to Lin's comment

  > In short, I have no idea if there is any lock replacing solution for
  > this bug. I need help and suggestions because the lock mechanism is
  > just so difficult.

at https://patchwork.kernel.org/project/bluetooth/patch/CAJjojJsj9pzF4j2MVvsM-hCpvyR7OkZn232yt3MdOGnLxOiRRg@mail.gmail.com
without changing behavior.

quoted

                }
                read_unlock(&hci_sk_list.lock);
        }
--
2.18.4

Hello Tetsuo,

Yeah, that's a great patch indeed. Add one extra mutex lock for handling this.
In fact, I have tried to replace all the hci_sk_list.lock from rwlock_t to mutext.

https://patchwork.kernel.org/project/bluetooth/patch/CAJjojJsj9pzF4j2MVvsM-hCpvyR7OkZn232yt3MdOGnLxOiRRg@mail.gmail.com/
However, from the lock principle in the Linux kernel, this lock
replacement is not appropriate. I take a lot of time to try with other
lock combinations for this case but failed. For example, I tried to
replace the rwlock_t in the hci_sk_list with a sleep-able mutex lock.

Because I have seem other part of code in kernel uses this combination: mutex_t + lock_sock. It shouldn't trigger any locking errors. (Will test it)

Also, this regression is currently 7th top
crashers for syzbot, and I'd like to apply this patch as soon as possible.

XD, Yeah. Because the bug crash point is located at function hci_sock_dev_event(). Whenever syzkaller fuzzes Bluetooth stack and the executor exits, the crash happens.

I think that this patch can serve as a response to Lin's comment

quoted

In short, I have no idea if there is any lock replacing solution for
this bug. I need help and suggestions because the lock mechanism is
just so difficult.

Thanks for that, it's quite appreciating.

Regards
Lin Ma

It seems that history of this locking problem is a trial and error.

Commit b40df5743ee8aed8 ("[PATCH] bluetooth: fix socket locking in
hci_sock_dev_event()") in 2.6.21-rc4 changed bh_lock_sock() to lock_sock()
as an attempt to fix lockdep warning.

Then, commit 4ce61d1c7a8ef4c1 ("[BLUETOOTH]: Fix locking in
hci_sock_dev_event().") in 2.6.22-rc2 changed lock_sock() to
local_bh_disable() + bh_lock_sock_nested() as an attempt to fix
sleep in atomic context warning.

Then, commit 4b5dd696f81b210c ("Bluetooth: Remove local_bh_disable() from
hci_sock.c") in 3.3-rc1 removed local_bh_disable().

Then, commit e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF
of hdev object") in 5.13-rc5 again changed bh_lock_sock_nested() to
lock_sock() as an attempt to fix CVE-2021-3573.

But unfortunately it is too difficult to convert rw spinlock into mutex;
we need to live with current rw spinlock.

And we have three choices that can live with current rw spinlock.
Patches for these choices are show bottom. All tested by syzbot.

(1) Introduce a global mutex dedicated for hci_sock_dev_event(), and block
    bt_sock_unlink() and concurrent hci_sock_dev_event() callers.

    This is simplest if it is guaranteed that total delay for lock_sock()
    on all sockets is short enough.

    But it is not clear how long lock_sock() might block, for e.g.
    hci_sock_bound_ioctl() which is called inside lock_sock() section is
    doing copy_from_user()/copy_to_user() which may result in blocking
    other lock_sock() waiters for many seconds. I think that POC.zip is
    demonstrating that this delay is controllable via userfaultfd.

    Of course, the robust fix will be not to call
    copy_from_user()/copy_to_user() inside lock_sock() section. But such
    big change is not suitable for a fix for commit e305509e678b3a4a
    ("Bluetooth: use correct lock to prevent UAF of hdev object").

(2) Introduce a global mutex for hci_sock_dev_event(), but don't block
    bt_sock_unlink() nor concurrent hci_sock_dev_event() callers (i.e.
    use this mutex like a spinlock).

    Since it will be safe to resume sk_for_each() as long as currently
    accessing socket remains on that list, we can track which socket is
    currently accessed, and let bt_sock_unlink() wait if that socket is
    currently accessed.

    It is possible that total delay becomes long enough for khungtaskd to
    complain. Commit 8d0caedb75968304 ("can: bcm/raw/isotp: use per module
    netdevice notifier") is an example for avoiding khungtaskd warning
    using this choice. Compared to that commit, this choice for
    hci_sock_dev_event() case will need to also touch "struct hci_pinfo"
    because we need to track concurrent hci_sock_dev_event() callers.

(3) Don't introduce a global mutex for hci_sock_dev_event(), and don't
    block bt_sock_unlink() nor concurrent hci_sock_dev_event() callers.

    Since it will be safe to resume sk_for_each() as long as currently
    accessing socket remains on that list, take a refcount on currently
    accessing socket and check if currently accessing socket is still
    on the list. This choice needs to touch only hci_sock_dev_event().

Which choice do we want to go?

Patch for choice (1):

----------------------------------------
 net/bluetooth/hci_sock.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..c860ec4ea7b8 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -150,6 +150,8 @@ static struct bt_sock_list hci_sk_list = {
 	.lock = __RW_LOCK_UNLOCKED(hci_sk_list.lock)
 };
 
+static DEFINE_MUTEX(hci_sk_list_lock);
+
 static bool is_filtered_packet(struct sock *sk, struct sk_buff *skb)
 {
 	struct hci_filter *flt;

@@ -758,10 +760,13 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 
 	if (event == HCI_DEV_UNREG) {
 		struct sock *sk;
+		int put_count = 0;
 
 		/* Detach sockets from device */
+		mutex_lock(&hci_sk_list_lock);
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
+			read_unlock(&hci_sk_list.lock);
 			lock_sock(sk);
 			if (hci_pi(sk)->hdev == hdev) {
 				hci_pi(sk)->hdev = NULL;

@@ -769,11 +774,15 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
 
-				hci_dev_put(hdev);
+				put_count++;
 			}
 			release_sock(sk);
+			read_lock(&hci_sk_list.lock);
 		}
 		read_unlock(&hci_sk_list.lock);
+		mutex_unlock(&hci_sk_list_lock);
+		while (put_count--)
+			hci_dev_put(hdev);
 	}
 }
 

@@ -838,6 +847,10 @@ static int hci_sock_release(struct socket *sock)
 	if (!sk)
 		return 0;
 
+	mutex_lock(&hci_sk_list_lock);
+	bt_sock_unlink(&hci_sk_list, sk);
+	mutex_unlock(&hci_sk_list_lock);
+
 	lock_sock(sk);
 
 	switch (hci_pi(sk)->channel) {

@@ -859,8 +872,6 @@ static int hci_sock_release(struct socket *sock)
 		break;
 	}
 
-	bt_sock_unlink(&hci_sk_list, sk);
-
 	hdev = hci_pi(sk)->hdev;
 	if (hdev) {
 		if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
----------------------------------------

Patch for choice (2):

----------------------------------------
 net/bluetooth/hci_sock.c |   39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..3e65fcc8c9af 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -43,6 +43,8 @@ static DEFINE_IDA(sock_cookie_ida);
 
 static atomic_t monitor_promisc = ATOMIC_INIT(0);
 
+static DEFINE_MUTEX(dev_event_lock);
+
 /* ----- HCI socket interface ----- */
 
 /* Socket info */

@@ -57,6 +59,7 @@ struct hci_pinfo {
 	unsigned long     flags;
 	__u32             cookie;
 	char              comm[TASK_COMM_LEN];
+	unsigned int      event_in_progress;
 };
 
 void hci_sock_set_flag(struct sock *sk, int nr)

@@ -758,10 +761,15 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 
 	if (event == HCI_DEV_UNREG) {
 		struct sock *sk;
+		int put_count = 0;
 
 		/* Detach sockets from device */
+		mutex_lock(&dev_event_lock);
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
+			read_unlock(&hci_sk_list.lock);
+			hci_pi(sk)->event_in_progress++;
+			mutex_unlock(&dev_event_lock);
 			lock_sock(sk);
 			if (hci_pi(sk)->hdev == hdev) {
 				hci_pi(sk)->hdev = NULL;

@@ -769,11 +777,17 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
 
-				hci_dev_put(hdev);
+				put_count++;
 			}
 			release_sock(sk);
+			mutex_lock(&dev_event_lock);
+			hci_pi(sk)->event_in_progress--;
+			read_lock(&hci_sk_list.lock);
 		}
 		read_unlock(&hci_sk_list.lock);
+		mutex_unlock(&dev_event_lock);
+		while (put_count--)
+			hci_dev_put(hdev);
 	}
 }
 

@@ -838,6 +852,26 @@ static int hci_sock_release(struct socket *sock)
 	if (!sk)
 		return 0;
 
+	/*
+	 * Wait for sk_for_each() in hci_sock_dev_event() to stop accessing
+	 * this sk before unlinking. Need to unlink before lock_sock(), for
+	 * hci_sock_dev_event() calls lock_sock() after incrementing
+	 * event_in_progress counter.
+	 */
+	while (1) {
+		bool unlinked = true;
+
+		mutex_lock(&dev_event_lock);
+		if (!hci_pi(sk)->event_in_progress)
+			bt_sock_unlink(&hci_sk_list, sk);
+		else
+			unlinked = false;
+		mutex_unlock(&dev_event_lock);
+		if (unlinked)
+			break;
+		schedule_timeout_uninterruptible(1);
+	}
+
 	lock_sock(sk);
 
 	switch (hci_pi(sk)->channel) {

@@ -859,8 +893,6 @@ static int hci_sock_release(struct socket *sock)
 		break;
 	}
 
-	bt_sock_unlink(&hci_sk_list, sk);
-
 	hdev = hci_pi(sk)->hdev;
 	if (hdev) {
 		if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {

@@ -2049,6 +2081,7 @@ static int hci_sock_create(struct net *net, struct socket *sock, int protocol,
 	sock->state = SS_UNCONNECTED;
 	sk->sk_state = BT_OPEN;
 
+	hci_pi(sk)->event_in_progress = 0;
 	bt_sock_link(&hci_sk_list, sk);
 	return 0;
 }
----------------------------------------

Patch for choice (3):

----------------------------------------
 net/bluetooth/hci_sock.c |   35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..38146cf37378 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -758,22 +758,53 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 
 	if (event == HCI_DEV_UNREG) {
 		struct sock *sk;
+		int put_count = 0;
 
 		/* Detach sockets from device */
+restart:
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
+			/* This sock_hold(sk) is safe, for bt_sock_unlink(sk)
+			 * is not called yet.
+			 */
+			sock_hold(sk);
+			read_unlock(&hci_sk_list.lock);
 			lock_sock(sk);
-			if (hci_pi(sk)->hdev == hdev) {
+			write_lock(&hci_sk_list.lock);
+			/* Check that bt_sock_unlink(sk) is not called yet. */
+			if (sk_hashed(sk) && hci_pi(sk)->hdev == hdev) {
 				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
 				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
 
-				hci_dev_put(hdev);
+				put_count++;
 			}
+			write_unlock(&hci_sk_list.lock);
 			release_sock(sk);
+			read_lock(&hci_sk_list.lock);
+			/* If bt_sock_unlink(sk) is not called yet, we can
+			 * continue iteration. We can use __sock_put(sk) here
+			 * because hci_sock_release() will call sock_put(sk)
+			 * after bt_sock_unlink(sk).
+			 */
+			if (sk_hashed(sk)) {
+				__sock_put(sk);
+				continue;
+			}
+			/* Otherwise, we need to restart iteration, for the
+			 * next socket pointed by sk->next might be already
+			 * gone. We can't use __sock_put(sk) here because
+			 * hci_sock_release() might have already called
+			 * sock_put(sk) after bt_sock_unlink(sk).
+			 */
+			read_unlock(&hci_sk_list.lock);
+			sock_put(sk);
+			goto restart;
 		}
 		read_unlock(&hci_sk_list.lock);
+		while (put_count--)
+			hci_dev_put(hdev);
 	}
 }
 
----------------------------------------

On 2021/07/08 8:33, Tetsuo Handa wrote:

I examined hci_unregister_dev() and concluded that this can't work.

hci_sock_dev_event(hdev, HCI_DEV_UNREG) can't defer dropping the reference to
this hdev till hci_sock_release(), for hci_unregister_dev() cleans up everything
related to this hdev and calls hci_dev_put(hdev) and then vhci_release() calls
hci_free_dev(hdev).

That's the reason hci_sock_dev_event() has to use lock_sock() in order not to
miss some hci_dev_put(hdev) calls.

quoted

This sounds a little too complicated, afaik backward goto is not even
consider a good practice either, since it appears we don't unlink the
sockets here

Despite your comment, I'd like to go with choice (3) for now. After lock_sock() became
free from delay caused by pagefault handling, we could consider updating to choice (1).

syzbot is hitting might_sleep() warning at hci_sock_dev_event() due to
calling lock_sock() with rw spinlock held [1]. Among three possible
approaches [2], this patch chose holding a refcount via sock_hold() and
revalidating the element via sk_hashed().

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Link: https://lkml.kernel.org/r/05535d35-30d6-28b6-067e-272d01679d24@i-love.sakura.ne.jp [2]
Reported-by: syzbot <redacted>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <redacted>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
---
Changes in v3:
  Don't use unlocked hci_pi(sk)->hdev != hdev test, for it is racy.
  No need to defer hci_dev_put(hdev), for it can't be the last reference.

Changes in v2:
  Take hci_sk_list.lock for write in case bt_sock_unlink() is called after
  sk_hashed(sk) test, and defer hci_dev_put(hdev) till schedulable context.

 net/bluetooth/hci_sock.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..786a06a232fd 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -760,10 +760,18 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 		struct sock *sk;
 
 		/* Detach sockets from device */
+restart:
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
+			/* This sock_hold(sk) is safe, for bt_sock_unlink(sk)
+			 * is not called yet.
+			 */
+			sock_hold(sk);
+			read_unlock(&hci_sk_list.lock);
 			lock_sock(sk);
-			if (hci_pi(sk)->hdev == hdev) {
+			write_lock(&hci_sk_list.lock);
+			/* Check that bt_sock_unlink(sk) is not called yet. */
+			if (sk_hashed(sk) && hci_pi(sk)->hdev == hdev) {
 				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
 				sk->sk_state = BT_OPEN;

@@ -771,7 +779,27 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 
 				hci_dev_put(hdev);
 			}
+			write_unlock(&hci_sk_list.lock);
 			release_sock(sk);
+			read_lock(&hci_sk_list.lock);
+			/* If bt_sock_unlink(sk) is not called yet, we can
+			 * continue iteration. We can use __sock_put(sk) here
+			 * because hci_sock_release() will call sock_put(sk)
+			 * after bt_sock_unlink(sk).
+			 */
+			if (sk_hashed(sk)) {
+				__sock_put(sk);
+				continue;
+			}
+			/* Otherwise, we need to restart iteration, for the
+			 * next socket pointed by sk->next might be already
+			 * gone. We can't use __sock_put(sk) here because
+			 * hci_sock_release() might have already called
+			 * sock_put(sk) after bt_sock_unlink(sk).
+			 */
+			read_unlock(&hci_sk_list.lock);
+			sock_put(sk);
+			goto restart;
 		}
 		read_unlock(&hci_sk_list.lock);
 	}

-- 
2.18.4

Hi Tetsuo,

On Tue, Jul 13, 2021 at 4:28 AM Tetsuo Handa
[off-list ref] wrote:

syzbot is hitting might_sleep() warning at hci_sock_dev_event() due to
calling lock_sock() with rw spinlock held [1]. Among three possible
approaches [2], this patch chose holding a refcount via sock_hold() and
revalidating the element via sk_hashed().

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Link: https://lkml.kernel.org/r/05535d35-30d6-28b6-067e-272d01679d24@i-love.sakura.ne.jp [2]
Reported-by: syzbot <redacted>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <redacted>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
---
Changes in v3:
  Don't use unlocked hci_pi(sk)->hdev != hdev test, for it is racy.
  No need to defer hci_dev_put(hdev), for it can't be the last reference.

Changes in v2:
  Take hci_sk_list.lock for write in case bt_sock_unlink() is called after
  sk_hashed(sk) test, and defer hci_dev_put(hdev) till schedulable context.

How about we revert back to use bh_lock_sock_nested but use
local_bh_disable like the following patch:

https://patchwork.kernel.org/project/bluetooth/patch/20210713162838.693266-1-desmondcheongzx@gmail.com/

-- 
Luiz Augusto von Dentz

Hi there,

I'm just exhilarated to see there have been some new ideas to fix this.

How about we revert back to use bh_lock_sock_nested but use
local_bh_disable like the following patch:

https://patchwork.kernel.org/project/bluetooth/patch/20210713162838.693266-1-desmondcheongzx@gmail.com/

I have checked that patch and learn about some `local_bh_disable/enable` usage.
To the best of my knowledge, the local_bh_disable() function can be used to disable the processing of bottom halves (softirqs).
Or in another word, if process context function, hci_sock_sendmsg() for example, can mask the BH (hci_dev_do_close()?). It doesn't need to worry about the UAF.

However, after doing some experiments, I failed :(
For instance, I try to do following patch:

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -1720,6 +1720,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
                return -EINVAL;

        lock_sock(sk);
+       local_bh_disable();

        switch (hci_pi(sk)->channel) {
        case HCI_CHANNEL_RAW:

@@ -1832,7 +1833,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
        err = len;

 done:
+       local_bh_enable();
        release_sock(sk);
+
        return err;

But the POC code shows error message like below:

[   18.169155] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:197
[   18.170181] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 120, name: exp
[   18.170987] 1 lock held by exp/120:
[   18.171384]  #0: ffff888011dd5120 (sk_lock-AF_BLUETOOTH-BTPROTO_HCI){+.+.}-{0:0}, at: hci_sock_sendmsg+0x11e/0x26c0
[   18.172300] CPU: 0 PID: 120 Comm: exp Not tainted 5.11.11+ #44
[   18.172921] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
...

The patch provided by Desmond adds the local_bh_disable() before the bh_lock_sock() so I also try that in 

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -762,6 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                /* Detach sockets from device */
                read_lock(&hci_sk_list.lock);
                sk_for_each(sk, &hci_sk_list.head) {
+                       local_bh_disable();
                        bh_lock_sock_nested(sk);
                        if (hci_pi(sk)->hdev == hdev) {
                                hci_pi(sk)->hdev = NULL;

@@ -772,6 +773,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                                hci_dev_put(hdev);
                        }
                        bh_unlock_sock(sk);
+                       local_bh_enable();
                }
                read_unlock(&hci_sk_list.lock);
        }

But this is not useful, the UAF still occurs

[   13.862117] ==================================================================
[   13.863064] BUG: KASAN: use-after-free in __lock_acquire+0xe5/0x2ca0
[   13.863852] Read of size 8 at addr ffff888011d9aeb0 by task exp/119
[   13.864620]
[   13.864818] CPU: 0 PID: 119 Comm: exp Not tainted 5.11.11+ #45
[   13.865543] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[   13.866634] Call Trace:
[   13.866947]  dump_stack+0x183/0x22e
[   13.867389]  ? show_regs_print_info+0x12/0x12
[   13.867927]  ? log_buf_vmcoreinfo_setup+0x45d/0x45d
[   13.868503]  ? _raw_spin_lock_irqsave+0xbd/0x100
[   13.869244]  print_address_description+0x7b/0x3a0
[   13.869828]  __kasan_report+0x14e/0x200
[   13.870288]  ? __lock_acquire+0xe5/0x2ca0
[   13.870768]  kasan_report+0x47/0x60
[   13.871189]  __lock_acquire+0xe5/0x2ca0
[   13.871647]  ? lock_acquire+0x168/0x6a0
[   13.872107]  ? trace_lock_release+0x5c/0x120
[   13.872615]  ? do_user_addr_fault+0x9c2/0xdb0
[   13.873135]  ? trace_lock_acquire+0x150/0x150
[   13.873661]  ? rcu_read_lock_sched_held+0x87/0x110
[   13.874232]  ? perf_trace_rcu_barrier+0x360/0x360
[   13.874790]  ? avc_has_perm_noaudit+0x442/0x4c0
[   13.875332]  lock_acquire+0x168/0x6a0
[   13.875772]  ? skb_queue_tail+0x32/0x120
[   13.876240]  ? do_kern_addr_fault+0x230/0x230
[   13.876756]  ? read_lock_is_recursive+0x10/0x10
[   13.877300]  ? exc_page_fault+0xf3/0x1b0
[   13.877770]  ? cred_has_capability+0x191/0x3f0
[   13.878290]  ? cred_has_capability+0x2a1/0x3f0
[   13.878816]  ? rcu_lock_release+0x20/0x20
[   13.879295]  _raw_spin_lock_irqsave+0xb1/0x100
[   13.879821]  ? skb_queue_tail+0x32/0x120
[   13.880287]  ? _raw_spin_lock+0x40/0x40
[   13.880745]  skb_queue_tail+0x32/0x120
[   13.881194]  hci_sock_sendmsg+0x1545/0x26b0

From my point of view, adding the local_bh_disable() cannot prevent current hci_sock_dev_event() to set and decrease the ref-count. It's not quite similar with the cases that Desmond discussed.
(Or maybe just I don't know how to use this).

I recently tried to find some similar cases (and I did, reported to security already but get no reply) and figure out how others are fixed.
Some guideline tells me that (http://books.gigatux.nl/mirror/kerneldevelopment/0672327201/ch07lev1sec6.html)

"If process context code and a bottom half share data, you need to disable bottom-half processing and obtain a lock before accessing the data. Doing both ensures local and SMP protection and prevents a deadlock."

Assuming hci_sock_sendmsg()/hci_sock_bound_ioctl() are the process contexts while the hci_sock_dev_event(), not sure, is the BH context. The fact is that the hci_sock_dev_event() should wait for the process contexts. Hence, I think Tetsuo is on the right way.

Regards
Lock-Noob LinMa

On 15/7/21 11:03 am, LinMa wrote:

Hi,

Saw this and thought I'd offer my two cents.

This is the original problem that Tetsuo's patch was trying to fix. 
Under the hood of lock_sock, we call lock_sock_nested which might sleep 
because of the mutex_acquire. But we shouldn't sleep while holding the 
rw spinlock. So we either have to acquire a spinlock instead of a mutex 
as was done before, or we need to move lock_sock out of the rw spinlock 
critical section as Tetsuo proposes.

I might be very mistaken on this, but I believe the UAF still happens 
because you can't really mix bh_lock_sock* and lock_sock* to protect the 
same things. The former holds the spinlock &sk->sk_lock.slock and 
synchronizes between user contexts and bottom halves, while the latter 
holds a mutex on &sk->sk_lock.dep_map to synchronize between multiple users.

One option I can think of would be to switch instances of lock_sock to 
bh_lock_sock_nested for users that might race (such as hci_sock_sendmsg, 
hci_sock_bound_ioctl, and others as needed). But I'm not sure if that's 
quite what we want, plus we would need to ensure that sleeping functions 
aren't called between the bh_lock/unlock.

Best wishes,
Desmond

[   13.862117] ==================================================================
[   13.863064] BUG: KASAN: use-after-free in __lock_acquire+0xe5/0x2ca0
[   13.863852] Read of size 8 at addr ffff888011d9aeb0 by task exp/119
[   13.864620]
[   13.864818] CPU: 0 PID: 119 Comm: exp Not tainted 5.11.11+ #45
[   13.865543] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[   13.866634] Call Trace:
[   13.866947]  dump_stack+0x183/0x22e
[   13.867389]  ? show_regs_print_info+0x12/0x12
[   13.867927]  ? log_buf_vmcoreinfo_setup+0x45d/0x45d
[   13.868503]  ? _raw_spin_lock_irqsave+0xbd/0x100
[   13.869244]  print_address_description+0x7b/0x3a0
[   13.869828]  __kasan_report+0x14e/0x200
[   13.870288]  ? __lock_acquire+0xe5/0x2ca0
[   13.870768]  kasan_report+0x47/0x60
[   13.871189]  __lock_acquire+0xe5/0x2ca0
[   13.871647]  ? lock_acquire+0x168/0x6a0
[   13.872107]  ? trace_lock_release+0x5c/0x120
[   13.872615]  ? do_user_addr_fault+0x9c2/0xdb0
[   13.873135]  ? trace_lock_acquire+0x150/0x150
[   13.873661]  ? rcu_read_lock_sched_held+0x87/0x110
[   13.874232]  ? perf_trace_rcu_barrier+0x360/0x360
[   13.874790]  ? avc_has_perm_noaudit+0x442/0x4c0
[   13.875332]  lock_acquire+0x168/0x6a0
[   13.875772]  ? skb_queue_tail+0x32/0x120
[   13.876240]  ? do_kern_addr_fault+0x230/0x230
[   13.876756]  ? read_lock_is_recursive+0x10/0x10
[   13.877300]  ? exc_page_fault+0xf3/0x1b0
[   13.877770]  ? cred_has_capability+0x191/0x3f0
[   13.878290]  ? cred_has_capability+0x2a1/0x3f0
[   13.878816]  ? rcu_lock_release+0x20/0x20
[   13.879295]  _raw_spin_lock_irqsave+0xb1/0x100
[   13.879821]  ? skb_queue_tail+0x32/0x120
[   13.880287]  ? _raw_spin_lock+0x40/0x40
[   13.880745]  skb_queue_tail+0x32/0x120
[   13.881194]  hci_sock_sendmsg+0x1545/0x26b0

 From my point of view, adding the local_bh_disable() cannot prevent current hci_sock_dev_event() to set and decrease the ref-count. It's not quite similar with the cases that Desmond discussed.
(Or maybe just I don't know how to use this).
 > I recently tried to find some similar cases (and I did, reported to 

security already but get no reply) and figure out how others are fixed.

Some guideline tells me that (http://books.gigatux.nl/mirror/kerneldevelopment/0672327201/ch07lev1sec6.html)

"If process context code and a bottom half share data, you need to disable bottom-half processing and obtain a lock before accessing the data. Doing both ensures local and SMP protection and prevents a deadlock."

Assuming hci_sock_sendmsg()/hci_sock_bound_ioctl() are the process contexts while the hci_sock_dev_event(), not sure, is the BH context. The fact is that the hci_sock_dev_event() should wait for the process contexts. Hence, I think Tetsuo is on the right way.

Regards
Lock-Noob LinMa

On 16/7/21 11:47 am, Desmond Cheong Zhi Xi wrote:

On 15/7/21 11:03 am, LinMa wrote:

quoted

Hi there,

I'm just exhilarated to see there have been some new ideas to fix this.

quoted

How about we revert back to use bh_lock_sock_nested but use
local_bh_disable like the following patch:

https://patchwork.kernel.org/project/bluetooth/patch/20210713162838.693266-1-desmondcheongzx@gmail.com/ 

I have checked that patch and learn about some 
`local_bh_disable/enable` usage.
To the best of my knowledge, the local_bh_disable() function can be 
used to disable the processing of bottom halves (softirqs).
Or in another word, if process context function, hci_sock_sendmsg() 
for example, can mask the BH (hci_dev_do_close()?). It doesn't need to 
worry about the UAF.

However, after doing some experiments, I failed :(
For instance, I try to do following patch:

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -1720,6 +1720,7 @@ static int hci_sock_sendmsg(struct socket *sock, 

struct msghdr *msg,
                 return -EINVAL;

         lock_sock(sk);
+       local_bh_disable();

         switch (hci_pi(sk)->channel) {
         case HCI_CHANNEL_RAW:

@@ -1832,7 +1833,9 @@ static int hci_sock_sendmsg(struct socket *sock, 

struct msghdr *msg,
         err = len;

  done:
+       local_bh_enable();
         release_sock(sk);
+
         return err;

But the POC code shows error message like below:

[   18.169155] BUG: sleeping function called from invalid context at 
include/linux/sched/mm.h:197
[   18.170181] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 
120, name: exp
[   18.170987] 1 lock held by exp/120:
[   18.171384]  #0: ffff888011dd5120 
(sk_lock-AF_BLUETOOTH-BTPROTO_HCI){+.+.}-{0:0}, at: 
hci_sock_sendmsg+0x11e/0x26c0
[   18.172300] CPU: 0 PID: 120 Comm: exp Not tainted 5.11.11+ #44
[   18.172921] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS 1.10.2-1ubuntu1 04/01/2014
...

Hi,

Saw this and thought I'd offer my two cents.
BUG: sleeping function called from invalid context
This is the original problem that Tetsuo's patch was trying to fix. 
Under the hood of lock_sock, we call lock_sock_nested which might sleep 
because of the mutex_acquire. But we shouldn't sleep while holding the 
rw spinlock. So we either have to acquire a spinlock instead of a mutex 
as was done before, or we need to move lock_sock out of the rw spinlock 
critical section as Tetsuo proposes.

My bad, was thinking more about the problem and noticed your poc was for 
hci_sock_sendmsg, not hci_sock_dev_event. In this case, it's not clear 
to me why the atomic context is being violated.

Sorry for the noise.

quoted

The patch provided by Desmond adds the local_bh_disable() before the 
bh_lock_sock() so I also try that in

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -762,6 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int 

event)
                 /* Detach sockets from device */
                 read_lock(&hci_sk_list.lock);
                 sk_for_each(sk, &hci_sk_list.head) {
+                       local_bh_disable();
                         bh_lock_sock_nested(sk);
                         if (hci_pi(sk)->hdev == hdev) {
                                 hci_pi(sk)->hdev = NULL;

@@ -772,6 +773,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int 

event)
                                 hci_dev_put(hdev);
                         }
                         bh_unlock_sock(sk);
+                       local_bh_enable();
                 }
                 read_unlock(&hci_sk_list.lock);
         }

But this is not useful, the UAF still occurs

I might be very mistaken on this, but I believe the UAF still happens 
because you can't really mix bh_lock_sock* and lock_sock* to protect the 
same things. The former holds the spinlock &sk->sk_lock.slock and 
synchronizes between user contexts and bottom halves, while the latter 
holds a mutex on &sk->sk_lock.dep_map to synchronize between multiple 
users.

One option I can think of would be to switch instances of lock_sock to 
bh_lock_sock_nested for users that might race (such as hci_sock_sendmsg, 
hci_sock_bound_ioctl, and others as needed). But I'm not sure if that's 
quite what we want, plus we would need to ensure that sleeping functions 
aren't called between the bh_lock/unlock.

Best wishes,
Desmond

quoted

[   13.862117] 
==================================================================
[   13.863064] BUG: KASAN: use-after-free in __lock_acquire+0xe5/0x2ca0
[   13.863852] Read of size 8 at addr ffff888011d9aeb0 by task exp/119
[   13.864620]
[   13.864818] CPU: 0 PID: 119 Comm: exp Not tainted 5.11.11+ #45
[   13.865543] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS 1.10.2-1ubuntu1 04/01/2014
[   13.866634] Call Trace:
[   13.866947]  dump_stack+0x183/0x22e
[   13.867389]  ? show_regs_print_info+0x12/0x12
[   13.867927]  ? log_buf_vmcoreinfo_setup+0x45d/0x45d
[   13.868503]  ? _raw_spin_lock_irqsave+0xbd/0x100
[   13.869244]  print_address_description+0x7b/0x3a0
[   13.869828]  __kasan_report+0x14e/0x200
[   13.870288]  ? __lock_acquire+0xe5/0x2ca0
[   13.870768]  kasan_report+0x47/0x60
[   13.871189]  __lock_acquire+0xe5/0x2ca0
[   13.871647]  ? lock_acquire+0x168/0x6a0
[   13.872107]  ? trace_lock_release+0x5c/0x120
[   13.872615]  ? do_user_addr_fault+0x9c2/0xdb0
[   13.873135]  ? trace_lock_acquire+0x150/0x150
[   13.873661]  ? rcu_read_lock_sched_held+0x87/0x110
[   13.874232]  ? perf_trace_rcu_barrier+0x360/0x360
[   13.874790]  ? avc_has_perm_noaudit+0x442/0x4c0
[   13.875332]  lock_acquire+0x168/0x6a0
[   13.875772]  ? skb_queue_tail+0x32/0x120
[   13.876240]  ? do_kern_addr_fault+0x230/0x230
[   13.876756]  ? read_lock_is_recursive+0x10/0x10
[   13.877300]  ? exc_page_fault+0xf3/0x1b0
[   13.877770]  ? cred_has_capability+0x191/0x3f0
[   13.878290]  ? cred_has_capability+0x2a1/0x3f0
[   13.878816]  ? rcu_lock_release+0x20/0x20
[   13.879295]  _raw_spin_lock_irqsave+0xb1/0x100
[   13.879821]  ? skb_queue_tail+0x32/0x120
[   13.880287]  ? _raw_spin_lock+0x40/0x40
[   13.880745]  skb_queue_tail+0x32/0x120
[   13.881194]  hci_sock_sendmsg+0x1545/0x26b0

 From my point of view, adding the local_bh_disable() cannot prevent 
current hci_sock_dev_event() to set and decrease the ref-count. It's 
not quite similar with the cases that Desmond discussed.
(Or maybe just I don't know how to use this).
 > I recently tried to find some similar cases (and I did, reported to 

security already but get no reply) and figure out how others are fixed.

quoted

Some guideline tells me that 
(http://books.gigatux.nl/mirror/kerneldevelopment/0672327201/ch07lev1sec6.html) 


"If process context code and a bottom half share data, you need to 
disable bottom-half processing and obtain a lock before accessing the 
data. Doing both ensures local and SMP protection and prevents a 
deadlock."

Assuming hci_sock_sendmsg()/hci_sock_bound_ioctl() are the process 
contexts while the hci_sock_dev_event(), not sure, is the BH context. 
The fact is that the hci_sock_dev_event() should wait for the process 
contexts. Hence, I think Tetsuo is on the right way.

Regards
Lock-Noob LinMa

On 2021/07/16 13:11, Desmond Cheong Zhi Xi wrote:

On 16/7/21 11:47 am, Desmond Cheong Zhi Xi wrote:

quoted

Saw this and thought I'd offer my two cents.
BUG: sleeping function called from invalid context
This is the original problem that Tetsuo's patch was trying to fix.

Yes.

quoted

Under the hood of lock_sock, we call lock_sock_nested which might sleep
because of the mutex_acquire.

Both lock_sock() and lock_sock_nested() might sleep.

quoted

But we shouldn't sleep while holding the rw spinlock.

Right. In atomic context (e.g. inside interrupt handler, schedulable context
with interrupts or preemption disabled, schedulable context inside RCU read
critical section, schedulable context inside a spinlock section), we must not
call functions (e.g. waiting for a mutex, waiting for a semaphore, waiting for
a page fault) which are not atomic.

quoted

So we either have to acquire a spinlock instead of a mutex as was done before,

Regarding hci_sock_dev_event(HCI_DEV_UNREG) case, we can't use a spinlock.

Like LinMa explained, lock_sock() has to be used in order to serialize functions
(e.g. hci_sock_sendmsg()) which access hci_pi(sk)->hdev between lock_sock(sk) and
release_sock(sk). And like I explained, we can't defer resetting hci_pi(sk)->hdev
to NULL, for hci_sock_dev_event(HCI_DEV_UNREG) is responsible for resetting
hci_pi(sk)->hdev to NULL because the caller of hci_sock_dev_event(HCI_DEV_UNREG)
immediately destroys resources associated with this hdev.

quoted

or we need to move lock_sock out of the rw spinlock critical section as Tetsuo proposes.

Exactly. Since this is a regression introduced when fixing CVE-2021-3573, Linux
distributors are waiting for this patch so that they can apply the fix for CVE-2021-3573.
This patch should be sent to linux.git and stables as soon as possible. But due to little
attention on this patch, I'm already testing this patch in linux-next.git via my tree.
I'll drop when Bluetooth maintainers pick this patch up for linux-5.14-rcX. (Or should I
directly send to Linus?)

quoted

My bad, was thinking more about the problem and noticed your poc was for hci_sock_sendmsg,
not hci_sock_dev_event.

I didn't catch this part. Are you talking about a different poc?
As far as I'm aware, exp.c in POC.zip was for hci_sock_bound_ioctl(HCIUNBLOCKADDR).

hci_sock_bound_ioctl(HCIUNBLOCKADDR) (which is called between lock_sock() and release_sock())
calls copy_from_user() which might cause page fault, and userfaultfd mechanism allows an attacker
to slowdown page fault handling enough to hci_sock_dev_event(HCI_DEV_UNREG) to return without
waiting for hci_sock_bound_ioctl(HCIUNBLOCKADDR) to call release_sock(). This race window
results in UAF (doesn't it, LinMa?).

In this case, it's not clear to me why the atomic context is being violated.

In atomic context (in hci_sock_dev_event(HCI_DEV_UNREG) case, between
read_lock(&hci_sk_list.lock) and read_unlock(&hci_sk_list.lock)), we must not call
lock_sock(sk) which might wait for hci_sock_bound_ioctl(HCIUNBLOCKADDR) to call release_sock().

Sorry for the noise.

quoted

quoted

The patch provided by Desmond adds the local_bh_disable() before the bh_lock_sock() so I also try that in

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -762,6 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                 /* Detach sockets from device */
                 read_lock(&hci_sk_list.lock);
                 sk_for_each(sk, &hci_sk_list.head) {
+                       local_bh_disable();
                         bh_lock_sock_nested(sk);
                         if (hci_pi(sk)->hdev == hdev) {
                                 hci_pi(sk)->hdev = NULL;

@@ -772,6 +773,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                                 hci_dev_put(hdev);
                         }
                         bh_unlock_sock(sk);
+                       local_bh_enable();
                 }
                 read_unlock(&hci_sk_list.lock);
         }

But this is not useful, the UAF still occurs

I might be very mistaken on this, but I believe the UAF still happens because
you can't really mix bh_lock_sock* and lock_sock* to protect the same things.

Right. https://www.kernel.org/doc/html/v5.13/kernel-hacking/locking.html

quoted

The former holds the spinlock &sk->sk_lock.slock and synchronizes between
user contexts and bottom halves,

serializes access to resources which might be accessed from atomic (i.e. non-schedulable) contexts

quoted

while the latter holds a mutex on &sk->sk_lock.dep_map to synchronize between
multiple users.

serializes access to resources which are accessed from only schedulable (i.e. non-atomic) contexts

quoted

One option I can think of would be to switch instances of lock_sock to bh_lock_sock_nested
for users that might race (such as hci_sock_sendmsg, hci_sock_bound_ioctl, and others as
needed). But I'm not sure if that's quite what we want, plus we would need to ensure that
sleeping functions aren't called between the bh_lock/unlock.

We can't do it for hci_sock_dev_event(HCI_DEV_UNREG).

Hello everyone,

Sorry, it's my fault to cause the misunderstanding.

As I keep mentioning "hci_sock_sendmsg()" instead of "hci_sock_bound_ioctl()". In fact, both these two functions are able to cause the race.

quoted

quoted

My bad, was thinking more about the problem and noticed your poc was for hci_sock_sendmsg,
not hci_sock_dev_event.

I didn't catch this part. Are you talking about a different poc?
As far as I'm aware, exp.c in POC.zip was for hci_sock_bound_ioctl(HCIUNBLOCKADDR).

hci_sock_bound_ioctl(HCIUNBLOCKADDR) (which is called between lock_sock() and release_sock())
calls copy_from_user() which might cause page fault, and userfaultfd mechanism allows an attacker
to slowdown page fault handling enough to hci_sock_dev_event(HCI_DEV_UNREG) to return without
waiting for hci_sock_bound_ioctl(HCIUNBLOCKADDR) to call release_sock(). This race window
results in UAF (doesn't it, LinMa?).

Your understanding above is quite right. In addition, because the hci_sock_sendmsg() calls the memcpy_from_msg(...), which also in fact fetch data from userspace memory, the userfaultfd can take effect as well.

(When writing the exploit for this CVE, the hci_sock_sendmsg() is much useful... so I recently keep mentioning this function)

Regards
Lin Ma

Hi everyone,

After reading large lines of code in the net directory of the kernel, I have the following thinkings and may need your guys' suggestions.

quoted

quoted

quoted

Saw this and thought I'd offer my two cents.
BUG: sleeping function called from invalid context
This is the original problem that Tetsuo's patch was trying to fix.

Yes.

quoted

quoted

Under the hood of lock_sock, we call lock_sock_nested which might sleep
because of the mutex_acquire.

Both lock_sock() and lock_sock_nested() might sleep.

quoted

quoted

But we shouldn't sleep while holding the rw spinlock.

Right. In atomic context (e.g. inside interrupt handler, schedulable context
with interrupts or preemption disabled, schedulable context inside RCU read
critical section, schedulable context inside a spinlock section), we must not
call functions (e.g. waiting for a mutex, waiting for a semaphore, waiting for
a page fault) which are not atomic.

quoted

quoted

So we either have to acquire a spinlock instead of a mutex as was done before,

Regarding hci_sock_dev_event(HCI_DEV_UNREG) case, we can't use a spinlock.

Like LinMa explained, lock_sock() has to be used in order to serialize functions
(e.g. hci_sock_sendmsg()) which access hci_pi(sk)->hdev between lock_sock(sk) and
release_sock(sk). And like I explained, we can't defer resetting hci_pi(sk)->hdev
to NULL, for hci_sock_dev_event(HCI_DEV_UNREG) is responsible for resetting
hci_pi(sk)->hdev to NULL because the caller of hci_sock_dev_event(HCI_DEV_UNREG)
immediately destroys resources associated with this hdev.

This is the critical part of the BUG here. As you can read some similar code, for example, code at /net/iucv/af_iucv.c.

The iucv_sock_bind() function will bind the device: 

static int iucv_sock_bind(struct socket *sock, struct sockaddr *addr,
			  int addr_len)
{
...
  iucv->hs_dev = dev;

And this field will be assigned as NULL only when the socket is closed.

static void iucv_sock_close(struct sock *sk)
{
...
  if (iucv->hs_dev) {
    dev_put(iucv->hs_dev);
    iucv->hs_dev = NULL;
    sk->sk_bound_dev_if = 0;
  }

Even in the afiucv_netdev_event() function, there is non business with iucv->hs_dev.

So why the hci_sock_dev_event(HCI_DEV_UNREG) need to set the NULL pointer and then decrease the ref-count? 
As Tetsuo said, because the hci_unregister_dev() function, which is the caller of hci_sock_dev_event() will
reclaim the resource of the hdev object. It will destroy the workqueue and also clean up the sysfs.

If we achieve our patches like the iucv stack, or some other ref-count idea (https://lkml.org/lkml/2021/6/22/1347) 
without care, the bad thing will happen. Because there is nothing useful in the hdev object, any changes to it make no sense.

But wait, the write or dereference for this object can be illegal, but there should be some legal actions, like reading flags?

Hence, we can still delay the release of the hdev object to hci_sock_release (like other net code does). 
We just need to take care of the checking part.

One quick patch is shown below, my POC didn't trigger any warning but more checks are needed.

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 251b9128f..db665f78a 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -764,12 +764,9 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 		sk_for_each(sk, &hci_sk_list.head) {
 			bh_lock_sock_nested(sk);
 			if (hci_pi(sk)->hdev == hdev) {
-				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
 				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
-
-				hci_dev_put(hdev);
 			}
 			bh_unlock_sock(sk);
 		}

@@ -880,6 +877,7 @@ static int hci_sock_release(struct socket *sock)

 		atomic_dec(&hdev->promisc);
 		hci_dev_put(hdev);
+		hci_pi(sk)->hdev = NULL;
 	}

 	sock_orphan(sk);

@@ -1727,10 +1725,10 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 		break;
 	case HCI_CHANNEL_MONITOR:
 		err = -EOPNOTSUPP;
-		goto done;
+		goto donefast;
 	case HCI_CHANNEL_LOGGING:
 		err = hci_logging_frame(sk, msg, len);
-		goto done;
+		goto donefast;
 	default:
 		mutex_lock(&mgmt_chan_list_lock);
 		chan = __hci_mgmt_chan_find(hci_pi(sk)->channel);

@@ -1740,15 +1738,16 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 			err = -EINVAL;

 		mutex_unlock(&mgmt_chan_list_lock);
-		goto done;
+		goto donefast;
 	}

 	hdev = hci_pi(sk)->hdev;
 	if (!hdev) {
 		err = -EBADFD;
-		goto done;
+		goto donefast;
 	}

+	hci_dev_lock(hdev);
 	if (!test_bit(HCI_UP, &hdev->flags)) {
 		err = -ENETDOWN;
 		goto done;

@@ -1832,6 +1831,8 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	err = len;

 done:
+	hci_dev_unlock(hdev);
+donefast:
 	release_sock(sk);
 	return err;

In short, this patch delays the hci_dev_put() to hci_sock_release() and keeps the old bh_lock_sock_nested().

Once we did that, the UAF in hci_sock_bound_ioctl() are fixed. ( The four different commands in hci_sock_bound_ioctl will just
traverse a empty linked list )

For another UAF point: hci_sock_sendmsg(), this patch uses hci_dev_lock() to make sure the flags and resource in hdev will not be
released till the sendmsg is finished. (Dislike the hci_sock_create(), the hci_sock_sendmsg() can sleep so the mutex lock is possible)

Of course, more auditing is needed but I just want to share this to you. Any suggestions and discussions will be much appreciated.

quoted

quoted

quoted

or we need to move lock_sock out of the rw spinlock critical section as Tetsuo proposes.

Exactly. Since this is a regression introduced when fixing CVE-2021-3573, Linux
distributors are waiting for this patch so that they can apply the fix for CVE-2021-3573.
This patch should be sent to linux.git and stables as soon as possible. But due to little
attention on this patch, I'm already testing this patch in linux-next.git via my tree.
I'll drop when Bluetooth maintainers pick this patch up for linux-5.14-rcX. (Or should I
directly send to Linus?)

quoted

quoted

My bad, was thinking more about the problem and noticed your poc was for hci_sock_sendmsg,
not hci_sock_dev_event.

I didn't catch this part. Are you talking about a different poc?
As far as I'm aware, exp.c in POC.zip was for hci_sock_bound_ioctl(HCIUNBLOCKADDR).

hci_sock_bound_ioctl(HCIUNBLOCKADDR) (which is called between lock_sock() and release_sock())
calls copy_from_user() which might cause page fault, and userfaultfd mechanism allows an attacker
to slowdown page fault handling enough to hci_sock_dev_event(HCI_DEV_UNREG) to return without
waiting for hci_sock_bound_ioctl(HCIUNBLOCKADDR) to call release_sock(). This race window
results in UAF (doesn't it, LinMa?).

quoted

In this case, it's not clear to me why the atomic context is being violated.

In atomic context (in hci_sock_dev_event(HCI_DEV_UNREG) case, between
read_lock(&hci_sk_list.lock) and read_unlock(&hci_sk_list.lock)), we must not call
lock_sock(sk) which might wait for hci_sock_bound_ioctl(HCIUNBLOCKADDR) to call release_sock().

quoted

Sorry for the noise.

quoted

quoted

The patch provided by Desmond adds the local_bh_disable() before the bh_lock_sock() so I also try that in

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -762,6 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                 /* Detach sockets from device */
                 read_lock(&hci_sk_list.lock);
                 sk_for_each(sk, &hci_sk_list.head) {
+                       local_bh_disable();
                         bh_lock_sock_nested(sk);
                         if (hci_pi(sk)->hdev == hdev) {
                                 hci_pi(sk)->hdev = NULL;

@@ -772,6 +773,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                                 hci_dev_put(hdev);
                         }
                         bh_unlock_sock(sk);
+                       local_bh_enable();
                 }
                 read_unlock(&hci_sk_list.lock);
         }

But this is not useful, the UAF still occurs

I might be very mistaken on this, but I believe the UAF still happens because
you can't really mix bh_lock_sock* and lock_sock* to protect the same things.

Right. https://www.kernel.org/doc/html/v5.13/kernel-hacking/locking.html

quoted

quoted

The former holds the spinlock &sk->sk_lock.slock and synchronizes between
user contexts and bottom halves,

serializes access to resources which might be accessed from atomic (i.e. non-schedulable) contexts

quoted

quoted

while the latter holds a mutex on &sk->sk_lock.dep_map to synchronize between
multiple users.

serializes access to resources which are accessed from only schedulable (i.e. non-atomic) contexts

quoted

quoted

One option I can think of would be to switch instances of lock_sock to bh_lock_sock_nested
for users that might race (such as hci_sock_sendmsg, hci_sock_bound_ioctl, and others as
needed). But I'm not sure if that's quite what we want, plus we would need to ensure that
sleeping functions aren't called between the bh_lock/unlock.

We can't do it for hci_sock_dev_event(HCI_DEV_UNREG).

Regards 

Lin Ma

Ooooooops

I just found that Luiz has already figured out one good patch:
https://www.spinics.net/lists/linux-bluetooth/msg92649.html

Sorry for the noise and happy weekend.

Thanks
Lin Ma

Hi Tetsuo,

Just find out another interesting function: sock_owned_by_user(). (I am just a noob of kernel locks)

Hence I think the following patch has the same 'effect' as the old patch e305509e678b3 ("Bluetooth: use correct lock to prevent UAF of hdev object")

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..0cc4b88daa96 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -762,7 +762,11 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                /* Detach sockets from device */
                read_lock(&hci_sk_list.lock);
                sk_for_each(sk, &hci_sk_list.head) {
-                       lock_sock(sk);
+                       bh_lock_sock_nested(sk);
+busywait:
+                       if (sock_owned_by_user(sk))
+                               goto busywait;
+
                        if (hci_pi(sk)->hdev == hdev) {
                                hci_pi(sk)->hdev = NULL;
                                sk->sk_err = EPIPE;

@@ -771,7 +775,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)

                                hci_dev_put(hdev);
                        }
-                       release_sock(sk);
+                       bh_unlock_sock(sk);
                }
                read_unlock(&hci_sk_list.lock);
        }

The sad thing is that it seems will cost CPU resource to do meaningless wait...

What do you think? Can this sock_owned_by_user() function do any help?

Thanks
Lin Ma

From: "Tetsuo Handa" <penguin-kernel@i-love.sakura.ne.jp>
Sent Time: 2021-07-16 22:48:13 (Friday)
To: "Desmond Cheong Zhi Xi" <redacted>, LinMa <redacted>, "Luiz Augusto von Dentz" <luiz.dentz@gmail.com>, "Johan Hedberg" <redacted>, "Marcel Holtmann" <marcel@holtmann.org>
Cc: "linux-bluetooth@vger.kernel.org" <redacted>, "David S. Miller" <davem@davemloft.net>, "Jakub Kicinski" <kuba@kernel.org>, "open list:NETWORKING [GENERAL]" <redacted>
Subject: Re: [PATCH v3] Bluetooth: call lock_sock() outside of spinlock section

On 2021/07/16 13:11, Desmond Cheong Zhi Xi wrote:

quoted

On 16/7/21 11:47 am, Desmond Cheong Zhi Xi wrote:

quoted

Saw this and thought I'd offer my two cents.
BUG: sleeping function called from invalid context
This is the original problem that Tetsuo's patch was trying to fix.

Yes.

quoted

quoted

Under the hood of lock_sock, we call lock_sock_nested which might sleep
because of the mutex_acquire.

Both lock_sock() and lock_sock_nested() might sleep.

quoted

quoted

But we shouldn't sleep while holding the rw spinlock.

Right. In atomic context (e.g. inside interrupt handler, schedulable context
with interrupts or preemption disabled, schedulable context inside RCU read
critical section, schedulable context inside a spinlock section), we must not
call functions (e.g. waiting for a mutex, waiting for a semaphore, waiting for
a page fault) which are not atomic.

quoted

quoted

So we either have to acquire a spinlock instead of a mutex as was done before,

Regarding hci_sock_dev_event(HCI_DEV_UNREG) case, we can't use a spinlock.

Like LinMa explained, lock_sock() has to be used in order to serialize functions
(e.g. hci_sock_sendmsg()) which access hci_pi(sk)->hdev between lock_sock(sk) and
release_sock(sk). And like I explained, we can't defer resetting hci_pi(sk)->hdev
to NULL, for hci_sock_dev_event(HCI_DEV_UNREG) is responsible for resetting
hci_pi(sk)->hdev to NULL because the caller of hci_sock_dev_event(HCI_DEV_UNREG)
immediately destroys resources associated with this hdev.

quoted

quoted

or we need to move lock_sock out of the rw spinlock critical section as Tetsuo proposes.

Exactly. Since this is a regression introduced when fixing CVE-2021-3573, Linux
distributors are waiting for this patch so that they can apply the fix for CVE-2021-3573.
This patch should be sent to linux.git and stables as soon as possible. But due to little
attention on this patch, I'm already testing this patch in linux-next.git via my tree.
I'll drop when Bluetooth maintainers pick this patch up for linux-5.14-rcX. (Or should I
directly send to Linus?)

quoted

quoted

My bad, was thinking more about the problem and noticed your poc was for hci_sock_sendmsg,
not hci_sock_dev_event.

I didn't catch this part. Are you talking about a different poc?
As far as I'm aware, exp.c in POC.zip was for hci_sock_bound_ioctl(HCIUNBLOCKADDR).

hci_sock_bound_ioctl(HCIUNBLOCKADDR) (which is called between lock_sock() and release_sock())
calls copy_from_user() which might cause page fault, and userfaultfd mechanism allows an attacker
to slowdown page fault handling enough to hci_sock_dev_event(HCI_DEV_UNREG) to return without
waiting for hci_sock_bound_ioctl(HCIUNBLOCKADDR) to call release_sock(). This race window
results in UAF (doesn't it, LinMa?).

quoted

In this case, it's not clear to me why the atomic context is being violated.

In atomic context (in hci_sock_dev_event(HCI_DEV_UNREG) case, between
read_lock(&hci_sk_list.lock) and read_unlock(&hci_sk_list.lock)), we must not call
lock_sock(sk) which might wait for hci_sock_bound_ioctl(HCIUNBLOCKADDR) to call release_sock().

quoted

Sorry for the noise.

quoted

quoted

The patch provided by Desmond adds the local_bh_disable() before the bh_lock_sock() so I also try that in

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -762,6 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                 /* Detach sockets from device */
                 read_lock(&hci_sk_list.lock);
                 sk_for_each(sk, &hci_sk_list.head) {
+                       local_bh_disable();
                         bh_lock_sock_nested(sk);
                         if (hci_pi(sk)->hdev == hdev) {
                                 hci_pi(sk)->hdev = NULL;

@@ -772,6 +773,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
                                 hci_dev_put(hdev);
                         }
                         bh_unlock_sock(sk);
+                       local_bh_enable();
                 }
                 read_unlock(&hci_sk_list.lock);
         }

But this is not useful, the UAF still occurs

I might be very mistaken on this, but I believe the UAF still happens because
you can't really mix bh_lock_sock* and lock_sock* to protect the same things.

Right. https://www.kernel.org/doc/html/v5.13/kernel-hacking/locking.html

quoted

quoted

The former holds the spinlock &sk->sk_lock.slock and synchronizes between
user contexts and bottom halves,

serializes access to resources which might be accessed from atomic (i.e. non-schedulable) contexts

quoted

quoted

while the latter holds a mutex on &sk->sk_lock.dep_map to synchronize between
multiple users.

serializes access to resources which are accessed from only schedulable (i.e. non-atomic) contexts

quoted

quoted

One option I can think of would be to switch instances of lock_sock to bh_lock_sock_nested
for users that might race (such as hci_sock_sendmsg, hci_sock_bound_ioctl, and others as
needed). But I'm not sure if that's quite what we want, plus we would need to ensure that
sleeping functions aren't called between the bh_lock/unlock.

We can't do it for hci_sock_dev_event(HCI_DEV_UNREG).