Re: [PATCH] Bluetooth: L2CAP: Fix use-after-free

[Eric Dumazet]

On Thu, Feb 2, 2023 at 10:07 AM Sungwoo Kim <iam@xxxxxxxxxxxx> wrote:
>
> Due to the race condition between l2cap_sock_cleanup_listen and
> l2cap_sock_close_cb, l2cap_sock_kill can receive already freed sk,
> resulting in use-after-free inside l2cap_sock_kill.
> This patch prevent this by adding a null check in l2cap_sock_kill.
>
> Context 1:
> l2cap_sock_cleanup_listen();
>   // context switched
>   l2cap_chan_lock(chan);
>   l2cap_sock_kill(sk); // <-- sk is already freed below

But sk is used in l2cap_sock_cleanup_listen()
and should not be NULL...

while ((sk = bt_accept_dequeue(parent, NULL))) {
  ...
  l2cap_sock_kill(sk);
  ..
}

It would help if you send us a stack trace ...

>
> Context 2:
> l2cap_chan_timeout();
>   l2cap_chan_lock(chan);
>   chan->ops->close(chan);
>     l2cap_sock_close_cb()
>     l2cap_sock_kill(sk); // <-- sk is freed here
>   l2cap_chan_unlock(chan);
>

Please add a Fixes: tag

> Signed-off-by: Sungwoo Kim <iam@xxxxxxxxxxxx>
> ---
>  net/bluetooth/l2cap_sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index ca8f07f35..657704059 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1245,7 +1245,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>   */
>  static void l2cap_sock_kill(struct sock *sk)
>  {
> -       if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
> +       if (!sk || !sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
>                 return;
>
>         BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
> --
> 2.25.1
>