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@xxxxxxxxx> 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@xxxxxxxxx>
> 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