On 2021/07/08 3:20, Luiz Augusto von Dentz wrote:
>> 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@xxxxxxxxxxxxxx
without changing behavior.
>
>
>> }
>> read_unlock(&hci_sk_list.lock);
>> }
>> --
>> 2.18.4
