* [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del [not found] <20260306023155.554597-1-luiz.dentz@gmail.com> @ 2026-04-22 1:14 ` Michael Bommarito 2026-04-22 14:55 ` Luiz Augusto von Dentz 2026-05-02 16:43 ` [PATCH v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem Michael Bommarito 0 siblings, 2 replies; 7+ messages in thread From: Michael Bommarito @ 2026-04-22 1:14 UTC (permalink / raw) To: Marcel Holtmann; +Cc: Luiz Augusto von Dentz, linux-bluetooth, linux-kernel commit dbf666e4fc9b ("Bluetooth: HIDP: Fix possible UAF") changed hidp_session_remove() to drop the L2CAP reference and set session->conn = NULL once the session is considered removed, and added an if (session->conn) guard around the l2cap_unregister_user() call at the kthread-exit site in hidp_session_thread(). The sibling call site in hidp_connection_del() still invokes l2cap_unregister_user(session->conn, &session->user) unconditionally. hidp_session_find() takes the session refcount under down_read(&hidp_session_sem) and returns; between the find() and the call at :1421, hidp_session_remove() can run on another thread (driven by the remote peer disconnecting or local teardown), take down_write(&hidp_session_sem), set session->conn to NULL, and return. The HIDPCONNDEL ioctl path then dereferences a NULL l2cap_conn inside l2cap_unregister_user(), which acquires conn->lock without a NULL check. Result: kernel NULL-pointer dereference. Apply the same if (session->conn) guard used at the twin site. No functional change when session->conn is non-NULL. Discovery and verification: - Found via static audit of every session->conn read in hidp/core.c after the referenced commit landed. The other reads are safe (creation-time in hidp_session_dev_init, already-guarded in session_free / hidp_session_thread / hidp_session_remove; the other hidp_session_find callers do not touch session->conn at all), so :1421 is the only remaining unguarded site. - Runtime A/B confirmed in UML with CONFIG_BT_HIDP=y + CONFIG_KASAN=y: a late_initcall stub that injects a fake hidp_session with conn=NULL into hidp_session_list and invokes hidp_connection_del() panics on the pre-fix tree at __mutex_lock from l2cap_unregister_user+0x2d, and returns cleanly on the post-fix tree with the new guard short-circuiting before the deref. Fixes: dbf666e4fc9b ("Bluetooth: HIDP: Fix possible UAF") Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com> Assisted-by: Claude:claude-opus-4-7 --- net/bluetooth/hidp/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c index 7bcf8c5ceaee..9192efd1b156 100644 --- a/net/bluetooth/hidp/core.c +++ b/net/bluetooth/hidp/core.c @@ -1417,7 +1417,7 @@ int hidp_connection_del(struct hidp_conndel_req *req) HIDP_TRANS_HID_CONTROL | HIDP_CTRL_VIRTUAL_CABLE_UNPLUG, NULL, 0); - else + else if (session->conn) l2cap_unregister_user(session->conn, &session->user); hidp_session_put(session); -- 2.53.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del 2026-04-22 1:14 ` [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del Michael Bommarito @ 2026-04-22 14:55 ` Luiz Augusto von Dentz 2026-04-22 15:09 ` Michael Bommarito 2026-05-02 16:43 ` [PATCH v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem Michael Bommarito 1 sibling, 1 reply; 7+ messages in thread From: Luiz Augusto von Dentz @ 2026-04-22 14:55 UTC (permalink / raw) To: Michael Bommarito; +Cc: Marcel Holtmann, linux-bluetooth, linux-kernel Hi Michael, On Tue, Apr 21, 2026 at 9:14 PM Michael Bommarito <michael.bommarito@gmail.com> wrote: > > commit dbf666e4fc9b ("Bluetooth: HIDP: Fix possible UAF") changed > hidp_session_remove() to drop the L2CAP reference and set > session->conn = NULL once the session is considered removed, and > added an if (session->conn) guard around the l2cap_unregister_user() > call at the kthread-exit site in hidp_session_thread(). > > The sibling call site in hidp_connection_del() still invokes > l2cap_unregister_user(session->conn, &session->user) unconditionally. > hidp_session_find() takes the session refcount under > down_read(&hidp_session_sem) and returns; between the find() and the > call at :1421, hidp_session_remove() can run on another thread > (driven by the remote peer disconnecting or local teardown), take > down_write(&hidp_session_sem), set session->conn to NULL, and return. > The HIDPCONNDEL ioctl path then dereferences a NULL l2cap_conn inside > l2cap_unregister_user(), which acquires conn->lock without a NULL > check. Result: kernel NULL-pointer dereference. > > Apply the same if (session->conn) guard used at the twin site. No > functional change when session->conn is non-NULL. > > Discovery and verification: > > - Found via static audit of every session->conn read in hidp/core.c > after the referenced commit landed. The other reads are safe > (creation-time in hidp_session_dev_init, already-guarded in > session_free / hidp_session_thread / hidp_session_remove; the other > hidp_session_find callers do not touch session->conn at all), so > :1421 is the only remaining unguarded site. > - Runtime A/B confirmed in UML with CONFIG_BT_HIDP=y + CONFIG_KASAN=y: > a late_initcall stub that injects a fake hidp_session with > conn=NULL into hidp_session_list and invokes hidp_connection_del() > panics on the pre-fix tree at __mutex_lock from > l2cap_unregister_user+0x2d, and returns cleanly on the post-fix > tree with the new guard short-circuiting before the deref. > > Fixes: dbf666e4fc9b ("Bluetooth: HIDP: Fix possible UAF") > Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com> > Assisted-by: Claude:claude-opus-4-7 > --- > net/bluetooth/hidp/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c > index 7bcf8c5ceaee..9192efd1b156 100644 > --- a/net/bluetooth/hidp/core.c > +++ b/net/bluetooth/hidp/core.c > @@ -1417,7 +1417,7 @@ int hidp_connection_del(struct hidp_conndel_req *req) > HIDP_TRANS_HID_CONTROL | > HIDP_CTRL_VIRTUAL_CABLE_UNPLUG, > NULL, 0); > - else > + else if (session->conn) > l2cap_unregister_user(session->conn, &session->user); > > hidp_session_put(session); > -- > 2.53.0 We might need a lock in order to access the session->conn: https://sashiko.dev/#/patchset/20260422011437.176643-1-michael.bommarito%40gmail.com -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del 2026-04-22 14:55 ` Luiz Augusto von Dentz @ 2026-04-22 15:09 ` Michael Bommarito 2026-04-22 15:48 ` Pauli Virtanen 2026-04-22 15:49 ` Luiz Augusto von Dentz 0 siblings, 2 replies; 7+ messages in thread From: Michael Bommarito @ 2026-04-22 15:09 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Marcel Holtmann, linux-bluetooth, linux-kernel On Wed, Apr 22, 2026 at 10:55 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > We might need a lock in order to access the session->conn: This one is a little easier than the other txwin_size issue in terms of blast radius. What pattern would you prefer here? Option 1, smaller but ordering questions: hold the semaphore across check and use like this: down_read(&hidp_session_sem); if (session->conn) l2cap_unregister_user(session->conn, &session->user); up_read(&hidp_session_sem) Option 2, more correct but more cycles: snapshot the conn and use outside down_read(&hidp_session_sem); conn = session->conn; if (conn) l2cap_conn_get(conn); up_read(&hidp_session_sem); if (conn) { l2cap_unregister_user(conn, &session->user); l2cap_conn_put(conn); } Thanks, Mike Bommarito ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del 2026-04-22 15:09 ` Michael Bommarito @ 2026-04-22 15:48 ` Pauli Virtanen 2026-04-22 15:49 ` Luiz Augusto von Dentz 1 sibling, 0 replies; 7+ messages in thread From: Pauli Virtanen @ 2026-04-22 15:48 UTC (permalink / raw) To: Michael Bommarito, Luiz Augusto von Dentz Cc: Marcel Holtmann, linux-bluetooth, linux-kernel Hi, ke, 2026-04-22 kello 11:09 -0400, Michael Bommarito kirjoitti: > On Wed, Apr 22, 2026 at 10:55 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > We might need a lock in order to access the session->conn: > > This one is a little easier than the other txwin_size issue in terms > of blast radius. > > What pattern would you prefer here? > > Option 1, smaller but ordering questions: hold the semaphore across > check and use like this: > > down_read(&hidp_session_sem); > if (session->conn) > l2cap_unregister_user(session->conn, &session->user); > up_read(&hidp_session_sem) > > > Option 2, more correct but more cycles: snapshot the conn and use outside > > down_read(&hidp_session_sem); > conn = session->conn; > if (conn) > l2cap_conn_get(conn); > up_read(&hidp_session_sem); > if (conn) { > l2cap_unregister_user(conn, &session->user); > l2cap_conn_put(conn); > } I'm not sure now (would need to rethink it through), but one probably should check if dbf666e4fc9b is needed or whether 752a6c9596d alone would be enough to address the original issue. -- Pauli Virtanen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del 2026-04-22 15:09 ` Michael Bommarito 2026-04-22 15:48 ` Pauli Virtanen @ 2026-04-22 15:49 ` Luiz Augusto von Dentz 1 sibling, 0 replies; 7+ messages in thread From: Luiz Augusto von Dentz @ 2026-04-22 15:49 UTC (permalink / raw) To: Michael Bommarito; +Cc: Marcel Holtmann, linux-bluetooth, linux-kernel Hi Michael, On Wed, Apr 22, 2026 at 11:09 AM Michael Bommarito <michael.bommarito@gmail.com> wrote: > > On Wed, Apr 22, 2026 at 10:55 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > We might need a lock in order to access the session->conn: > > This one is a little easier than the other txwin_size issue in terms > of blast radius. > > What pattern would you prefer here? > > Option 1, smaller but ordering questions: hold the semaphore across > check and use like this: > > down_read(&hidp_session_sem); > if (session->conn) > l2cap_unregister_user(session->conn, &session->user); > up_read(&hidp_session_sem) > > > Option 2, more correct but more cycles: snapshot the conn and use outside > > down_read(&hidp_session_sem); > conn = session->conn; > if (conn) > l2cap_conn_get(conn); > up_read(&hidp_session_sem); > if (conn) { > l2cap_unregister_user(conn, &session->user); > l2cap_conn_put(conn); > } Neither seems completely correct, in my opinion. I guess we want to use option 2 but set the session->conn to NULL since it will call l2cap_unregister_user. > Thanks, > Mike Bommarito -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem 2026-04-22 1:14 ` [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del Michael Bommarito 2026-04-22 14:55 ` Luiz Augusto von Dentz @ 2026-05-02 16:43 ` Michael Bommarito 2026-05-04 17:10 ` patchwork-bot+bluetooth 1 sibling, 1 reply; 7+ messages in thread From: Michael Bommarito @ 2026-05-02 16:43 UTC (permalink / raw) To: Marcel Holtmann Cc: Luiz Augusto von Dentz, Pauli Virtanen, linux-bluetooth, linux-kernel Commit dbf666e4fc9b ("Bluetooth: HIDP: Fix possible UAF") made hidp_session_remove() drop the L2CAP reference and set session->conn = NULL once the session is considered removed, and added a bare if (session->conn) guard around the kthread-exit l2cap_unregister_user() call in hidp_session_thread(). The sibling ioctl site in hidp_connection_del() still reads session->conn unlocked and unguarded, and the kthread-exit guard itself is a lockless double-read. hidp_session_find() drops hidp_session_sem before returning, so hidp_session_remove() can null session->conn between the lookup and the call in hidp_connection_del(). Worse, since commit 752a6c9596dd ("Bluetooth: L2CAP: Fix use-after-free in l2cap_unregister_user") takes mutex_lock(&conn->lock) inside l2cap_unregister_user(), a stale non-NULL snapshot also UAFs on conn->lock. v1 only added an if (session->conn) guard at the ioctl site, which doesn't address either race; Luiz suggested snapshotting session->conn under the sem and clearing it before the call. Taking hidp_session_sem across l2cap_unregister_user() would be wrong: l2cap_conn_del() already establishes the lock order conn->lock -> hidp_session_sem via l2cap_unregister_all_users() -> user->remove == hidp_session_remove(), so taking hidp_session_sem before conn->lock would AB/BA deadlock. Factor a helper hidp_session_unregister_conn() that under down_write(&hidp_session_sem) snapshots session->conn and clears the member, then outside the sem calls l2cap_unregister_user() and l2cap_conn_put() on the snapshot. Call it from both hidp_connection_del() and hidp_session_thread()'s exit path. At most one consumer wins the write-sem; later callers observe session->conn == NULL and skip the unregister and put, so the reference hidp_session_new() took via l2cap_conn_get() is consumed exactly once. session_free() already tolerates a NULL session->conn. Fixes: dbf666e4fc9b ("Bluetooth: HIDP: Fix possible UAF") Suggested-by: Luiz Augusto von Dentz <luiz.dentz@gmail.com> Link: https://lore.kernel.org/all/20260422011437.176643-1-michael.bommarito@gmail.com/ Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com> Assisted-by: Claude:claude-opus-4-7 --- Tested under UML with CONFIG_BT_HIDP=y, CONFIG_KASAN=y, and CONFIG_PROVE_LOCKING=y. W=1 build of net/bluetooth/hidp/ clean; boot clean. A KCFLAGS-gated test stub (not part of this patch) drove hidp_session_unregister_conn() twice in a row against a fake session with conn = NULL: the helper short-circuited both times and lockdep stayed silent. net/bluetooth/hidp/core.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c index 7bcf8c5ceaee..976f91eeb745 100644 --- a/net/bluetooth/hidp/core.c +++ b/net/bluetooth/hidp/core.c @@ -1035,6 +1035,28 @@ static struct hidp_session *hidp_session_find(const bdaddr_t *bdaddr) return session; } +/* + * Consume session->conn: clear the member under hidp_session_sem, then + * l2cap_unregister_user() and l2cap_conn_put() the snapshot outside the + * sem. At most one caller wins; later callers see NULL and skip. The + * reference is the one hidp_session_new() took via l2cap_conn_get(). + */ +static void hidp_session_unregister_conn(struct hidp_session *session) +{ + struct l2cap_conn *conn; + + down_write(&hidp_session_sem); + conn = session->conn; + if (conn) + session->conn = NULL; + up_write(&hidp_session_sem); + + if (conn) { + l2cap_unregister_user(conn, &session->user); + l2cap_conn_put(conn); + } +} + /* * Start session synchronously * This starts a session thread and waits until initialization @@ -1311,8 +1333,7 @@ static int hidp_session_thread(void *arg) * Instead, this call has the same semantics as if user-space tried to * delete the session. */ - if (session->conn) - l2cap_unregister_user(session->conn, &session->user); + hidp_session_unregister_conn(session); hidp_session_put(session); @@ -1418,7 +1439,7 @@ int hidp_connection_del(struct hidp_conndel_req *req) HIDP_CTRL_VIRTUAL_CABLE_UNPLUG, NULL, 0); else - l2cap_unregister_user(session->conn, &session->user); + hidp_session_unregister_conn(session); hidp_session_put(session); -- 2.53.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem 2026-05-02 16:43 ` [PATCH v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem Michael Bommarito @ 2026-05-04 17:10 ` patchwork-bot+bluetooth 0 siblings, 0 replies; 7+ messages in thread From: patchwork-bot+bluetooth @ 2026-05-04 17:10 UTC (permalink / raw) To: Michael Bommarito; +Cc: marcel, luiz.dentz, pav, linux-bluetooth, linux-kernel Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Sat, 2 May 2026 12:43:03 -0400 you wrote: > Commit dbf666e4fc9b ("Bluetooth: HIDP: Fix possible UAF") made > hidp_session_remove() drop the L2CAP reference and set > session->conn = NULL once the session is considered removed, and > added a bare if (session->conn) guard around the kthread-exit > l2cap_unregister_user() call in hidp_session_thread(). The sibling > ioctl site in hidp_connection_del() still reads session->conn > unlocked and unguarded, and the kthread-exit guard itself is a > lockless double-read. > > [...] Here is the summary with links: - [v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem https://git.kernel.org/bluetooth/bluetooth-next/c/fd572908d364 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-05-04 17:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260306023155.554597-1-luiz.dentz@gmail.com>
2026-04-22 1:14 ` [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del Michael Bommarito
2026-04-22 14:55 ` Luiz Augusto von Dentz
2026-04-22 15:09 ` Michael Bommarito
2026-04-22 15:48 ` Pauli Virtanen
2026-04-22 15:49 ` Luiz Augusto von Dentz
2026-05-02 16:43 ` [PATCH v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem Michael Bommarito
2026-05-04 17:10 ` patchwork-bot+bluetooth
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox