[PW_SID:1153622] Bluetooth: L2CAP: fix and annotate l2cap_conn::chan_l locking - #664
Open
BluezTestBot wants to merge 21 commits into
Open
[PW_SID:1153622] Bluetooth: L2CAP: fix and annotate l2cap_conn::chan_l locking#664BluezTestBot wants to merge 21 commits into
BluezTestBot wants to merge 21 commits into
Conversation
This patch adds workflow files for ci: [sync.yml] - The workflow file for scheduled work - Sync the repo with upstream repo and rebase the workflow branch - Review the patches in the patchwork and creates the PR if needed [ci.yml] - The workflow file for CI tasks - Run CI tests when PR is created Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com>
This replaces the bzcafe action with bluez/action-ci so we can maintain everything in the github bluez organization Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This attempts to sync every 5 minutes instead of 30. Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
bluez/action-ci uses master as default branch for workflow which is incorrect for kernel Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
The CI action now creates individual GitHub Check Runs per test, which requires 'checks: write' permission on the GITHUB_TOKEN. Also make the pull_request trigger types explicit to include 'reopened', allowing CI to be retriggered by closing and reopening a PR.
chan->lock must be held for __l2cap_chan_add as eg. calls to l2cap_chan_close assume chan->conn writes are guarded by it. It must be held for l2cap_chan_del() due to l2cap_sock.c:l2cap_chan_conn, l2cap_monitor_timeout, etc. Similarly it should be held for l2cap_ops::ready (assumed in 6lowpan.c). Also teardown usually has chan->lock held, it should always have it held to have the same locking context. The lock is not correctly held by l2cap_core in several places. Add the missing locks for l2cap_chan_del/add/ready(), except in l2cap_ecred_rsp_defer() which needs separate fix as it needs lock nesting. Fixes: 6fef032 ("Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb()") Signed-off-by: Pauli Virtanen <pav@iki.fi>
l2cap_chan_close() requires holding chan->lock and chan->conn->lock if associated chan->conn exists, to guard eg. conn->chan_l. Taking the locks with right ordering requires handling a race condition. Add helper function l2cap_chan_(un)lock_conn that do the locking right. Add l2cap_chan_close_unlocked() that does not require locks to be held, as all callsites do this lock -> close -> unlock pattern. Link: https://syzkaller.appspot.com/bug?extid=0e4ebcc970728e056324 Signed-off-by: Pauli Virtanen <pav@iki.fi>
l2cap_sock_shutdown() has the race condition
[Task 1] [Task 2]
l2cap_sock_shutdown l2cap_sock_connect
l2cap_chan_lock l2cap_chan_connect
conn = ... /* == NULL*/
l2cap_chan_unlock ------------> l2cap_chan_lock
if (conn) /* false */
__l2cap_chan_add(conn, chan)
l2cap_chan_lock <-------------- l2cap_chan_unlock
l2cap_chan_close /* chan->conn->lock not held! */
conn->lock protects conn->chan_l and is not properly held here.
Use the l2cap_chan_close_unlocked() helper that ensures conn->lock is
held for l2cap_chan_close().
Fixes: ab4eedb ("Bluetooth: L2CAP: Fix corrupted list in hci_chan_del")
Reported-by: Eulgyu Kim <eulgyukim@snu.ac.kr>
Reported-by: Jaeyoung Chung <jjy600901@snu.ac.kr>
Link: https://lore.kernel.org/linux-bluetooth/20260824153908.2327306-1-jjy600901@snu.ac.kr/
Link: https://syzkaller.appspot.com/bug?extid=0e4ebcc970728e056324
Signed-off-by: Pauli Virtanen <pav@iki.fi>
6lowpan.c is using l2cap_chan_close() without taking chan->conn->lock, so it may modify conn->chan_l without holding the guarding lock. Fix the locking by using the l2cap_chan_close_unlocked() helper that acquires the necessary locks. Fixes: 15f32ca ("Bluetooth: 6lowpan: add missing l2cap_chan_lock()") Link: https://syzkaller.appspot.com/bug?extid=0e4ebcc970728e056324 Signed-off-by: Pauli Virtanen <pav@iki.fi>
l2cap_chan_close() is now unused, and l2cap_chan_close_unlocked() should be used instead. Remove l2cap_chan_close(). Signed-off-by: Pauli Virtanen <pav@iki.fi>
bt_6lowpan_disconnect() looks up and accesses peer->chan, without holding locks guaranteeing peer_del() cannot free the peer concurrently. Take devices_lock to ensure peer can be dereferenced safely. Fixes: 15f32ca ("Bluetooth: 6lowpan: add missing l2cap_chan_lock()") Signed-off-by: Pauli Virtanen <pav@iki.fi>
__l2cap_ecred_conn_rsp_defer() > __l2cap_chan_list_id() accesses conn->chan_l which is guarded by conn->lock. The lock fails to be held when calling from l2cap_sock.c. Fix by using l2cap_chan_conn_lock(), and taking the locks in required order l2cap_conn::lock > l2cap_chan::lock > sk. Leave fast path with sk->sk_state precheck. Move the L2CAP defer handling to l2cap_sock_defer(). The code should also take l2cap_chan_lock() for sibling channels, but that needs separate fix due to lock nesting. Fixes: ab4eedb ("Bluetooth: L2CAP: Fix corrupted list in hci_chan_del") Signed-off-by: Pauli Virtanen <pav@iki.fi>
l2cap_new_connection() -> __l2cap_chan_add() modifies l2cap_conn::chan_l, which is guarded by l2cap_conn::lock. The lock is not held in l2cap_connect_cfm(). Fix by holding conn->lock in l2cap_connect_cfm() to make the locking systematic. Fixes: ab4eedb ("Bluetooth: L2CAP: Fix corrupted list in hci_chan_del") Signed-off-by: Pauli Virtanen <pav@iki.fi>
Add context analysis annotations for l2cap_conn::chan_l and chan_list locking. Add corresponding required annotations to accessors and callers. This is not complete chan_l annotation, l2cap_chan::list and l2cap_chan_del() locking is currently not fully correct, and needs separate fix + annotations. Signed-off-by: Pauli Virtanen <pav@iki.fi>
l2cap_ecred_rsp_defer() calls l2cap_chan_del without holding chan->lock, which ends up calling ops->teardown() with wrong lock context. Fix by taking chan->lock in l2cap_ecred_rsp_defer(). AB-BA deadlocks between sibling l2cap_chan are avoided here via requiring l2cap_conn::lock to serialize all nested l2cap_chan locking on same nesting level. In current code, there is no nested l2cap_chan locking on same nesting level, so we can add this new requirement. Also return early from __l2cap_ecred_conn_rsp_defer() if chan did not have FLAG_DEFER_SETUP, as then no RSP shall be sent for it, to make sure SMP channels are excluded. Also hold chan reference over l2cap_chan_del(), in case chan_l reference was the last. Signed-off-by: Pauli Virtanen <pav@iki.fi>
l2cap_chan_del() calls l2cap_chan_put() to drop the conn->chan_l reference. If this was the last reference, UAF follows. l2cap_ecred_conn_rsp() iterates chan_l list and calls l2cap_chan_del() on some members, without holding chan reference. Fix by holding refcount while using chan after l2cap_chan_del(). Since orig is looked up by dcid provided by remote, it's also possible orig == chan, so reference needs to be held also after orig use. Fixes: 41c2713 ("Bluetooth: L2CAP: Fix possible crash on l2cap_ecred_conn_rsp") Assisted-by: deepseek-v4-flash Signed-off-by: Pauli Virtanen <pav@iki.fi>
Add context analysis annotations for chan->lock and chan->conn->lock involving l2cap_chan_del() usage. Add necessary annotations and related lockdep_assert_held to callers. Move struct l2cap_ops definition after struct l2cap_conn, so that the callbacks can be annotated. In l2cap_chan_close_unlocked() we consider chan->conn->lock as locked even if chan->conn == NULL, to avoid needing to define separate __l2cap_chan_close/del for this NULL case. Signed-off-by: Pauli Virtanen <pav@iki.fi>
Annotate current locking context for l2cap_ops callbacks. Signed-off-by: Pauli Virtanen <pav@iki.fi>
Since l2cap_set_timer() does not check return value of schedule_delayed_work(), two concurrent calls may result to l2cap_chan refcount leak. Change the refcounting by using mod_delayed_work() and checking its return value. Code paths aside from l2cap_chan_busy() hold chan->lock, so this has little correctness impact. Signed-off-by: Pauli Virtanen <pav@iki.fi>
Context analysis does not understand conditional locking. Restructure l2cap_connect() by removing conditional locking at the cost of some code duplication, so that static analysis can see its content. Signed-off-by: Pauli Virtanen <pav@iki.fi>
l2cap_chan may be linked to l2cap_conn at most once. This is assumed in
several places, eg l2cap_chan_del cleanup.
There is a TOCTOU race where the invariant is violated:
[Task 1] [Task 2]
l2cap_chan_connect l2cap_sock_bind
l2cap_chan_lock lock_sock
l2cap_state_change if (sk->sk_state != BT_OPEN)
chan->state = BT_CONNECT
l2cap_sock_state_change_cb chan->state = BT_BOUND
sk->sk_state = BT_BOUND
lock_sock <------------------ release_sock
sk->sk_state = BT_CONNECT
l2cap_sock_connect() does not check sk->sk_state, so since chan->state
is now BT_BOUND, subsequent connect() ends up with second
__l2cap_chan_add.
Explicitly document and check the invariant in __l2cap_chan_add with
WARN_ON_ONCE. The only callsite where it could be hit is
l2cap_chan_connect, so add pre-check there to avoid relying on
chan->state. chan->state read/write is not properly guarded currently so
there can be other TOCTOUC problems.
Add l2cap_lock_chan in l2cap_sock_bind() to guard chan->state write.
Fixes: b66774b ("Bluetooth: L2CAP: Fix UAF in channel timeout by holding conn ref")
Assisted-by: deepseek-4-flash # finding the race condition
Signed-off-by: Pauli Virtanen <pav@iki.fi>
|
CheckPatch |
|
VerifyFixes |
|
VerifySignedoff |
|
GitLint |
|
SubjectPrefix |
|
BuildKernel |
|
CheckAllWarning |
|
CheckSparse |
|
BuildKernel32 |
|
CheckKernelLLVM |
|
TestRunnerSetup |
|
TestRunner_l2cap-tester |
|
TestRunner_6lowpan-tester |
|
IncrementalBuild |
github-actions
Bot
force-pushed
the
workflow
branch
3 times, most recently
from
September 1, 2026 22:08
1424fd6 to
b3d5ba7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
chan->lock must be held for __l2cap_chan_add as eg. calls to
l2cap_chan_close assume chan->conn writes are guarded by it.
It must be held for l2cap_chan_del() due to
l2cap_sock.c:l2cap_chan_conn, l2cap_monitor_timeout, etc.
Similarly it should be held for l2cap_ops::ready (assumed in 6lowpan.c).
Also teardown usually has chan->lock held, it should always have it held
to have the same locking context.
The lock is not correctly held by l2cap_core in several places.
Add the missing locks for l2cap_chan_del/add/ready(), except in
l2cap_ecred_rsp_defer() which needs separate fix as it needs lock
nesting.
Fixes: 6fef032 ("Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb()")
Signed-off-by: Pauli Virtanen pav@iki.fi
include/net/bluetooth/l2cap.h | 3 ++-
net/bluetooth/l2cap_core.c | 15 +++++++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)