In hci_abort_conn_sync, some errors from hci_le_connect_cancel_sync
indicate the connection can be terminated immediately. However,
hci_abort_conn_sync passes these errors through, making
hci_disconnect_all_sync stop processing in these cases.
Fix by using a separate flag to indicate if canceled connection can be
deleted directly.
Fixes: 0f2efc1d02d5 ("Bluetooth: hci_conn: Consolidate code for aborting connections")
Fixes: dd1f6778b17b ("Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting")
Signed-off-by: Pauli Virtanen <[email protected]>
---
Notes:
v2: no change
net/bluetooth/hci_sync.c | 48 ++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 41a8e57d8267..51ff682f66e0 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5253,13 +5253,14 @@ static int hci_disconnect_sync(struct hci_dev *hdev, struct hci_conn *conn,
}
static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
- struct hci_conn *conn, u8 reason)
+ struct hci_conn *conn, u8 reason,
+ bool *del)
{
- /* Return reason if scanning since the connection shall probably be
- * cleanup directly.
- */
- if (test_bit(HCI_CONN_SCANNING, &conn->flags))
- return reason;
+ /* If scanning, connection can be just deleted */
+ if (test_bit(HCI_CONN_SCANNING, &conn->flags)) {
+ *del = true;
+ return 0;
+ }
if (conn->role == HCI_ROLE_SLAVE ||
test_and_set_bit(HCI_CONN_CANCEL, &conn->flags))
@@ -5270,10 +5271,10 @@ static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
}
static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn,
- u8 reason)
+ u8 reason, bool *del)
{
if (conn->type == LE_LINK)
- return hci_le_connect_cancel_sync(hdev, conn, reason);
+ return hci_le_connect_cancel_sync(hdev, conn, reason, del);
if (conn->type == ISO_LINK) {
/* BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
@@ -5287,9 +5288,9 @@ static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn,
if (test_bit(HCI_CONN_CREATE_CIS, &conn->flags))
return hci_disconnect_sync(hdev, conn, reason);
- /* CIS with no Create CIS sent have nothing to cancel */
+ /* CIS with no Create CIS sent can be just deleted */
if (bacmp(&conn->dst, BDADDR_ANY))
- return HCI_ERROR_LOCAL_HOST_TERM;
+ *del = true;
/* There is no way to cancel a BIS without terminating the BIG
* which is done later on connection cleanup.
@@ -5370,6 +5371,7 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
{
int err = 0;
u16 handle = conn->handle;
+ bool del = false;
switch (conn->state) {
case BT_CONNECTED:
@@ -5377,17 +5379,15 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
err = hci_disconnect_sync(hdev, conn, reason);
break;
case BT_CONNECT:
- err = hci_connect_cancel_sync(hdev, conn, reason);
+ err = hci_connect_cancel_sync(hdev, conn, reason, &del);
break;
case BT_CONNECT2:
err = hci_reject_conn_sync(hdev, conn, reason);
break;
case BT_OPEN:
case BT_BOUND:
- hci_dev_lock(hdev);
- hci_conn_failed(conn, reason);
- hci_dev_unlock(hdev);
- return 0;
+ del = true;
+ break;
default:
conn->state = BT_CLOSED;
return 0;
@@ -5398,18 +5398,15 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
* or in case of LE it was still scanning so it can be cleanup
* safely.
*/
- if (err) {
- struct hci_conn *c;
+ if (err || del) {
+ hci_dev_lock(hdev);
/* Check if the connection hasn't been cleanup while waiting
* commands to complete.
*/
- c = hci_conn_hash_lookup_handle(hdev, handle);
- if (!c || c != conn)
- return 0;
+ if (hci_conn_hash_lookup_handle(hdev, handle) == conn)
+ hci_conn_failed(conn, del ? reason : err);
- hci_dev_lock(hdev);
- hci_conn_failed(conn, err);
hci_dev_unlock(hdev);
}
@@ -6311,8 +6308,11 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
conn->conn_timeout, NULL);
done:
- if (err == -ETIMEDOUT)
- hci_le_connect_cancel_sync(hdev, conn, 0x00);
+ if (err == -ETIMEDOUT) {
+ bool __maybe_unused del;
+
+ hci_le_connect_cancel_sync(hdev, conn, 0x00, &del);
+ }
/* Re-enable advertising after the connection attempt is finished. */
hci_resume_advertising_sync(hdev);
--
2.41.0
When processing connections in abort_conn_sync, also check the
connection handle still refers to a connection that should be aborted.
There is a theoretical race condition where a connection handle is
reused, after hci_abort_conn but before abort_conn_sync is processed in
hci_sync. This change should avoid terminating a wrong connection in
this case.
Signed-off-by: Pauli Virtanen <[email protected]>
---
Notes:
v2: no change
net/bluetooth/hci_conn.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index ae206eb551f7..b65a1e9b178b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2861,7 +2861,7 @@ static int abort_conn_sync(struct hci_dev *hdev, void *data)
u16 handle = PTR_ERR(data);
conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
+ if (!conn || !conn->abort_reason)
return 0;
return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
@@ -2876,6 +2876,8 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
*/
if (conn->abort_reason)
return 0;
+ if (!reason)
+ return -EINVAL;
bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
--
2.41.0
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=773450
---Test result---
Test Summary:
CheckPatch FAIL 2.55 seconds
GitLint FAIL 1.27 seconds
SubjectPrefix PASS 0.33 seconds
BuildKernel PASS 33.28 seconds
CheckAllWarning PASS 36.11 seconds
CheckSparse PASS 40.39 seconds
CheckSmatch PASS 112.02 seconds
BuildKernel32 PASS 30.97 seconds
TestRunnerSetup PASS 485.38 seconds
TestRunner_l2cap-tester PASS 22.99 seconds
TestRunner_iso-tester PASS 44.11 seconds
TestRunner_bnep-tester PASS 10.17 seconds
TestRunner_mgmt-tester PASS 210.83 seconds
TestRunner_rfcomm-tester PASS 15.40 seconds
TestRunner_sco-tester PASS 18.57 seconds
TestRunner_ioctl-tester PASS 17.02 seconds
TestRunner_mesh-tester PASS 12.77 seconds
TestRunner_smp-tester PASS 13.66 seconds
TestRunner_userchan-tester PASS 10.60 seconds
IncrementalBuild PASS 48.22 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v2,2/4] Bluetooth: hci_sync: fix use-after-free in hci_disconnect_all_sync
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#100:
CPU: 0 PID: 124 Comm: kworker/u9:0 Tainted: G W 6.5.0-rc1+ #10
total: 0 errors, 1 warnings, 0 checks, 56 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.
/github/workspace/src/src/13342903.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2,2/4] Bluetooth: hci_sync: fix use-after-free in hci_disconnect_all_sync
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
13: B1 Line exceeds max length (99>80): "BUG: KASAN: slab-use-after-free in hci_set_powered_sync (net/bluetooth/hci_sync.c:5424) [bluetooth]"
17: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
80: B2 Line has trailing whitespace: " "
---
Regards,
Linux Bluetooth
Use-after-free occurs in hci_disconnect_all_sync if a connection is
deleted by concurrent processing of a controller event. This can occur
while waiting for controller events (big time window) or at other times
(less likely).
Fix the iteration in hci_disconnect_all_sync to allow hci_conn to be
deleted at any time.
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 <[email protected]>
---
Notes:
v2: use only valid values for abort_reason, in case we fail before
handling all conns
This is still a bit clumsy, a separate flag indicating if connection was
aborted yet could be better.
net/bluetooth/hci_sync.c | 47 ++++++++++++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 51ff682f66e0..228259f44815 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5415,16 +5415,49 @@ 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 hci_conn *c, *conn;
+ int err = 0;
- list_for_each_entry_safe(conn, tmp, &hdev->conn_hash.list, list) {
- err = hci_abort_conn_sync(hdev, conn, reason);
- if (err)
- return err;
+ rcu_read_lock();
+
+ /* Any conn may be gone while waiting for events, iterate safely.
+ * If conn is in conn_hash and we didn't abort it, it probably
+ * has not yet been aborted.
+ */
+ list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) {
+ if (c->abort_reason != reason)
+ continue;
+
+ c->abort_reason = (reason != HCI_ERROR_REMOTE_USER_TERM) ?
+ HCI_ERROR_REMOTE_USER_TERM : HCI_ERROR_UNSPECIFIED;
}
- return 0;
+ do {
+ conn = NULL;
+ list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) {
+ if (c->abort_reason == reason)
+ continue;
+
+ conn = c;
+ break;
+ }
+ if (!conn)
+ break;
+
+ conn->abort_reason = reason;
+ hci_conn_get(conn);
+
+ rcu_read_unlock();
+
+ err = hci_abort_conn_sync(hdev, conn, reason);
+ hci_conn_put(conn);
+
+ rcu_read_lock();
+ } while (!err);
+
+ rcu_read_unlock();
+
+ return err;
}
/* This function perform power off HCI command sequence as follows:
--
2.41.0
Hi Pauli,
On Sun, Aug 6, 2023 at 2:39 PM Pauli Virtanen <[email protected]> wrote:
>
> Use-after-free occurs in hci_disconnect_all_sync if a connection is
> deleted by concurrent processing of a controller event. This can occur
> while waiting for controller events (big time window) or at other times
> (less likely).
>
> Fix the iteration in hci_disconnect_all_sync to allow hci_conn to be
> deleted at any time.
>
> 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 <[email protected]>
> ---
>
> Notes:
> v2: use only valid values for abort_reason, in case we fail before
> handling all conns
>
> This is still a bit clumsy, a separate flag indicating if connection was
> aborted yet could be better.
I suspect this is caused by the links being removed when the ACL is
removed, so perhaps disconnect_all shall actually use
list_for_each_entry_safe_reverse so we disconnect the links before we
attempt to disconnect the parents. That said we still have the risk
that if there is an event in the meantime that messes up with the list
past the previous item then the whole thing is not safe, so perhaps we
should switch to hci_conn_hash_flush method and guarantee hci_conn_del
has been called.
> net/bluetooth/hci_sync.c | 47 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 51ff682f66e0..228259f44815 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5415,16 +5415,49 @@ 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 hci_conn *c, *conn;
> + int err = 0;
>
> - list_for_each_entry_safe(conn, tmp, &hdev->conn_hash.list, list) {
> - err = hci_abort_conn_sync(hdev, conn, reason);
> - if (err)
> - return err;
> + rcu_read_lock();
> +
> + /* Any conn may be gone while waiting for events, iterate safely.
> + * If conn is in conn_hash and we didn't abort it, it probably
> + * has not yet been aborted.
> + */
> + list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) {
> + if (c->abort_reason != reason)
> + continue;
> +
> + c->abort_reason = (reason != HCI_ERROR_REMOTE_USER_TERM) ?
> + HCI_ERROR_REMOTE_USER_TERM : HCI_ERROR_UNSPECIFIED;
> }
>
> - return 0;
> + do {
> + conn = NULL;
> + list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) {
> + if (c->abort_reason == reason)
> + continue;
> +
> + conn = c;
> + break;
> + }
> + if (!conn)
> + break;
> +
> + conn->abort_reason = reason;
> + hci_conn_get(conn);
> +
> + rcu_read_unlock();
> +
> + err = hci_abort_conn_sync(hdev, conn, reason);
> + hci_conn_put(conn);
> +
> + rcu_read_lock();
> + } while (!err);
> +
> + rcu_read_unlock();
> +
> + return err;
> }
>
> /* This function perform power off HCI command sequence as follows:
> --
> 2.41.0
>
--
Luiz Augusto von Dentz