Re: [PATCH] Bluetooth: L2CAP: fix chan mode for LE_CONN_REQ + EXT_FLOWCTL pchan

[Pauli Virtanen]

su, 2026-08-30 kello 20:11 +0300, Pauli Virtanen kirjoitti:
> l2cap_new_connection() sets default value of channel mode to match the
> parent channel.  l2cap_le_connect_req() left this at the default, and
> created L2CAP_MODE_EXT_FLOWCTL channels if listening pchan has that
> mode.  This causes FLAG_DEFER_SETUP channels to reply to
> L2CAP_LE_CONN_REQ with L2CAP_ECRED_CONN_RSP, which is incorrect.
> 
> It can also result to stack OOB write (of l2cap_alloc_cid determined
> values) in l2cap_ecred_rsp_defer(), as l2cap_le_connect_req() does not
> limit maximum number of deferred channels or check for duplicate ident.
> 
> Fix by setting chan->mode correctly in l2cap_le_connect_req().
> 
> Also check channel mode in l2cap_ecred_rsp_defer(), and do WARN_ON_ONCE
> instead of OOB write to make it less brittle.

On Sashiko review comments:

> > +	/* Unreachable, check in l2cap_ecred_conn_req. If reached, drop rest */
> > +	if (WARN_ON_ONCE(rsp->count >= ARRAY_SIZE(rsp->pdu.scid)))
> > +		rsp->pdu.rsp.result = cpu_to_le16(L2CAP_CR_LE_NO_MEM);
> > +
> >  	/* Include all channels pending with the same ident */
> >  	if (!rsp->pdu.rsp.result)
> >  		rsp_flex->dcid[rsp->count++] = cpu_to_le16(chan->scid);
> >  	else
> >  		l2cap_chan_del(chan, ECONNRESET);
> 
> Can this lead to a self-deadlock and list corruption?
> If the WARN_ON_ONCE condition is met, the result is set to L2CAP_CR_LE_NO_MEM,
> causing the code to fall through to the else branch and call l2cap_chan_del().
> When an L2CAP LE socket is created with BT_SK_DEFER_SETUP, the deferred
> setup path calls l2cap_sock_recvmsg() which acquires the socket lock, and
> then it calls __l2cap_ecred_conn_rsp_defer() without acquiring conn->lock.
> Calling l2cap_chan_del() here unconditionally calls list_del() on the
> unprotected channel list, which corrupts the list. Furthermore, it invokes
> the teardown callback, which attempts to acquire the socket lock again,
> resulting in a self-deadlock since l2cap_sock_recvmsg() already holds it.

This is about the l2cap_chan_del() call there which indeed can
deadlock, but it is pre-existing issue. The WARN_ON should also be
unreachable.

The pre-existing deadlock is reachable on bluetooth-next/master, as in
this test:

https://lore.kernel.org/linux-bluetooth/3d6b440c7c3a077daf91a47bbc578a2cd40f2a01.1788119838.git.pav@xxxxxx/

The locking is fixed in

https://lore.kernel.org/linux-bluetooth/3213ea451b4db947b3333c002ab19678431781eb.1788013041.git.pav@xxxxxx/

> >  	chan->ident = cmd->ident;
> > +	chan->mode = L2CAP_MODE_LE_FLOWCTL;
> >  
> >  	if (test_bit(FLAG_DEFER_SETUP, &chan->flags)) {
> >  		l2cap_state_change(chan, BT_CONNECT2);
> 
> This isn't a bug introduced by this patch, but does locklessly assigning
> fields like chan->mode here create a data race?
> In l2cap_le_connect_req(), l2cap_new_connection() is called earlier, which
> invokes l2cap_sock_new_connection_cb() and immediately enqueues the socket
> into the parent's accept queue. This exposes the socket to userspace before
> initialization is complete.
> If userspace accepts the socket and calls setsockopt(), could it race with
> these lockless initializations, allowing userspace to observe or corrupt
> the uninitialized socket state? This patch exacerbates the issue by adding
> another unprotected assignment.

These data races should be cleaned up later, probably adding
__guarded_by() annotations at the same time.

> Fixes: 15f02b910562 ("Bluetooth: L2CAP: Add initial code for Enhanced Credit Based Mode")
> Signed-off-by: Pauli Virtanen <pav@xxxxxx>
> ---
> 
> Notes:
>     This is another pre-existing issue from sashiko.dev reviews
> 
>  net/bluetooth/l2cap_core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 358b11eabd4f..fa7dbf5f448e 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3886,6 +3886,9 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
>  	struct l2cap_ecred_conn_rsp *rsp_flex =
>  		container_of(&rsp->pdu.rsp, struct l2cap_ecred_conn_rsp, hdr);
>  
> +	if (chan->mode != L2CAP_MODE_EXT_FLOWCTL)
> +		return;
> +
>  	/* Check if channel for outgoing connection or if it wasn't deferred
>  	 * since in those cases it must be skipped.
>  	 */
> @@ -3896,6 +3899,10 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
>  	/* Reset ident so only one response is sent */
>  	chan->ident = 0;
>  
> +	/* Unreachable, check in l2cap_ecred_conn_req. If reached, drop rest */
> +	if (WARN_ON_ONCE(rsp->count >= ARRAY_SIZE(rsp->pdu.scid)))
> +		rsp->pdu.rsp.result = cpu_to_le16(L2CAP_CR_LE_NO_MEM);
> +
>  	/* Include all channels pending with the same ident */
>  	if (!rsp->pdu.rsp.result)
>  		rsp_flex->dcid[rsp->count++] = cpu_to_le16(chan->scid);
> @@ -5064,6 +5071,7 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
>  	__set_chan_timer(chan, chan->ops->get_sndtimeo(chan));
>  
>  	chan->ident = cmd->ident;
> +	chan->mode = L2CAP_MODE_LE_FLOWCTL;
>  
>  	if (test_bit(FLAG_DEFER_SETUP, &chan->flags)) {
>  		l2cap_state_change(chan, BT_CONNECT2);

-- 
Pauli Virtanen