[PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del

[PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del

[Michael Bommarito]

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

Re: [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del

[Luiz Augusto von Dentz]

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

Re: [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del

[Michael Bommarito]

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

Re: [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del

[Pauli Virtanen]

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

Re: [PATCH] Bluetooth: HIDP: guard session->conn in hidp_connection_del

[Luiz Augusto von Dentz]

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

[PATCH v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem

[Michael Bommarito]

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

Re: [PATCH v2] Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem

[patchwork-bot+bluetooth]

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