Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220420221433.2933868-1-luiz.dentz@gmail.com> <654FF692-95EB-459A-9144-62EA911C7BBB@holtmann.org>
In-Reply-To: <654FF692-95EB-459A-9144-62EA911C7BBB@holtmann.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 21 Apr 2022 13:52:40 -0700
Message-ID: <CABBYNZKZB3Xv0z+KdV2nBmV0YXK_e0anSD0oxpNKJ68C1tWQYg@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on
 error status
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Marcel,

On Thu, Apr 21, 2022 at 8:57 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > Commit d5ebaa7c5f6f6 introduces checks for handle range
> > (e.g HCI_CONN_HANDLE_MAX) but controllers don't seem to respect the
> > valid range int case of error status:
> >
> >> HCI Event: Connect Complete (0x03) plen 11
> >        Status: Page Timeout (0x04)
> >        Handle: 65535
> >        Address: 94:DB:56:XX:XX:XX (Sony Home Entertainment&
> >       Sound Products Inc)
> >        Link type: ACL (0x01)
> >        Encryption: Disabled (0x00)
> > [1644965.827560] Bluetooth: hci0: Ignoring HCI_Connection_Complete for
> > invalid handle
>
> so the problem is that with BR/EDR the lookup is by BD_ADDR. I think the check for valid handle is wrong at the beginning of connect complete handler.
>
> The problem is really the fact the we trying a big hammer at the beginning. The hci_conn_add in case of auto-connect should validate status and handle. We are not even validating the status right now assuming we always get a status == 0 on auto-connect.

Ive sent a separate patch addressing the use of hci_conn_add on error
status, in some places we did it properly but others didn't so it
would result in hci_conn object being created just to be freed later
at the same function.

> The second handle validation should only occur if we have !status in the bottom half of that function.

Send a v2 checking the handle just before assigning it to hci_conn object.

>
> > Because of it is impossible to cleanup the connections properly since
> > the stack would attempt to cancel the connection which is no longer in
> > progress causing the following trace:
> >
> > < HCI Command: Create Connection Cancel (0x01|0x0008) plen 6
> >        Address: 94:DB:56:XX:XX:XX (Sony Home Entertainment&
> >       Sound Products Inc)
> > = bluetoothd: src/profile.c:record_cb() Unable to get Hands-Free Voice
> >       gateway SDP record: Connection timed out
> >> HCI Event: Command Complete (0x0e) plen 10
> >      Create Connection Cancel (0x01|0x0008) ncmd 1
> >        Status: Unknown Connection Identifier (0x02)
> >        Address: 94:DB:56:XX:XX:XX (Sony Home Entertainment&
> >       Sound Products Inc)
> > < HCI Command: Create Connection Cancel (0x01|0x0008) plen 6
> >        Address: 94:DB:56:XX:XX:XX (Sony Home Entertainment&
> >       Sound Products Inc)
>
> Can we get details about which controller uses 0xffff instead of 0 for the handle?
>
> >
> > Fixes: d5ebaa7c5f6f6 ("Bluetooth: hci_event: Ignore multiple conn complete events")
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > net/bluetooth/hci_event.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index abaabfae19cc..1cc5a712459e 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3068,7 +3068,7 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> >       struct hci_ev_conn_complete *ev = data;
> >       struct hci_conn *conn;
> >
> > -     if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
> > +     if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
> >               bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for invalid handle");
> >               return;
> >       }
>
> See comments above.
>
> > @@ -4690,7 +4690,7 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data,
> >               return;
> >       }
> >
> > -     if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
> > +     if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
> >               bt_dev_err(hdev, "Ignoring HCI_Sync_Conn_Complete for invalid handle");
> >               return;
> >       }
>
> This is also in the wrong position. Fundamentally we need to check the validity of the handle before we assign it and not just globally.
>
> > @@ -5527,7 +5527,7 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
> >       struct smp_irk *irk;
> >       u8 addr_type;
> >
> > -     if (handle > HCI_CONN_HANDLE_MAX) {
> > +     if (!status && handle > HCI_CONN_HANDLE_MAX) {
> >               bt_dev_err(hdev, "Ignoring HCI_LE_Connection_Complete for invalid handle");
> >               return;
> >       }
>
> Same here. The global check is pointless. Check before using it.
>
> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz