linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] Bluetooth: defer cleanup of resources in hci_unregister_dev()
@ 2021-07-26 23:36 Luiz Augusto von Dentz
  2021-07-27  3:26 ` [v2] " bluez.test.bot
  0 siblings, 1 reply; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-26 23:36 UTC (permalink / raw)
  To: linux-bluetooth

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

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 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.

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_release_dev() which is called by bt_host_release when all references
to this unregistered device (which is a kobject) are gone.

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Reported-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Add hci_release_dev and make bt_host_release call it instead of making
bt_host_release public.

 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         | 17 +++++++++--------
 net/bluetooth/hci_sock.c         | 20 +++++++++++++-------
 net/bluetooth/hci_sysfs.c        |  2 +-
 4 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a53e94459ecd..4abe3c494002 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1230,6 +1230,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_release_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);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2560ed2f144d..2b78e1336c53 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3996,14 +3996,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);
@@ -4038,7 +4034,13 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	}
 
 	device_del(&hdev->dev);
+	hci_dev_put(hdev);
+}
+EXPORT_SYMBOL(hci_unregister_dev);
 
+/* Release HCI device */
+void hci_release_dev(struct hci_dev *hdev)
+{
 	debugfs_remove_recursive(hdev->debugfs);
 	kfree_const(hdev->hw_info);
 	kfree_const(hdev->fw_info);
@@ -4063,11 +4065,10 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	hci_blocked_keys_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);
+	kfree(hdev);
 }
-EXPORT_SYMBOL(hci_unregister_dev);
+EXPORT_SYMBOL(hci_release_dev);
 
 /* Suspend HCI device */
 int hci_suspend_dev(struct hci_dev *hdev)
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..5c28ec051dd6 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -759,19 +759,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);
 	}
@@ -1103,6 +1097,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;
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 9874844a95a9..ebf282d1eb2b 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -83,7 +83,7 @@ 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);
-	kfree(hdev);
+	hci_release_dev(hdev);
 	module_put(THIS_MODULE);
 }
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [v2] Bluetooth: defer cleanup of resources in hci_unregister_dev()
  2021-07-26 23:36 [PATCH v2] Bluetooth: defer cleanup of resources in hci_unregister_dev() Luiz Augusto von Dentz
@ 2021-07-27  3:26 ` bluez.test.bot
  2021-07-28  0:30   ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 3+ messages in thread
From: bluez.test.bot @ 2021-07-27  3:26 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz

[-- Attachment #1: Type: text/plain, Size: 4020 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=521583

---Test result---

Test Summary:
CheckPatch                    FAIL      1.14 seconds
GitLint                       FAIL      0.12 seconds
BuildKernel                   PASS      615.44 seconds
TestRunner: Setup             PASS      399.97 seconds
TestRunner: l2cap-tester      PASS      3.01 seconds
TestRunner: bnep-tester       PASS      2.18 seconds
TestRunner: mgmt-tester       PASS      32.18 seconds
TestRunner: rfcomm-tester     PASS      2.42 seconds
TestRunner: sco-tester        PASS      2.32 seconds
TestRunner: smp-tester        FAIL      2.32 seconds
TestRunner: userchan-tester   PASS      2.25 seconds

Details
##############################
Test: CheckPatch - FAIL - 1.14 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: defer cleanup of resources in hci_unregister_dev()
WARNING: Unknown commit id 'b40df5743ee8aed8', maybe rebased or not pulled?
#11: 
Commit b40df5743ee8aed8 ("[PATCH] bluetooth: fix socket locking in

WARNING: Unknown commit id '4ce61d1c7a8ef4c1', maybe rebased or not pulled?
#15: 
Then, commit 4ce61d1c7a8ef4c1 ("[BLUETOOTH]: Fix locking in

WARNING: Unknown commit id '4b5dd696f81b210c', maybe rebased or not pulled?
#20: 
Then, commit 4b5dd696f81b210c ("Bluetooth: Remove local_bh_disable() from

WARNING: Unknown commit id 'e305509e678b3a4a', maybe rebased or not pulled?
#23: 
Then, commit e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF

total: 0 errors, 4 warnings, 0 checks, 94 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: defer cleanup of resources in hci_unregister_dev()" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL - 0.12 seconds
Run gitlint with rule in .gitlint
Bluetooth: defer cleanup of resources in hci_unregister_dev()
41: B1 Line exceeds max length (85>80): "Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")"


##############################
Test: BuildKernel - PASS - 615.44 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 399.97 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 3.01 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 2.18 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 32.18 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.42 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.32 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.32 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.022 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 2.25 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44386 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3592 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 616863 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11711 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9948 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11741 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5489 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [v2] Bluetooth: defer cleanup of resources in hci_unregister_dev()
  2021-07-27  3:26 ` [v2] " bluez.test.bot
@ 2021-07-28  0:30   ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-28  0:30 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Tetsuo Handa

Hi Tetsuo,

On Mon, Jul 26, 2021 at 8:26 PM <bluez.test.bot@gmail.com> wrote:
>
> This is automated email and please do not reply to this email!
>
> Dear submitter,
>
> Thank you for submitting the patches to the linux bluetooth mailing list.
> This is a CI test results with your patch series:
> PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=521583
>
> ---Test result---
>
> Test Summary:
> CheckPatch                    FAIL      1.14 seconds
> GitLint                       FAIL      0.12 seconds
> BuildKernel                   PASS      615.44 seconds
> TestRunner: Setup             PASS      399.97 seconds
> TestRunner: l2cap-tester      PASS      3.01 seconds
> TestRunner: bnep-tester       PASS      2.18 seconds
> TestRunner: mgmt-tester       PASS      32.18 seconds
> TestRunner: rfcomm-tester     PASS      2.42 seconds
> TestRunner: sco-tester        PASS      2.32 seconds
> TestRunner: smp-tester        FAIL      2.32 seconds
> TestRunner: userchan-tester   PASS      2.25 seconds
>
> Details
> ##############################
> Test: CheckPatch - FAIL - 1.14 seconds
> Run checkpatch.pl script with rule in .checkpatch.conf
> Bluetooth: defer cleanup of resources in hci_unregister_dev()
> WARNING: Unknown commit id 'b40df5743ee8aed8', maybe rebased or not pulled?
> #11:
> Commit b40df5743ee8aed8 ("[PATCH] bluetooth: fix socket locking in
>
> WARNING: Unknown commit id '4ce61d1c7a8ef4c1', maybe rebased or not pulled?
> #15:
> Then, commit 4ce61d1c7a8ef4c1 ("[BLUETOOTH]: Fix locking in
>
> WARNING: Unknown commit id '4b5dd696f81b210c', maybe rebased or not pulled?
> #20:
> Then, commit 4b5dd696f81b210c ("Bluetooth: Remove local_bh_disable() from
>
> WARNING: Unknown commit id 'e305509e678b3a4a', maybe rebased or not pulled?
> #23:
> Then, commit e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF
>
> total: 0 errors, 4 warnings, 0 checks, 94 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
>
> "[PATCH] Bluetooth: defer cleanup of resources in hci_unregister_dev()" has style problems, please review.
>
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
>
> ##############################
> Test: GitLint - FAIL - 0.12 seconds
> Run gitlint with rule in .gitlint
> Bluetooth: defer cleanup of resources in hci_unregister_dev()
> 41: B1 Line exceeds max length (85>80): "Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")"
>
>
> ##############################
> Test: BuildKernel - PASS - 615.44 seconds
> Build Kernel with minimal configuration supports Bluetooth
>
>
> ##############################
> Test: TestRunner: Setup - PASS - 399.97 seconds
> Setup environment for running Test Runner
>
>
> ##############################
> Test: TestRunner: l2cap-tester - PASS - 3.01 seconds
> Run test-runner with l2cap-tester
> Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
>
> ##############################
> Test: TestRunner: bnep-tester - PASS - 2.18 seconds
> Run test-runner with bnep-tester
> Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
>
> ##############################
> Test: TestRunner: mgmt-tester - PASS - 32.18 seconds
> Run test-runner with mgmt-tester
> Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3
>
> ##############################
> Test: TestRunner: rfcomm-tester - PASS - 2.42 seconds
> Run test-runner with rfcomm-tester
> Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
>
> ##############################
> Test: TestRunner: sco-tester - PASS - 2.32 seconds
> Run test-runner with sco-tester
> Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
>
> ##############################
> Test: TestRunner: smp-tester - FAIL - 2.32 seconds
> Run test-runner with smp-tester
> Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0
>
> Failed Test Cases
> SMP Client - SC Request 2                            Failed       0.022 seconds
>
> ##############################
> Test: TestRunner: userchan-tester - PASS - 2.25 seconds
> Run test-runner with userchan-tester
> Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
>
>
>
> ---
> Regards,
> Linux Bluetooth

Pushed, thanks.


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-07-28  0:30 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 23:36 [PATCH v2] Bluetooth: defer cleanup of resources in hci_unregister_dev() Luiz Augusto von Dentz
2021-07-27  3:26 ` [v2] " bluez.test.bot
2021-07-28  0:30   ` Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox