Re: [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release

[Pauli Virtanen]

Hi,

ti, 2024-07-16 kello 10:24 -0400, Sasha Levin kirjoitti:
> From: Edward Adam Davis <eadavis@xxxxxx>
> 
> [ Upstream commit 89e856e124f9ae548572c56b1b70c2255705f8fe ]

This one needed an additional fixup that I don't see AUTOSEL picked up,
otherwise it results to a worse regression:

https://lore.kernel.org/linux-bluetooth/20240624134637.3790278-1-luiz.dentz@xxxxxxxxx/

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f1a8f402f13f94263cf349216c257b2985100927


Looks like f1a8f402f13f94263cf349216c257b2985100927 also contains other
changes not related to this patch, seems like 
https://lore.kernel.org/linux-bluetooth/20240624144911.3817479-1-luiz.dentz@xxxxxxxxx/
was squashed.

> The problem occurs between the system call to close the sock and hci_rx_work,
> where the former releases the sock and the latter accesses it without lock protection.
> 
>            CPU0                       CPU1
>            ----                       ----
>            sock_close                 hci_rx_work
> 	   l2cap_sock_release         hci_acldata_packet
> 	   l2cap_sock_kill            l2cap_recv_frame
> 	   sk_free                    l2cap_conless_channel
> 	                              l2cap_sock_recv_cb
> 
> If hci_rx_work processes the data that needs to be received before the sock is
> closed, then everything is normal; Otherwise, the work thread may access the
> released sock when receiving data.
> 
> Add a chan mutex in the rx callback of the sock to achieve synchronization between
> the sock release and recv cb.
> 
> Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer.
> 
> Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 8645461d45e81..64827e553d638 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1239,6 +1239,10 @@ static void l2cap_sock_kill(struct sock *sk)
>  
>  	BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
>  
> +	/* Sock is dead, so set chan data to NULL, avoid other task use invalid
> +	 * sock pointer.
> +	 */
> +	l2cap_pi(sk)->chan->data = NULL;
>  	/* Kill poor orphan */
>  
>  	l2cap_chan_put(l2cap_pi(sk)->chan);
> @@ -1481,12 +1485,25 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
>  
>  static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
>  {
> -	struct sock *sk = chan->data;
> -	struct l2cap_pinfo *pi = l2cap_pi(sk);
> +	struct sock *sk;
> +	struct l2cap_pinfo *pi;
>  	int err;
>  
> -	lock_sock(sk);
> +	/* To avoid race with sock_release, a chan lock needs to be added here
> +	 * to synchronize the sock.
> +	 */
> +	l2cap_chan_hold(chan);
> +	l2cap_chan_lock(chan);
> +	sk = chan->data;
>  
> +	if (!sk) {
> +		l2cap_chan_unlock(chan);
> +		l2cap_chan_put(chan);
> +		return -ENXIO;
> +	}
> +
> +	pi = l2cap_pi(sk);
> +	lock_sock(sk);
>  	if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) {
>  		err = -ENOMEM;
>  		goto done;
> @@ -1535,6 +1552,8 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
>  
>  done:
>  	release_sock(sk);
> +	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  
>  	return err;
>  }

-- 
Pauli Virtanen