Commit c5d41559 authored May 02, 2026 by Michael Bommarito Committed by Luiz Augusto von Dentz May 06, 2026

Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem



Commit dbf666e4 ("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 752a6c95
("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: dbf666e4 ("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
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

parent 72d97cae

net/bluetooth/hidp/core.c

+24 −3

Original line number	Diff line number	Diff line
		@@ -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);