* [PATCH v2 1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status
@ 2022-04-21 21:46 Luiz Augusto von Dentz
2022-04-21 21:46 ` [PATCH v2 2/3] Bluetooth: hci_event: Fix creating hci_conn object " Luiz Augusto von Dentz
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2022-04-21 21:46 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
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;
+ 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;
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/3] Bluetooth: hci_event: Fix creating hci_conn object on error status
2022-04-21 21:46 [PATCH v2 1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
@ 2022-04-21 21:46 ` Luiz Augusto von Dentz
2022-04-21 21:46 ` [PATCH v2 3/3] Bluetooth: hci_sync: Cleanup hci_conn if it cannot be aborted Luiz Augusto von Dentz
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2022-04-21 21:46 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
It is useless to create a hci_conn object if on error status as the
result would be it being freed in the process and anyway it is likely
the result of controller and host stack being out of sync.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
net/bluetooth/hci_event.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 9feef7dbde3d..3002df41f16b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3074,6 +3074,12 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
if (!conn) {
+ /* In case of error status and there is no connection pending
+ * just unlock as there is nothing to cleanup.
+ */
+ if (ev->status)
+ goto unlock;
+
/* Connection may not exist if auto-connected. Check the bredr
* allowlist to see if this device is allowed to auto connect.
* If link is an ACL type, create a connection class
@@ -5540,6 +5546,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
conn = hci_lookup_le_connect(hdev);
if (!conn) {
+ /* In case of error status and there is no connection pending
+ * just unlock as there is nothing to cleanup.
+ */
+ if (status)
+ goto unlock;
+
conn = hci_conn_add(hdev, LE_LINK, bdaddr, role);
if (!conn) {
bt_dev_err(hdev, "no memory for new connection");
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] Bluetooth: hci_sync: Cleanup hci_conn if it cannot be aborted
2022-04-21 21:46 [PATCH v2 1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
2022-04-21 21:46 ` [PATCH v2 2/3] Bluetooth: hci_event: Fix creating hci_conn object " Luiz Augusto von Dentz
@ 2022-04-21 21:46 ` Luiz Augusto von Dentz
2022-04-21 23:01 ` [v2,1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status bluez.test.bot
2022-04-22 8:21 ` [PATCH v2 1/3] " Marcel Holtmann
3 siblings, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2022-04-21 21:46 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This attempts to cleanup the hci_conn if it cannot be aborted as
otherwise it would likely result in having the controller and host
stack out of sync with respect to connection handle.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_conn.c | 32 ++++++++++++++++++++++++--------
net/bluetooth/hci_event.c | 13 ++++---------
net/bluetooth/hci_sync.c | 11 ++++++++++-
4 files changed, 39 insertions(+), 19 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 59815df1272a..64d3a63759a8 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1156,7 +1156,7 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
-void hci_le_conn_failed(struct hci_conn *conn, u8 status);
+void hci_conn_failed(struct hci_conn *conn, u8 status);
/*
* hci_conn_get() and hci_conn_put() are used to control the life-time of an
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index cd51bf2a709b..882a7df13005 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -670,7 +670,7 @@ static void le_conn_timeout(struct work_struct *work)
/* Disable LE Advertising */
le_disable_advertising(hdev);
hci_dev_lock(hdev);
- hci_le_conn_failed(conn, HCI_ERROR_ADVERTISING_TIMEOUT);
+ hci_conn_failed(conn, HCI_ERROR_ADVERTISING_TIMEOUT);
hci_dev_unlock(hdev);
return;
}
@@ -873,7 +873,7 @@ struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
EXPORT_SYMBOL(hci_get_route);
/* This function requires the caller holds hdev->lock */
-void hci_le_conn_failed(struct hci_conn *conn, u8 status)
+static void hci_le_conn_failed(struct hci_conn *conn, u8 status)
{
struct hci_dev *hdev = conn->hdev;
struct hci_conn_params *params;
@@ -886,8 +886,6 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status)
params->conn = NULL;
}
- conn->state = BT_CLOSED;
-
/* If the status indicates successful cancellation of
* the attempt (i.e. Unknown Connection Id) there's no point of
* notifying failure since we'll go back to keep trying to
@@ -899,10 +897,6 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status)
mgmt_connect_failed(hdev, &conn->dst, conn->type,
conn->dst_type, status);
- hci_connect_cfm(conn, status);
-
- hci_conn_del(conn);
-
/* Since we may have temporarily stopped the background scanning in
* favor of connection establishment, we should restart it.
*/
@@ -914,6 +908,28 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status)
hci_enable_advertising(hdev);
}
+/* This function requires the caller holds hdev->lock */
+void hci_conn_failed(struct hci_conn *conn, u8 status)
+{
+ struct hci_dev *hdev = conn->hdev;
+
+ bt_dev_dbg(hdev, "status 0x%2.2x", status);
+
+ switch (conn->type) {
+ case LE_LINK:
+ hci_le_conn_failed(conn, status);
+ break;
+ case ACL_LINK:
+ mgmt_connect_failed(hdev, &conn->dst, conn->type,
+ conn->dst_type, status);
+ break;
+ }
+
+ conn->state = BT_CLOSED;
+ hci_connect_cfm(conn, status);
+ hci_conn_del(conn);
+}
+
static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
{
struct hci_conn *conn = data;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 3002df41f16b..ad460f1da4ed 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2834,7 +2834,7 @@ static void hci_cs_le_create_conn(struct hci_dev *hdev, u8 status)
bt_dev_dbg(hdev, "status 0x%2.2x", status);
/* All connection failure handling is taken care of by the
- * hci_le_conn_failed function which is triggered by the HCI
+ * hci_conn_failed function which is triggered by the HCI
* request completion callbacks used for connecting.
*/
if (status)
@@ -2859,7 +2859,7 @@ static void hci_cs_le_ext_create_conn(struct hci_dev *hdev, u8 status)
bt_dev_dbg(hdev, "status 0x%2.2x", status);
/* All connection failure handling is taken care of by the
- * hci_le_conn_failed function which is triggered by the HCI
+ * hci_conn_failed function which is triggered by the HCI
* request completion callbacks used for connecting.
*/
if (status)
@@ -3178,12 +3178,7 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
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);
+ hci_conn_failed(conn, ev->status);
} else if (ev->link_type == SCO_LINK) {
switch (conn->setting & SCO_AIRMODE_MASK) {
case SCO_AIRMODE_CVSD:
@@ -5621,7 +5616,7 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
}
if (status) {
- hci_le_conn_failed(conn, status);
+ hci_conn_failed(conn, status);
goto unlock;
}
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 2d3b9adbd215..fa95eb2dcffa 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -4466,12 +4466,21 @@ static int hci_reject_conn_sync(struct hci_dev *hdev, struct hci_conn *conn,
static int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn,
u8 reason)
{
+ int err;
+
switch (conn->state) {
case BT_CONNECTED:
case BT_CONFIG:
return hci_disconnect_sync(hdev, conn, reason);
case BT_CONNECT:
- return hci_connect_cancel_sync(hdev, conn);
+ err = hci_connect_cancel_sync(hdev, conn);
+ /* Cleanup hci_conn object if it cannot be cancelled as it
+ * likelly means the controller and host stack are out of sync.
+ */
+ if (err)
+ hci_conn_failed(conn, err);
+
+ return err;
case BT_CONNECT2:
return hci_reject_conn_sync(hdev, conn, reason);
default:
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [v2,1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status
2022-04-21 21:46 [PATCH v2 1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
2022-04-21 21:46 ` [PATCH v2 2/3] Bluetooth: hci_event: Fix creating hci_conn object " Luiz Augusto von Dentz
2022-04-21 21:46 ` [PATCH v2 3/3] Bluetooth: hci_sync: Cleanup hci_conn if it cannot be aborted Luiz Augusto von Dentz
@ 2022-04-21 23:01 ` bluez.test.bot
2022-04-22 8:21 ` [PATCH v2 1/3] " Marcel Holtmann
3 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2022-04-21 23:01 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status
2022-04-21 21:46 [PATCH v2 1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
` (2 preceding siblings ...)
2022-04-21 23:01 ` [v2,1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status bluez.test.bot
@ 2022-04-22 8:21 ` Marcel Holtmann
2022-04-22 19:45 ` Luiz Augusto von Dentz
3 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2022-04-22 8:21 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status
2022-04-22 8:21 ` [PATCH v2 1/3] " Marcel Holtmann
@ 2022-04-22 19:45 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2022-04-22 19:45 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
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
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-22 22:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-21 21:46 [PATCH v2 1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
2022-04-21 21:46 ` [PATCH v2 2/3] Bluetooth: hci_event: Fix creating hci_conn object " Luiz Augusto von Dentz
2022-04-21 21:46 ` [PATCH v2 3/3] Bluetooth: hci_sync: Cleanup hci_conn if it cannot be aborted Luiz Augusto von Dentz
2022-04-21 23:01 ` [v2,1/3] Bluetooth: hci_event: Fix checking for invalid handle on error status bluez.test.bot
2022-04-22 8:21 ` [PATCH v2 1/3] " Marcel Holtmann
2022-04-22 19:45 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).