Bluetooth: HIDP: serialise l2cap_unregister_user via hidp_session_sem

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
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 7bcf8c5ceaeedc837bdbcf562a9a9a2f30ebb2f3..976f91eeb745e4925f2c5c2da6c318eb0bfa308d 100644 (file)
--- 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);