Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit ff29fe26 authored by Tetsuo Handa's avatar Tetsuo Handa Committed by Greg Kroah-Hartman
Browse files

Bluetooth: defer cleanup of resources in hci_unregister_dev() 

[ Upstream commit e04480920d1eec9c061841399aa6f35b6f987d8b ]

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

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

Commit b40df574 ("[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 4ce61d1c ("[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 the
sleep in atomic context warning.

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

Then, commit e305509e678b ("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.

This difficulty comes from current implementation that
hci_sock_dev_event(HCI_DEV_UNREG) is responsible for dropping all
references from sockets because hci_unregister_dev() immediately
reclaims resources as soon as returning from
hci_sock_dev_event(HCI_DEV_UNREG).

But the history suggests that hci_sock_dev_event(HCI_DEV_UNREG) was not
doing what it should do.

Therefore, instead of trying to detach sockets from device, let's accept
not detaching sockets from device at hci_sock_dev_event(HCI_DEV_UNREG),
by moving actual cleanup of resources from hci_unregister_dev() to
hci_cleanup_dev() which is called by bt_host_release() when all
references to this unregistered device (which is a kobject) are gone.

Since hci_sock_dev_event(HCI_DEV_UNREG) no longer resets
hci_pi(sk)->hdev, we need to check whether this device was unregistered
and return an error based on HCI_UNREGISTER flag.  There might be subtle
behavioral difference in "monitor the hdev" functionality; please report
if you found something went wrong due to this patch.

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9

 [1]
Reported-by: default avatarsyzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Suggested-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: default avatarTetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: e305509e678b ("Bluetooth: use correct lock to prevent UAF of hdev object")
Acked-by: default avatarLuiz Augusto von Dentz <luiz.von.dentz@intel.com>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: default avatarSasha Levin <sashal@kernel.org>
parent 580c10a4

include/net/bluetooth/hci_core.h

+1 −0
Original line number Diff line number Diff line
@@ -1056,6 +1056,7 @@ struct hci_dev *hci_alloc_dev(void); 
void hci_free_dev(struct hci_dev *hdev);

int hci_register_dev(struct hci_dev *hdev);

void hci_unregister_dev(struct hci_dev *hdev);

void hci_cleanup_dev(struct hci_dev *hdev);

int hci_suspend_dev(struct hci_dev *hdev);

int hci_resume_dev(struct hci_dev *hdev);

int hci_reset_dev(struct hci_dev *hdev);

net/bluetooth/hci_core.c

+8 −8
Original line number Diff line number Diff line
@@ -3393,14 +3393,10 @@ EXPORT_SYMBOL(hci_register_dev); 
/* Unregister HCI device */

void hci_unregister_dev(struct hci_dev *hdev)

{

	int id;



	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);



	hci_dev_set_flag(hdev, HCI_UNREGISTER);



	id = hdev->id;



	write_lock(&hci_dev_list_lock);

	list_del(&hdev->list);

	write_unlock(&hci_dev_list_lock);
@@ -3429,7 +3425,14 @@ void hci_unregister_dev(struct hci_dev *hdev) 
	}



	device_del(&hdev->dev);

	/* Actual cleanup is deferred until hci_cleanup_dev(). */

	hci_dev_put(hdev);

}

EXPORT_SYMBOL(hci_unregister_dev);



/* Cleanup HCI device */

void hci_cleanup_dev(struct hci_dev *hdev)

{

	debugfs_remove_recursive(hdev->debugfs);

	kfree_const(hdev->hw_info);

	kfree_const(hdev->fw_info);
@@ -3452,11 +3455,8 @@ void hci_unregister_dev(struct hci_dev *hdev) 
	hci_discovery_filter_clear(hdev);

	hci_dev_unlock(hdev);



	hci_dev_put(hdev);



	ida_simple_remove(&hci_index_ida, id);

	ida_simple_remove(&hci_index_ida, hdev->id);

}

EXPORT_SYMBOL(hci_unregister_dev);



/* Suspend HCI device */

int hci_suspend_dev(struct hci_dev *hdev)

net/bluetooth/hci_sock.c

+33 −16
Original line number Diff line number Diff line
@@ -59,6 +59,17 @@ struct hci_pinfo { 
	char              comm[TASK_COMM_LEN];

};



static struct hci_dev *hci_hdev_from_sock(struct sock *sk)

{

	struct hci_dev *hdev = hci_pi(sk)->hdev;



	if (!hdev)

		return ERR_PTR(-EBADFD);

	if (hci_dev_test_flag(hdev, HCI_UNREGISTER))

		return ERR_PTR(-EPIPE);

	return hdev;

}



void hci_sock_set_flag(struct sock *sk, int nr)

{

	set_bit(nr, &hci_pi(sk)->flags);
@@ -752,19 +763,13 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event) 
	if (event == HCI_DEV_UNREG) {

		struct sock *sk;



		/* Detach sockets from device */

		/* Wake up sockets using this dead device */

		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);

	}
@@ -923,10 +928,10 @@ static int hci_sock_blacklist_del(struct hci_dev *hdev, void __user *arg) 
static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,

				unsigned long arg)

{

	struct hci_dev *hdev = hci_pi(sk)->hdev;

	struct hci_dev *hdev = hci_hdev_from_sock(sk);



	if (!hdev)

		return -EBADFD;

	if (IS_ERR(hdev))

		return PTR_ERR(hdev);



	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))

		return -EBUSY;
@@ -1080,6 +1085,18 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, 


	lock_sock(sk);



	/* Allow detaching from dead device and attaching to alive device, if

	 * the caller wants to re-bind (instead of close) this socket in

	 * response to hci_sock_dev_event(HCI_DEV_UNREG) notification.

	 */

	hdev = hci_pi(sk)->hdev;

	if (hdev && hci_dev_test_flag(hdev, HCI_UNREGISTER)) {

		hci_pi(sk)->hdev = NULL;

		sk->sk_state = BT_OPEN;

		hci_dev_put(hdev);

	}

	hdev = NULL;



	if (sk->sk_state == BT_BOUND) {

		err = -EALREADY;

		goto done;
@@ -1356,9 +1373,9 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr, 


	lock_sock(sk);



	hdev = hci_pi(sk)->hdev;

	if (!hdev) {

		err = -EBADFD;

	hdev = hci_hdev_from_sock(sk);

	if (IS_ERR(hdev)) {

		err = PTR_ERR(hdev);

		goto done;

	}
@@ -1718,9 +1735,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg, 
		goto done;

	}



	hdev = hci_pi(sk)->hdev;

	if (!hdev) {

		err = -EBADFD;

	hdev = hci_hdev_from_sock(sk);

	if (IS_ERR(hdev)) {

		err = PTR_ERR(hdev);

		goto done;

	}

net/bluetooth/hci_sysfs.c

+3 −0
Original line number Diff line number Diff line
@@ -83,6 +83,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn) 
static void bt_host_release(struct device *dev)

{

	struct hci_dev *hdev = to_hci_dev(dev);



	if (hci_dev_test_flag(hdev, HCI_UNREGISTER))

		hci_cleanup_dev(hdev);

	kfree(hdev);

	module_put(THIS_MODULE);

}