Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
References: <53130b4a5fb21a15f2674c336282d25ef5d2232e.1696077070.git.pav@iki.fi>
 <fd9c0f0eddb8a7d73e9797568dc3cf2bc9b8ad62.1696077070.git.pav@iki.fi>
In-Reply-To: <fd9c0f0eddb8a7d73e9797568dc3cf2bc9b8ad62.1696077070.git.pav@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 2 Oct 2023 16:22:25 -0700
Message-ID: <CABBYNZKcTxZ916gdjCSWe6j+9JJ=Jz-Yazfm5bu-vzNss8U9oQ@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] Bluetooth: hci_conn: verify connection is to be
 aborted before doing it
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org, syzkaller-bugs@googlegroups.com,
        syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Pauli,

On Sat, Sep 30, 2023 at 3:25=E2=80=AFPM Pauli Virtanen <pav@iki.fi> wrote:
>
> There is a race condition where a connection handle is reused, after
> hci_abort_conn but before abort_conn_sync is processed in hci_sync. In
> this case, hci_abort_conn_sync ends up working on the wrong connection,
> which can have abort_reason=3D=3D0, in which case hci_connect_cfm is call=
ed
> with success status and then connection is deleted, which causes various
> use-after-free.
>
> Fix by checking abort_reason is nonzero before calling
> hci_abort_conn_sync. If it's zero, then the connection is probably a
> different one than we expected and should not be aborted.
>
> Also fix some theoretical UAF / races, where something frees the conn
> while hci_abort_conn_sync is working on it.
>
> Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting =
connections")
> Reported-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-bluetooth/0000000000005ab9840603715=
83e@google.com/
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>
> Notes:
>     v2: drop one probably not necessary WARN_ON
>
>     Not sure how you'd hit this condition in real controller, but syzbot
>     does end up calling hci_abort_conn_sync with reason =3D=3D 0 which th=
en
>     causes havoc.
>
>     This can be verified: with a patch that changes abort_conn_sync to
>
>         2874    conn =3D hci_conn_hash_lookup_handle(hdev, handle);
>         2875    if (!conn || WARN_ON(!conn->abort_reason))
>         2876            return 0;
>
>     https://syzkaller.appspot.com/text?tag=3DPatch&x=3D16eff740680000
>
>     it hits that WARN_ON:
>
>     https://syzkaller.appspot.com/x/log.txt?x=3D10affb97a80000
>
> #syz test git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetoo=
th-next.git master
>
>  net/bluetooth/hci_conn.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index e62a5f368a51..22de82071e35 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -2901,12 +2901,25 @@ static int abort_conn_sync(struct hci_dev *hdev, =
void *data)
>  {
>         struct hci_conn *conn;
>         u16 handle =3D PTR_UINT(data);
> +       u8 reason;
> +       int err;
> +
> +       rcu_read_lock();
>
>         conn =3D hci_conn_hash_lookup_handle(hdev, handle);
> +       if (conn) {
> +               reason =3D READ_ONCE(conn->abort_reason);
> +               conn =3D reason ? hci_conn_get(conn) : NULL;
> +       }
> +
> +       rcu_read_unlock();
> +
>         if (!conn)
>                 return 0;
>
> -       return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
> +       err =3D hci_abort_conn_sync(hdev, conn, reason);
> +       hci_conn_put(conn);
> +       return err;
>  }
>
>  int hci_abort_conn(struct hci_conn *conn, u8 reason)
> @@ -2918,6 +2931,8 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason=
)
>          */
>         if (conn->abort_reason)
>                 return 0;
> +       if (WARN_ON(!reason))
> +               reason =3D HCI_ERROR_UNSPECIFIED;
>
>         bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, r=
eason);
>
> --
> 2.41.0

Don't really like where this is going, the culprit here seems that we
are not able to cancel the callback when freeing the hci_conn so it
stays enqueued while the new connection is created with the same
handle, so I think we need to introduce a function that cancel queued
like Ive sent sometime back, that way we don't need to keep trying to
check if it is the same connection or not.


--=20
Luiz Augusto von Dentz