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
>
