Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2244-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240301185034.2756103-1-luiz.dentz@gmail.com>
In-Reply-To: <20240301185034.2756103-1-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Fri, 1 Mar 2024 14:00:10 -0500
Message-ID: <CABBYNZ+0o2EqGtub9jBGHC+DLdb5ZNXbcc9FXquLtAxRAvGVVg@mail.gmail.com>
Subject: Re: [PATCH v1] Bluetooth: af_bluetooth: Fix deadlock
To: linux-bluetooth@vger.kernel.org
Cc: Hyunwoo Kim <v4bel@theori.io>, Simon Kirby <sim@hostway.ca>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 1, 2024 at 1:50=E2=80=AFPM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Attemting to do sock_lock on .recvmsg may cause a deadlock as shown
> bellow, so instead of using sock_sock this uses sk_receive_queue.lock
> on bt_sock_ioctl to avoid the UAF:
>
> INFO: task kworker/u9:1:121 blocked for more than 30 seconds.
>       Not tainted 6.7.6-lemon #183
> Workqueue: hci0 hci_rx_work
> Call Trace:
>  <TASK>
>  __schedule+0x37d/0xa00
>  schedule+0x32/0xe0
>  __lock_sock+0x68/0xa0
>  ? __pfx_autoremove_wake_function+0x10/0x10
>  lock_sock_nested+0x43/0x50
>  l2cap_sock_recv_cb+0x21/0xa0
>  l2cap_recv_frame+0x55b/0x30a0
>  ? psi_task_switch+0xeb/0x270
>  ? finish_task_switch.isra.0+0x93/0x2a0
>  hci_rx_work+0x33a/0x3f0
>  process_one_work+0x13a/0x2f0
>  worker_thread+0x2f0/0x410
>  ? __pfx_worker_thread+0x10/0x10
>  kthread+0xe0/0x110
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork+0x2c/0x50
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork_asm+0x1b/0x30
>  </TASK>
>
> Fixes: 2e07e8348ea4 ("Bluetooth: af_bluetooth: Fix Use-After-Free in bt_s=
ock_recvmsg")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  net/bluetooth/af_bluetooth.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index b93464ac3517..67604ccec2f4 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -309,14 +309,11 @@ int bt_sock_recvmsg(struct socket *sock, struct msg=
hdr *msg, size_t len,
>         if (flags & MSG_OOB)
>                 return -EOPNOTSUPP;
>
> -       lock_sock(sk);
> -
>         skb =3D skb_recv_datagram(sk, flags, &err);
>         if (!skb) {
>                 if (sk->sk_shutdown & RCV_SHUTDOWN)
>                         err =3D 0;
>
> -               release_sock(sk);
>                 return err;
>         }
>
> @@ -346,8 +343,6 @@ int bt_sock_recvmsg(struct socket *sock, struct msghd=
r *msg, size_t len,
>
>         skb_free_datagram(sk, skb);
>
> -       release_sock(sk);
> -
>         if (flags & MSG_TRUNC)
>                 copied =3D skblen;
>
> @@ -570,10 +565,11 @@ int bt_sock_ioctl(struct socket *sock, unsigned int=
 cmd, unsigned long arg)
>                 if (sk->sk_state =3D=3D BT_LISTEN)
>                         return -EINVAL;
>
> -               lock_sock(sk);
> +               spin_lock(&sk->sk_receive_queue.lock);
>                 skb =3D skb_peek(&sk->sk_receive_queue);
>                 amount =3D skb ? skb->len : 0;
> -               release_sock(sk);
> +               spin_unlock(&sk->sk_receive_queue.lock);
> +
>                 err =3D put_user(amount, (int __user *)arg);
>                 break;
>
> --
> 2.43.0
>


--=20
Luiz Augusto von Dentz