Hi Pauli,
On Sat, Sep 30, 2023 at 3:25 PM Pauli Virtanen <pav@xxxxxx> 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==0, in which case hci_connect_cfm is called
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/linux-bluetooth/0000000000005ab984060371583e@xxxxxxxxxx/
> Signed-off-by: Pauli Virtanen <pav@xxxxxx>
> ---
>
> 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 == 0 which then
> causes havoc.
>
> This can be verified: with a patch that changes abort_conn_sync to
>
> 2874 conn = hci_conn_hash_lookup_handle(hdev, handle);
> 2875 if (!conn || WARN_ON(!conn->abort_reason))
> 2876 return 0;
>
> https://syzkaller.appspot.com/text?tag=Patch&x=16eff740680000
>
> it hits that WARN_ON:
>
> https://syzkaller.appspot.com/x/log.txt?x=10affb97a80000
>
> #syz test git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-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 = PTR_UINT(data);
> + u8 reason;
> + int err;
> +
> + rcu_read_lock();
>
> conn = hci_conn_hash_lookup_handle(hdev, handle);
> + if (conn) {
> + reason = READ_ONCE(conn->abort_reason);
> + conn = reason ? hci_conn_get(conn) : NULL;
> + }
> +
> + rcu_read_unlock();
> +
> if (!conn)
> return 0;
>
> - return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
> + err = 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 = HCI_ERROR_UNSPECIFIED;
>
> bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
>
> --
> 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.
--
Luiz Augusto von Dentz
