| Message ID | 20220421214607.3326277-1-luiz.dentz@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=634391 ---Test result--- Test Summary: CheckPatch PASS 5.65 seconds GitLint FAIL 0.99 seconds SubjectPrefix PASS 2.65 seconds BuildKernel PASS 31.29 seconds BuildKernel32 PASS 28.64 seconds Incremental Build with patchesPASS 72.57 seconds TestRunner: Setup PASS 476.19 seconds TestRunner: l2cap-tester PASS 17.44 seconds TestRunner: bnep-tester PASS 6.36 seconds TestRunner: mgmt-tester PASS 104.48 seconds TestRunner: rfcomm-tester PASS 9.90 seconds TestRunner: sco-tester PASS 9.69 seconds TestRunner: smp-tester PASS 9.82 seconds TestRunner: userchan-tester PASS 6.58 seconds Details ############################## Test: GitLint - FAIL - 0.99 seconds Run gitlint with rule in .gitlint [v2,1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status 13: B3 Line contains hard tab characters (\t): " Sound Products Inc)" 25: B3 Line contains hard tab characters (\t): " Sound Products Inc)" 27: B3 Line contains hard tab characters (\t): " gateway SDP record: Connection timed out" 32: B3 Line contains hard tab characters (\t): " Sound Products Inc)" 35: B3 Line contains hard tab characters (\t): " Sound Products Inc)" --- Regards, Linux Bluetooth
Hi Luiz, > Commit d5ebaa7c5f6f6 introduces checks for handle range > (e.g HCI_CONN_HANDLE_MAX) but controllers like Intel AX200 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 > > 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) > > Fixes: d5ebaa7c5f6f6 ("Bluetooth: hci_event: Ignore multiple conn complete events") > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > v2: Check if handle is valid just before assigning it to hci_conn object and > in case it is invalid reset the status to HCI_ERROR_INVALID_PARAMETERS(0x12) > so it can be passed to the likes of hci_connect_cfm and then is translated to > EINVAL by bt_to_errno. > v3: Rebase changes. > > include/net/bluetooth/hci.h | 1 + > net/bluetooth/hci_event.c | 45 ++++++++++++++++++++----------------- > 2 files changed, 26 insertions(+), 20 deletions(-) > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 8bb81ea4d286..62a9bb022aed 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -587,6 +587,7 @@ enum { > #define HCI_ERROR_CONNECTION_TIMEOUT 0x08 > #define HCI_ERROR_REJ_LIMITED_RESOURCES 0x0d > #define HCI_ERROR_REJ_BAD_ADDR 0x0f > +#define HCI_ERROR_INVALID_PARAMETERS 0x12 > #define HCI_ERROR_REMOTE_USER_TERM 0x13 > #define HCI_ERROR_REMOTE_LOW_RESOURCES 0x14 > #define HCI_ERROR_REMOTE_POWER_OFF 0x15 > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index abaabfae19cc..9feef7dbde3d 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -3068,11 +3068,6 @@ 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) { > - bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for invalid handle"); > - return; > - } > - > bt_dev_dbg(hdev, "status 0x%2.2x", ev->status); > > hci_dev_lock(hdev); > @@ -3124,6 +3119,12 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data, > > if (!ev->status) { > conn->handle = __le16_to_cpu(ev->handle); > + if (conn->handle > HCI_CONN_HANDLE_MAX) { > + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", > + conn->handle, HCI_CONN_HANDLE_MAX); > + ev->status = HCI_ERROR_INVALID_PARAMETERS; it is not a good idea to overwrite ev. We should have a separate status variable. Remember that we have ev = data and I think it is a mistake that it is not const void *data in the first place. > + goto done; > + } > > if (conn->type == ACL_LINK) { > conn->state = BT_CONFIG; > @@ -3164,17 +3165,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data, > hci_send_cmd(hdev, HCI_OP_CHANGE_CONN_PTYPE, sizeof(cp), > &cp); > } > - } else { > - conn->state = BT_CLOSED; > - if (conn->type == ACL_LINK) > - mgmt_connect_failed(hdev, &conn->dst, conn->type, > - conn->dst_type, ev->status); > } > > if (conn->type == ACL_LINK) > hci_sco_setup(conn, ev->status); > > +done: > if (ev->status) { > + conn->state = BT_CLOSED; > + if (conn->type == ACL_LINK) > + mgmt_connect_failed(hdev, &conn->dst, conn->type, > + conn->dst_type, ev->status); > hci_connect_cfm(conn, ev->status); > hci_conn_del(conn); > } else if (ev->link_type == SCO_LINK) { > @@ -4690,11 +4691,6 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data, > return; > } > > - if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) { > - bt_dev_err(hdev, "Ignoring HCI_Sync_Conn_Complete for invalid handle"); > - return; > - } > - > bt_dev_dbg(hdev, "status 0x%2.2x", ev->status); > > hci_dev_lock(hdev); > @@ -4732,6 +4728,14 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data, > switch (ev->status) { > case 0x00: > conn->handle = __le16_to_cpu(ev->handle); > + if (conn->handle > HCI_CONN_HANDLE_MAX) { > + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", > + conn->handle, HCI_CONN_HANDLE_MAX); > + ev->status = HCI_ERROR_INVALID_PARAMETERS; > + conn->state = BT_CLOSED; > + break; > + } > + > conn->state = BT_CONNECTED; > conn->type = ev->link_type; > > @@ -5527,11 +5531,6 @@ 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) { > - bt_dev_err(hdev, "Ignoring HCI_LE_Connection_Complete for invalid handle"); > - return; > - } > - > hci_dev_lock(hdev); > > /* All controllers implicitly stop advertising in the event of a > @@ -5603,6 +5602,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, > > conn->dst_type = ev_bdaddr_type(hdev, conn->dst_type, NULL); > > + if (handle > HCI_CONN_HANDLE_MAX) { > + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", handle, > + HCI_CONN_HANDLE_MAX); > + status = HCI_ERROR_INVALID_PARAMETERS; > + } > + > if (status) { > hci_le_conn_failed(conn, status); > goto unlock; Regards Marcel
Hi Marcel, On Fri, Apr 22, 2022 at 1:21 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Luiz, > > > Commit d5ebaa7c5f6f6 introduces checks for handle range > > (e.g HCI_CONN_HANDLE_MAX) but controllers like Intel AX200 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 > > > > 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) > > > > Fixes: d5ebaa7c5f6f6 ("Bluetooth: hci_event: Ignore multiple conn complete events") > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > v2: Check if handle is valid just before assigning it to hci_conn object and > > in case it is invalid reset the status to HCI_ERROR_INVALID_PARAMETERS(0x12) > > so it can be passed to the likes of hci_connect_cfm and then is translated to > > EINVAL by bt_to_errno. > > v3: Rebase changes. > > > > include/net/bluetooth/hci.h | 1 + > > net/bluetooth/hci_event.c | 45 ++++++++++++++++++++----------------- > > 2 files changed, 26 insertions(+), 20 deletions(-) > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > index 8bb81ea4d286..62a9bb022aed 100644 > > --- a/include/net/bluetooth/hci.h > > +++ b/include/net/bluetooth/hci.h > > @@ -587,6 +587,7 @@ enum { > > #define HCI_ERROR_CONNECTION_TIMEOUT 0x08 > > #define HCI_ERROR_REJ_LIMITED_RESOURCES 0x0d > > #define HCI_ERROR_REJ_BAD_ADDR 0x0f > > +#define HCI_ERROR_INVALID_PARAMETERS 0x12 > > #define HCI_ERROR_REMOTE_USER_TERM 0x13 > > #define HCI_ERROR_REMOTE_LOW_RESOURCES 0x14 > > #define HCI_ERROR_REMOTE_POWER_OFF 0x15 > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > > index abaabfae19cc..9feef7dbde3d 100644 > > --- a/net/bluetooth/hci_event.c > > +++ b/net/bluetooth/hci_event.c > > @@ -3068,11 +3068,6 @@ 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) { > > - bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for invalid handle"); > > - return; > > - } > > - > > bt_dev_dbg(hdev, "status 0x%2.2x", ev->status); > > > > hci_dev_lock(hdev); > > @@ -3124,6 +3119,12 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data, > > > > if (!ev->status) { > > conn->handle = __le16_to_cpu(ev->handle); > > + if (conn->handle > HCI_CONN_HANDLE_MAX) { > > + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", > > + conn->handle, HCI_CONN_HANDLE_MAX); > > + ev->status = HCI_ERROR_INVALID_PARAMETERS; > > it is not a good idea to overwrite ev. We should have a separate status variable. Remember that we have ev = data and I think it is a mistake that it is not const void *data in the first place. Ack, will send a v3 shortly. > > > + goto done; > > + } > > > > if (conn->type == ACL_LINK) { > > conn->state = BT_CONFIG; > > @@ -3164,17 +3165,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data, > > hci_send_cmd(hdev, HCI_OP_CHANGE_CONN_PTYPE, sizeof(cp), > > &cp); > > } > > - } else { > > - conn->state = BT_CLOSED; > > - if (conn->type == ACL_LINK) > > - mgmt_connect_failed(hdev, &conn->dst, conn->type, > > - conn->dst_type, ev->status); > > } > > > > if (conn->type == ACL_LINK) > > hci_sco_setup(conn, ev->status); > > > > +done: > > if (ev->status) { > > + conn->state = BT_CLOSED; > > + if (conn->type == ACL_LINK) > > + mgmt_connect_failed(hdev, &conn->dst, conn->type, > > + conn->dst_type, ev->status); > > hci_connect_cfm(conn, ev->status); > > hci_conn_del(conn); > > } else if (ev->link_type == SCO_LINK) { > > @@ -4690,11 +4691,6 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data, > > return; > > } > > > > - if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) { > > - bt_dev_err(hdev, "Ignoring HCI_Sync_Conn_Complete for invalid handle"); > > - return; > > - } > > - > > bt_dev_dbg(hdev, "status 0x%2.2x", ev->status); > > > > hci_dev_lock(hdev); > > @@ -4732,6 +4728,14 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data, > > switch (ev->status) { > > case 0x00: > > conn->handle = __le16_to_cpu(ev->handle); > > + if (conn->handle > HCI_CONN_HANDLE_MAX) { > > + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", > > + conn->handle, HCI_CONN_HANDLE_MAX); > > + ev->status = HCI_ERROR_INVALID_PARAMETERS; > > + conn->state = BT_CLOSED; > > + break; > > + } > > + > > conn->state = BT_CONNECTED; > > conn->type = ev->link_type; > > > > @@ -5527,11 +5531,6 @@ 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) { > > - bt_dev_err(hdev, "Ignoring HCI_LE_Connection_Complete for invalid handle"); > > - return; > > - } > > - > > hci_dev_lock(hdev); > > > > /* All controllers implicitly stop advertising in the event of a > > @@ -5603,6 +5602,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, > > > > conn->dst_type = ev_bdaddr_type(hdev, conn->dst_type, NULL); > > > > + if (handle > HCI_CONN_HANDLE_MAX) { > > + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", handle, > > + HCI_CONN_HANDLE_MAX); > > + status = HCI_ERROR_INVALID_PARAMETERS; > > + } > > + > > if (status) { > > hci_le_conn_failed(conn, status); > > goto unlock; > > Regards > > Marcel >
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index 8bb81ea4d286..62a9bb022aed 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -587,6 +587,7 @@ enum { #define HCI_ERROR_CONNECTION_TIMEOUT 0x08 #define HCI_ERROR_REJ_LIMITED_RESOURCES 0x0d #define HCI_ERROR_REJ_BAD_ADDR 0x0f +#define HCI_ERROR_INVALID_PARAMETERS 0x12 #define HCI_ERROR_REMOTE_USER_TERM 0x13 #define HCI_ERROR_REMOTE_LOW_RESOURCES 0x14 #define HCI_ERROR_REMOTE_POWER_OFF 0x15 diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index abaabfae19cc..9feef7dbde3d 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -3068,11 +3068,6 @@ 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) { - bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for invalid handle"); - return; - } - bt_dev_dbg(hdev, "status 0x%2.2x", ev->status); hci_dev_lock(hdev); @@ -3124,6 +3119,12 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data, if (!ev->status) { conn->handle = __le16_to_cpu(ev->handle); + if (conn->handle > HCI_CONN_HANDLE_MAX) { + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", + conn->handle, HCI_CONN_HANDLE_MAX); + ev->status = HCI_ERROR_INVALID_PARAMETERS; + goto done; + } if (conn->type == ACL_LINK) { conn->state = BT_CONFIG; @@ -3164,17 +3165,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data, hci_send_cmd(hdev, HCI_OP_CHANGE_CONN_PTYPE, sizeof(cp), &cp); } - } else { - conn->state = BT_CLOSED; - if (conn->type == ACL_LINK) - mgmt_connect_failed(hdev, &conn->dst, conn->type, - conn->dst_type, ev->status); } if (conn->type == ACL_LINK) hci_sco_setup(conn, ev->status); +done: if (ev->status) { + conn->state = BT_CLOSED; + if (conn->type == ACL_LINK) + mgmt_connect_failed(hdev, &conn->dst, conn->type, + conn->dst_type, ev->status); hci_connect_cfm(conn, ev->status); hci_conn_del(conn); } else if (ev->link_type == SCO_LINK) { @@ -4690,11 +4691,6 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data, return; } - if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) { - bt_dev_err(hdev, "Ignoring HCI_Sync_Conn_Complete for invalid handle"); - return; - } - bt_dev_dbg(hdev, "status 0x%2.2x", ev->status); hci_dev_lock(hdev); @@ -4732,6 +4728,14 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data, switch (ev->status) { case 0x00: conn->handle = __le16_to_cpu(ev->handle); + if (conn->handle > HCI_CONN_HANDLE_MAX) { + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", + conn->handle, HCI_CONN_HANDLE_MAX); + ev->status = HCI_ERROR_INVALID_PARAMETERS; + conn->state = BT_CLOSED; + break; + } + conn->state = BT_CONNECTED; conn->type = ev->link_type; @@ -5527,11 +5531,6 @@ 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) { - bt_dev_err(hdev, "Ignoring HCI_LE_Connection_Complete for invalid handle"); - return; - } - hci_dev_lock(hdev); /* All controllers implicitly stop advertising in the event of a @@ -5603,6 +5602,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, conn->dst_type = ev_bdaddr_type(hdev, conn->dst_type, NULL); + if (handle > HCI_CONN_HANDLE_MAX) { + bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x", handle, + HCI_CONN_HANDLE_MAX); + status = HCI_ERROR_INVALID_PARAMETERS; + } + if (status) { hci_le_conn_failed(conn, status); goto unlock;