Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230403224412.2021594-1-luiz.dentz@gmail.com>
In-Reply-To: <20230403224412.2021594-1-luiz.dentz@gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 3 Apr 2023 15:52:02 -0700
Message-ID: <CABBYNZK74Q6Hm=g0dnnP16tqcU9ObSk6Sj6+2uRf=4aFNmUaeQ@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: hci_conn: Fix possible UAF
To:     linux-bluetooth@vger.kernel.org
Cc:     Archie Pusaka <apusaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Archie,

On Mon, Apr 3, 2023 at 3:44=E2=80=AFPM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This fixes the following trace:
>
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> BUG: KASAN: slab-use-after-free in hci_conn_del+0xba/0x3a0
> Write of size 8 at addr ffff88800208e9c8 by task iso-tester/31
>
> CPU: 0 PID: 31 Comm: iso-tester Not tainted 6.3.0-rc2-g991aa4a69a47
>  #4716
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc36
> 04/01/2014
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x1d/0x70
>  print_report+0xce/0x610
>  ? __virt_addr_valid+0xd4/0x150
>  ? hci_conn_del+0xba/0x3a0
>  kasan_report+0xdd/0x110
>  ? hci_conn_del+0xba/0x3a0
>  hci_conn_del+0xba/0x3a0
>  hci_conn_hash_flush+0xf2/0x120
>  hci_dev_close_sync+0x388/0x920
>  hci_unregister_dev+0x122/0x260
>  vhci_release+0x4f/0x90
>  __fput+0x102/0x430
>  task_work_run+0xf1/0x160
>  ? __pfx_task_work_run+0x10/0x10
>  ? mark_held_locks+0x24/0x90
>  exit_to_user_mode_prepare+0x170/0x180
>  syscall_exit_to_user_mode+0x19/0x50
>  do_syscall_64+0x4e/0x90
>  entry_SYSCALL_64_after_hwframe+0x70/0xda
>
> Fixes: 0f00cd322d22 ("Bluetooth: Free potentially unfreed SCO connection"=
)
> Link: https://syzkaller.appspot.com/bug?extid=3D8bb72f86fc823817bc5d
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  net/bluetooth/hci_conn.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index fe8d54f8f04f..5672b4924572 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1069,6 +1069,17 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev=
, int type, bdaddr_t *dst,
>         return conn;
>  }
>
> +static bool hci_conn_unlink(struct hci_conn *conn)
> +{
> +       if (!conn->link)
> +               return false;
> +
> +       conn->link->link =3D NULL;
> +       conn->link =3D NULL;
> +
> +       return true;
> +}
> +
>  int hci_conn_del(struct hci_conn *conn)
>  {
>         struct hci_dev *hdev =3D conn->hdev;
> @@ -1080,15 +1091,16 @@ int hci_conn_del(struct hci_conn *conn)
>         cancel_delayed_work_sync(&conn->idle_work);
>
>         if (conn->type =3D=3D ACL_LINK) {
> -               struct hci_conn *sco =3D conn->link;
> -               if (sco) {
> -                       sco->link =3D NULL;
> +               struct hci_conn *link =3D conn->link;
> +
> +               if (link) {
> +                       hci_conn_unlink(conn);
>                         /* Due to race, SCO connection might be not estab=
lished
>                          * yet at this point. Delete it now, otherwise it=
 is
>                          * possible for it to be stuck and can't be delet=
ed.
>                          */
> -                       if (sco->handle =3D=3D HCI_CONN_HANDLE_UNSET)
> -                               hci_conn_del(sco);
> +                       if (link->handle =3D=3D HCI_CONN_HANDLE_UNSET)
> +                               hci_conn_del(link);
>                 }
>
>                 /* Unacked frames */
> @@ -1104,7 +1116,7 @@ int hci_conn_del(struct hci_conn *conn)
>                 struct hci_conn *acl =3D conn->link;
>
>                 if (acl) {
> -                       acl->link =3D NULL;
> +                       hci_conn_unlink(conn);
>                         hci_conn_drop(acl);
>                 }
>
> @@ -2444,6 +2456,12 @@ void hci_conn_hash_flush(struct hci_dev *hdev)
>                 c->state =3D BT_CLOSED;
>
>                 hci_disconn_cfm(c, HCI_ERROR_LOCAL_HOST_TERM);
> +
> +               /* Unlink before deleting otherwise it is possible that
> +                * hci_conn_del removes the link which may cause the list=
 to
> +                * contain items already freed.
> +                */
> +               hci_conn_unlink(c);
>                 hci_conn_del(c);
>         }
>  }
> --
> 2.39.2

Can you give it a try if this doesn't break the existing problem you
were trying to fix, anyway it is quite easy to reproduce with the
likes of iso-tester and syzbot also seems to have bumped into it as
well.


--=20
Luiz Augusto von Dentz