Hi Marcel,
On Thu, Apr 21, 2022 at 8:57 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
