MIME-Version: 1.0
References: <20230123091708.4112735-1-git@sung-woo.kim> <20230202090509.2774062-1-iam@sung-woo.kim>
In-Reply-To: <20230202090509.2774062-1-iam@sung-woo.kim>
From:   Eric Dumazet <edumazet@google.com>
Date:   Thu, 2 Feb 2023 10:26:21 +0100
Message-ID: <CANn89i+hAht=g1F6kjPfq8eO4j6-2WEE+CNtRtq1S4UnwXEQaw@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: L2CAP: Fix use-after-free
To:     Sungwoo Kim <iam@sung-woo.kim>
Cc:     happiness.sung.woo@gmail.com, benquike@gmail.com,
        davem@davemloft.net, daveti@purdue.edu, johan.hedberg@gmail.com,
        kuba@kernel.org, linux-bluetooth@vger.kernel.org,
        linux-kernel@vger.kernel.org, luiz.dentz@gmail.com,
        marcel@holtmann.org, netdev@vger.kernel.org, pabeni@redhat.com,
        wuruoyu@me.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 2, 2023 at 10:07 AM Sungwoo Kim <iam@sung-woo.kim> 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@sung-woo.kim>
> ---
>  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
>