| Message ID | 20230811174633.1818931-1-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] Bluetooth: hci_sync: Fix UAF in hci_disconnect_all_sync | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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=775448 ---Test result--- Test Summary: CheckPatch PASS 0.77 seconds GitLint PASS 0.33 seconds SubjectPrefix PASS 0.10 seconds BuildKernel PASS 44.80 seconds CheckAllWarning PASS 48.41 seconds CheckSparse PASS 54.47 seconds CheckSmatch PASS 145.66 seconds BuildKernel32 PASS 42.36 seconds TestRunnerSetup PASS 639.62 seconds TestRunner_l2cap-tester PASS 31.76 seconds TestRunner_iso-tester PASS 72.96 seconds TestRunner_bnep-tester PASS 14.73 seconds TestRunner_mgmt-tester PASS 274.49 seconds TestRunner_rfcomm-tester PASS 21.91 seconds TestRunner_sco-tester PASS 24.41 seconds TestRunner_ioctl-tester PASS 24.53 seconds TestRunner_mesh-tester PASS 17.87 seconds TestRunner_smp-tester PASS 19.13 seconds TestRunner_userchan-tester PASS 15.05 seconds IncrementalBuild PASS 39.65 seconds --- Regards, Linux Bluetooth
Hi Luiz, pe, 2023-08-11 kello 10:46 -0700, Luiz Augusto von Dentz kirjoitti: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Use-after-free can occur in hci_disconnect_all_sync if a connection is > deleted by concurrent processing of a controller event. > > To prevent this the code now tries to iterate over the list backwards > to ensure the links are cleanup before its parents, also it no longer > relies on a cursor, instead it always uses the last element since > hci_abort_conn_sync is guaranteed to call hci_conn_del. This v2 patch seems to be same as v1. Is it the right one? > > UAF crash log: > ================================================================== > BUG: KASAN: slab-use-after-free in hci_set_powered_sync > (net/bluetooth/hci_sync.c:5424) [bluetooth] > Read of size 8 at addr ffff888009d9c000 by task kworker/u9:0/124 > > CPU: 0 PID: 124 Comm: kworker/u9:0 Tainted: G W > 6.5.0-rc1+ #10 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > 1.16.2-1.fc38 04/01/2014 > Workqueue: hci0 hci_cmd_sync_work [bluetooth] > Call Trace: > <TASK> > dump_stack_lvl+0x5b/0x90 > print_report+0xcf/0x670 > ? __virt_addr_valid+0xdd/0x160 > ? hci_set_powered_sync+0x2c9/0x4a0 [bluetooth] > kasan_report+0xa6/0xe0 > ? hci_set_powered_sync+0x2c9/0x4a0 [bluetooth] > ? __pfx_set_powered_sync+0x10/0x10 [bluetooth] > hci_set_powered_sync+0x2c9/0x4a0 [bluetooth] > ? __pfx_hci_set_powered_sync+0x10/0x10 [bluetooth] > ? __pfx_lock_release+0x10/0x10 > ? __pfx_set_powered_sync+0x10/0x10 [bluetooth] > hci_cmd_sync_work+0x137/0x220 [bluetooth] > process_one_work+0x526/0x9d0 > ? __pfx_process_one_work+0x10/0x10 > ? __pfx_do_raw_spin_lock+0x10/0x10 > ? mark_held_locks+0x1a/0x90 > worker_thread+0x92/0x630 > ? __pfx_worker_thread+0x10/0x10 > kthread+0x196/0x1e0 > ? __pfx_kthread+0x10/0x10 > ret_from_fork+0x2c/0x50 > </TASK> > > Allocated by task 1782: > kasan_save_stack+0x33/0x60 > kasan_set_track+0x25/0x30 > __kasan_kmalloc+0x8f/0xa0 > hci_conn_add+0xa5/0xa80 [bluetooth] > hci_bind_cis+0x881/0x9b0 [bluetooth] > iso_connect_cis+0x121/0x520 [bluetooth] > iso_sock_connect+0x3f6/0x790 [bluetooth] > __sys_connect+0x109/0x130 > __x64_sys_connect+0x40/0x50 > do_syscall_64+0x60/0x90 > entry_SYSCALL_64_after_hwframe+0x6e/0xd8 > > Freed by task 695: > kasan_save_stack+0x33/0x60 > kasan_set_track+0x25/0x30 > kasan_save_free_info+0x2b/0x50 > __kasan_slab_free+0x10a/0x180 > __kmem_cache_free+0x14d/0x2e0 > device_release+0x5d/0xf0 > kobject_put+0xdf/0x270 > hci_disconn_complete_evt+0x274/0x3a0 [bluetooth] > hci_event_packet+0x579/0x7e0 [bluetooth] > hci_rx_work+0x287/0xaa0 [bluetooth] > process_one_work+0x526/0x9d0 > worker_thread+0x92/0x630 > kthread+0x196/0x1e0 > ret_from_fork+0x2c/0x50 > ================================================================== > > Fixes: 182ee45da083 ("Bluetooth: hci_sync: Rework hci_suspend_notifier") > Signed-off-by: Pauli Virtanen <pav@iki.fi> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > net/bluetooth/hci_sync.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index 5eb30ba21370..c2fa6690c84c 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -5389,7 +5389,11 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) > hci_dev_unlock(hdev); > return 0; > default: > + hci_dev_lock(hdev); > conn->state = BT_CLOSED; > + hci_disconn_cfm(conn, reason); > + hci_conn_del(conn); > + hci_dev_unlock(hdev); > return 0; > } > > @@ -5418,13 +5422,19 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) > > static int hci_disconnect_all_sync(struct hci_dev *hdev, u8 reason) > { > - struct hci_conn *conn, *tmp; > - int err; > + struct list_head *head = &hdev->conn_hash.list; > > - list_for_each_entry_safe(conn, tmp, &hdev->conn_hash.list, list) { > - err = hci_abort_conn_sync(hdev, conn, reason); > - if (err) > - return err; > + /* Use reverse order so links are cleanup before parents */ > + while (!list_empty(&hdev->conn_hash.list)) { > + struct hci_conn *conn = list_last_entry(head, struct hci_conn, > + list); > + > + /* Disregard possible errors since hci_conn_del shall have been > + * called even in case of errors had occurred since it would > + * then cause hci_conn_failed to be called which calls > + * hci_conn_del internally. > + */ > + hci_abort_conn_sync(hdev, conn, reason); > } > > return 0;
Hi Pauli, On Fri, Aug 11, 2023 at 4:05 PM Pauli Virtanen <pav@iki.fi> wrote: > > Hi Luiz, > > pe, 2023-08-11 kello 10:46 -0700, Luiz Augusto von Dentz kirjoitti: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > Use-after-free can occur in hci_disconnect_all_sync if a connection is > > deleted by concurrent processing of a controller event. > > > > To prevent this the code now tries to iterate over the list backwards > > to ensure the links are cleanup before its parents, also it no longer > > relies on a cursor, instead it always uses the last element since > > hci_abort_conn_sync is guaranteed to call hci_conn_del. > > This v2 patch seems to be same as v1. Is it the right one? > > > > > UAF crash log: > > ================================================================== > > BUG: KASAN: slab-use-after-free in hci_set_powered_sync > > (net/bluetooth/hci_sync.c:5424) [bluetooth] > > Read of size 8 at addr ffff888009d9c000 by task kworker/u9:0/124 > > > > CPU: 0 PID: 124 Comm: kworker/u9:0 Tainted: G W > > 6.5.0-rc1+ #10 > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > > 1.16.2-1.fc38 04/01/2014 > > Workqueue: hci0 hci_cmd_sync_work [bluetooth] > > Call Trace: > > <TASK> > > dump_stack_lvl+0x5b/0x90 > > print_report+0xcf/0x670 > > ? __virt_addr_valid+0xdd/0x160 > > ? hci_set_powered_sync+0x2c9/0x4a0 [bluetooth] > > kasan_report+0xa6/0xe0 > > ? hci_set_powered_sync+0x2c9/0x4a0 [bluetooth] > > ? __pfx_set_powered_sync+0x10/0x10 [bluetooth] > > hci_set_powered_sync+0x2c9/0x4a0 [bluetooth] > > ? __pfx_hci_set_powered_sync+0x10/0x10 [bluetooth] > > ? __pfx_lock_release+0x10/0x10 > > ? __pfx_set_powered_sync+0x10/0x10 [bluetooth] > > hci_cmd_sync_work+0x137/0x220 [bluetooth] > > process_one_work+0x526/0x9d0 > > ? __pfx_process_one_work+0x10/0x10 > > ? __pfx_do_raw_spin_lock+0x10/0x10 > > ? mark_held_locks+0x1a/0x90 > > worker_thread+0x92/0x630 > > ? __pfx_worker_thread+0x10/0x10 > > kthread+0x196/0x1e0 > > ? __pfx_kthread+0x10/0x10 > > ret_from_fork+0x2c/0x50 > > </TASK> > > > > Allocated by task 1782: > > kasan_save_stack+0x33/0x60 > > kasan_set_track+0x25/0x30 > > __kasan_kmalloc+0x8f/0xa0 > > hci_conn_add+0xa5/0xa80 [bluetooth] > > hci_bind_cis+0x881/0x9b0 [bluetooth] > > iso_connect_cis+0x121/0x520 [bluetooth] > > iso_sock_connect+0x3f6/0x790 [bluetooth] > > __sys_connect+0x109/0x130 > > __x64_sys_connect+0x40/0x50 > > do_syscall_64+0x60/0x90 > > entry_SYSCALL_64_after_hwframe+0x6e/0xd8 > > > > Freed by task 695: > > kasan_save_stack+0x33/0x60 > > kasan_set_track+0x25/0x30 > > kasan_save_free_info+0x2b/0x50 > > __kasan_slab_free+0x10a/0x180 > > __kmem_cache_free+0x14d/0x2e0 > > device_release+0x5d/0xf0 > > kobject_put+0xdf/0x270 > > hci_disconn_complete_evt+0x274/0x3a0 [bluetooth] > > hci_event_packet+0x579/0x7e0 [bluetooth] > > hci_rx_work+0x287/0xaa0 [bluetooth] > > process_one_work+0x526/0x9d0 > > worker_thread+0x92/0x630 > > kthread+0x196/0x1e0 > > ret_from_fork+0x2c/0x50 > > ================================================================== > > > > Fixes: 182ee45da083 ("Bluetooth: hci_sync: Rework hci_suspend_notifier") > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > net/bluetooth/hci_sync.c | 22 ++++++++++++++++------ > > 1 file changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > > index 5eb30ba21370..c2fa6690c84c 100644 > > --- a/net/bluetooth/hci_sync.c > > +++ b/net/bluetooth/hci_sync.c > > @@ -5389,7 +5389,11 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) > > hci_dev_unlock(hdev); > > return 0; > > default: > > + hci_dev_lock(hdev); > > conn->state = BT_CLOSED; > > + hci_disconn_cfm(conn, reason); > > + hci_conn_del(conn); > > + hci_dev_unlock(hdev); > > return 0; > > } > > > > @@ -5418,13 +5422,19 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) > > > > static int hci_disconnect_all_sync(struct hci_dev *hdev, u8 reason) > > { > > - struct hci_conn *conn, *tmp; > > - int err; > > + struct list_head *head = &hdev->conn_hash.list; > > > > - list_for_each_entry_safe(conn, tmp, &hdev->conn_hash.list, list) { > > - err = hci_abort_conn_sync(hdev, conn, reason); > > - if (err) > > - return err; > > + /* Use reverse order so links are cleanup before parents */ > > + while (!list_empty(&hdev->conn_hash.list)) { > > + struct hci_conn *conn = list_last_entry(head, struct hci_conn, > > + list); > > + > > + /* Disregard possible errors since hci_conn_del shall have been > > + * called even in case of errors had occurred since it would > > + * then cause hci_conn_failed to be called which calls > > + * hci_conn_del internally. > > + */ > > + hci_abort_conn_sync(hdev, conn, reason); > > } > > > > return 0; > > -- > Pauli Virtanen Must have sent the wrong one, let's see if I got it right on v3.
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 5eb30ba21370..c2fa6690c84c 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -5389,7 +5389,11 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) hci_dev_unlock(hdev); return 0; default: + hci_dev_lock(hdev); conn->state = BT_CLOSED; + hci_disconn_cfm(conn, reason); + hci_conn_del(conn); + hci_dev_unlock(hdev); return 0; } @@ -5418,13 +5422,19 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) static int hci_disconnect_all_sync(struct hci_dev *hdev, u8 reason) { - struct hci_conn *conn, *tmp; - int err; + struct list_head *head = &hdev->conn_hash.list; - list_for_each_entry_safe(conn, tmp, &hdev->conn_hash.list, list) { - err = hci_abort_conn_sync(hdev, conn, reason); - if (err) - return err; + /* Use reverse order so links are cleanup before parents */ + while (!list_empty(&hdev->conn_hash.list)) { + struct hci_conn *conn = list_last_entry(head, struct hci_conn, + list); + + /* Disregard possible errors since hci_conn_del shall have been + * called even in case of errors had occurred since it would + * then cause hci_conn_failed to be called which calls + * hci_conn_del internally. + */ + hci_abort_conn_sync(hdev, conn, reason); } return 0;