Re: [PATCH v2] Bluetooth: ISO: fix use-after-free of listener socket in iso_conn_ready

to, 2026-08-13 kello 19:39 +0800, Hang Nan kirjoitti:
> iso_conn_ready() looks up the BIS listener socket with iso_get_sock(),
> which takes a reference, and then, without re-checking its state,
> creates a child socket from it:
> 
>     parent = iso_get_sock(hdev, ...);
>     if (!parent)
>         return;
> 
>     lock_sock(parent);
>     sk = iso_sock_alloc(sock_net(parent), NULL, BTPROTO_ISO, ...);
>     ...
>     iso_chan_add(conn, sk, parent);
>     ...
>     release_sock(parent);
>     sock_put(parent);
> 
> If the listener socket is closed concurrently, between iso_get_sock()
> and lock_sock(), the reference taken by iso_get_sock() may be the last
> one: the close path drops the link-list reference, and once
> iso_conn_ready() drops its own reference at the end of the function the
> socket is freed.  The child socket, however, is already linked to the
> freed parent, and a later disconnect of the child runs iso_chan_del()
> -> bt_accept_unlink(), which dereferences the dangling parent pointer
> into the freed accept queue (a use-after-free).  The same dangling
> pointer is also dereferenced through parent->***() in
> iso_chan_del().
> 
> Fix it the same way the connected (non-BIS) path was fixed in commit
> 0d255e63fcf3 ("Bluetooth: ISO: hold sk properly in iso_conn_ready"):
> after taking the socket lock, re-check that the parent is still a
> listening, alive socket, and bail out otherwise.
> 
> Fixes: ccf74f2390d60 ("Bluetooth: Add BTPROTO_ISO socket type")
> Cc: [email protected]
> 
> Changes in v2:
> - Fix GitLint B3: replace hard tabs with spaces in the commit message
>   code snippet (no functional change)

Changelog should not be in the commit message, but below the --- line.

> Signed-off-by: Hang Nan <[email protected]>
> ---
>  net/bluetooth/iso.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index aa2ce78f56a2..069fc87a4e18 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -2277,6 +2277,21 @@ static void iso_conn_ready(struct iso_conn *conn)
>  
>  		lock_sock(parent);
>  
> +		/* The listener socket may have been closed concurrently
> +		 * between iso_get_sock() and lock_sock(): the reference
> +		 * taken by iso_get_sock() may be the last one, in which
> +		 * case the socket is freed as soon as we drop it at the
> +		 * end of this function.  Recheck that the parent is still
> +		 * a valid, listening socket before creating a child
> +		 * socket from it.
> +		 */

The explanations are long. Assisted-by: some LLM?

> +		if (parent->sk_state != BT_LISTEN ||
> +		    sock_flag(parent, SOCK_ZAPPED)) {
> +			release_sock(parent);
> +			sock_put(parent);
> +			return;
> +		}

The change itsef looks good to me.

> +
>  		sk = iso_sock_alloc(sock_net(parent), NULL,
>  				    BTPROTO_ISO, GFP_ATOMIC, 0);
>  		if (!sk) {

-- 
Pauli Virtanen