* [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status
@ 2022-04-20 22:14 Luiz Augusto von Dentz
2022-04-21 6:43 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2022-04-20 22:14 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 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>
---
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;
}
@@ -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;
}
@@ -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;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status
2022-04-20 22:14 [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
@ 2022-04-21 6:43 ` kernel test robot
2022-04-21 6:43 ` kernel test robot
2022-04-21 15:57 ` Marcel Holtmann
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-04-21 6:43 UTC (permalink / raw)
To: Luiz Augusto von Dentz, linux-bluetooth; +Cc: kbuild-all
Hi Luiz,
I love your patch! Yet something to improve:
[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on linus/master v5.18-rc3 next-20220420]
[cannot apply to bluetooth/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Luiz-Augusto-von-Dentz/Bluetooth-hci_event-Fix-checking-for-invalid-handle-on-error-status/20220421-061600
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: arc-randconfig-r043-20220420 (https://download.01.org/0day-ci/archive/20220421/202204210853.eFHdXHTU-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/91a252b91692543d5f9536ebdf10f20a413a858f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Luiz-Augusto-von-Dentz/Bluetooth-hci_event-Fix-checking-for-invalid-handle-on-error-status/20220421-061600
git checkout 91a252b91692543d5f9536ebdf10f20a413a858f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash net/bluetooth/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
net/bluetooth/hci_event.c: In function 'hci_conn_complete_evt':
>> net/bluetooth/hci_event.c:3071:14: error: 'status' undeclared (first use in this function); did you mean 'kstatfs'?
3071 | if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
| ^~~~~~
| kstatfs
net/bluetooth/hci_event.c:3071:14: note: each undeclared identifier is reported only once for each function it appears in
net/bluetooth/hci_event.c: In function 'hci_sync_conn_complete_evt':
net/bluetooth/hci_event.c:4693:14: error: 'status' undeclared (first use in this function); did you mean 'kstatfs'?
4693 | if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
| ^~~~~~
| kstatfs
vim +3071 net/bluetooth/hci_event.c
3064
3065 static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
3066 struct sk_buff *skb)
3067 {
3068 struct hci_ev_conn_complete *ev = data;
3069 struct hci_conn *conn;
3070
> 3071 if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
3072 bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for invalid handle");
3073 return;
3074 }
3075
3076 bt_dev_dbg(hdev, "status 0x%2.2x", ev->status);
3077
3078 hci_dev_lock(hdev);
3079
3080 conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
3081 if (!conn) {
3082 /* Connection may not exist if auto-connected. Check the bredr
3083 * allowlist to see if this device is allowed to auto connect.
3084 * If link is an ACL type, create a connection class
3085 * automatically.
3086 *
3087 * Auto-connect will only occur if the event filter is
3088 * programmed with a given address. Right now, event filter is
3089 * only used during suspend.
3090 */
3091 if (ev->link_type == ACL_LINK &&
3092 hci_bdaddr_list_lookup_with_flags(&hdev->accept_list,
3093 &ev->bdaddr,
3094 BDADDR_BREDR)) {
3095 conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr,
3096 HCI_ROLE_SLAVE);
3097 if (!conn) {
3098 bt_dev_err(hdev, "no memory for new conn");
3099 goto unlock;
3100 }
3101 } else {
3102 if (ev->link_type != SCO_LINK)
3103 goto unlock;
3104
3105 conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK,
3106 &ev->bdaddr);
3107 if (!conn)
3108 goto unlock;
3109
3110 conn->type = SCO_LINK;
3111 }
3112 }
3113
3114 /* The HCI_Connection_Complete event is only sent once per connection.
3115 * Processing it more than once per connection can corrupt kernel memory.
3116 *
3117 * As the connection handle is set here for the first time, it indicates
3118 * whether the connection is already set up.
3119 */
3120 if (conn->handle != HCI_CONN_HANDLE_UNSET) {
3121 bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
3122 goto unlock;
3123 }
3124
3125 if (!ev->status) {
3126 conn->handle = __le16_to_cpu(ev->handle);
3127
3128 if (conn->type == ACL_LINK) {
3129 conn->state = BT_CONFIG;
3130 hci_conn_hold(conn);
3131
3132 if (!conn->out && !hci_conn_ssp_enabled(conn) &&
3133 !hci_find_link_key(hdev, &ev->bdaddr))
3134 conn->disc_timeout = HCI_PAIRING_TIMEOUT;
3135 else
3136 conn->disc_timeout = HCI_DISCONN_TIMEOUT;
3137 } else
3138 conn->state = BT_CONNECTED;
3139
3140 hci_debugfs_create_conn(conn);
3141 hci_conn_add_sysfs(conn);
3142
3143 if (test_bit(HCI_AUTH, &hdev->flags))
3144 set_bit(HCI_CONN_AUTH, &conn->flags);
3145
3146 if (test_bit(HCI_ENCRYPT, &hdev->flags))
3147 set_bit(HCI_CONN_ENCRYPT, &conn->flags);
3148
3149 /* Get remote features */
3150 if (conn->type == ACL_LINK) {
3151 struct hci_cp_read_remote_features cp;
3152 cp.handle = ev->handle;
3153 hci_send_cmd(hdev, HCI_OP_READ_REMOTE_FEATURES,
3154 sizeof(cp), &cp);
3155
3156 hci_req_update_scan(hdev);
3157 }
3158
3159 /* Set packet type for incoming connection */
3160 if (!conn->out && hdev->hci_ver < BLUETOOTH_VER_2_0) {
3161 struct hci_cp_change_conn_ptype cp;
3162 cp.handle = ev->handle;
3163 cp.pkt_type = cpu_to_le16(conn->pkt_type);
3164 hci_send_cmd(hdev, HCI_OP_CHANGE_CONN_PTYPE, sizeof(cp),
3165 &cp);
3166 }
3167 } else {
3168 conn->state = BT_CLOSED;
3169 if (conn->type == ACL_LINK)
3170 mgmt_connect_failed(hdev, &conn->dst, conn->type,
3171 conn->dst_type, ev->status);
3172 }
3173
3174 if (conn->type == ACL_LINK)
3175 hci_sco_setup(conn, ev->status);
3176
3177 if (ev->status) {
3178 hci_connect_cfm(conn, ev->status);
3179 hci_conn_del(conn);
3180 } else if (ev->link_type == SCO_LINK) {
3181 switch (conn->setting & SCO_AIRMODE_MASK) {
3182 case SCO_AIRMODE_CVSD:
3183 if (hdev->notify)
3184 hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
3185 break;
3186 }
3187
3188 hci_connect_cfm(conn, ev->status);
3189 }
3190
3191 unlock:
3192 hci_dev_unlock(hdev);
3193
3194 hci_conn_check_pending(hdev);
3195 }
3196
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status
2022-04-20 22:14 [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
2022-04-21 6:43 ` kernel test robot
@ 2022-04-21 6:43 ` kernel test robot
2022-04-21 15:57 ` Marcel Holtmann
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-04-21 6:43 UTC (permalink / raw)
To: Luiz Augusto von Dentz, linux-bluetooth; +Cc: llvm, kbuild-all
Hi Luiz,
I love your patch! Yet something to improve:
[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on linus/master v5.18-rc3 next-20220420]
[cannot apply to bluetooth/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Luiz-Augusto-von-Dentz/Bluetooth-hci_event-Fix-checking-for-invalid-handle-on-error-status/20220421-061600
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: riscv-randconfig-r042-20220420 (https://download.01.org/0day-ci/archive/20220421/202204210838.G9CZnn9u-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project bac6cd5bf85669e3376610cfc4c4f9ca015e7b9b)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/91a252b91692543d5f9536ebdf10f20a413a858f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Luiz-Augusto-von-Dentz/Bluetooth-hci_event-Fix-checking-for-invalid-handle-on-error-status/20220421-061600
git checkout 91a252b91692543d5f9536ebdf10f20a413a858f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash net/bluetooth/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> net/bluetooth/hci_event.c:3071:7: error: use of undeclared identifier 'status'
if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
^
net/bluetooth/hci_event.c:4693:7: error: use of undeclared identifier 'status'
if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
^
2 errors generated.
vim +/status +3071 net/bluetooth/hci_event.c
3064
3065 static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
3066 struct sk_buff *skb)
3067 {
3068 struct hci_ev_conn_complete *ev = data;
3069 struct hci_conn *conn;
3070
> 3071 if (!status && __le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
3072 bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for invalid handle");
3073 return;
3074 }
3075
3076 bt_dev_dbg(hdev, "status 0x%2.2x", ev->status);
3077
3078 hci_dev_lock(hdev);
3079
3080 conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
3081 if (!conn) {
3082 /* Connection may not exist if auto-connected. Check the bredr
3083 * allowlist to see if this device is allowed to auto connect.
3084 * If link is an ACL type, create a connection class
3085 * automatically.
3086 *
3087 * Auto-connect will only occur if the event filter is
3088 * programmed with a given address. Right now, event filter is
3089 * only used during suspend.
3090 */
3091 if (ev->link_type == ACL_LINK &&
3092 hci_bdaddr_list_lookup_with_flags(&hdev->accept_list,
3093 &ev->bdaddr,
3094 BDADDR_BREDR)) {
3095 conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr,
3096 HCI_ROLE_SLAVE);
3097 if (!conn) {
3098 bt_dev_err(hdev, "no memory for new conn");
3099 goto unlock;
3100 }
3101 } else {
3102 if (ev->link_type != SCO_LINK)
3103 goto unlock;
3104
3105 conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK,
3106 &ev->bdaddr);
3107 if (!conn)
3108 goto unlock;
3109
3110 conn->type = SCO_LINK;
3111 }
3112 }
3113
3114 /* The HCI_Connection_Complete event is only sent once per connection.
3115 * Processing it more than once per connection can corrupt kernel memory.
3116 *
3117 * As the connection handle is set here for the first time, it indicates
3118 * whether the connection is already set up.
3119 */
3120 if (conn->handle != HCI_CONN_HANDLE_UNSET) {
3121 bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
3122 goto unlock;
3123 }
3124
3125 if (!ev->status) {
3126 conn->handle = __le16_to_cpu(ev->handle);
3127
3128 if (conn->type == ACL_LINK) {
3129 conn->state = BT_CONFIG;
3130 hci_conn_hold(conn);
3131
3132 if (!conn->out && !hci_conn_ssp_enabled(conn) &&
3133 !hci_find_link_key(hdev, &ev->bdaddr))
3134 conn->disc_timeout = HCI_PAIRING_TIMEOUT;
3135 else
3136 conn->disc_timeout = HCI_DISCONN_TIMEOUT;
3137 } else
3138 conn->state = BT_CONNECTED;
3139
3140 hci_debugfs_create_conn(conn);
3141 hci_conn_add_sysfs(conn);
3142
3143 if (test_bit(HCI_AUTH, &hdev->flags))
3144 set_bit(HCI_CONN_AUTH, &conn->flags);
3145
3146 if (test_bit(HCI_ENCRYPT, &hdev->flags))
3147 set_bit(HCI_CONN_ENCRYPT, &conn->flags);
3148
3149 /* Get remote features */
3150 if (conn->type == ACL_LINK) {
3151 struct hci_cp_read_remote_features cp;
3152 cp.handle = ev->handle;
3153 hci_send_cmd(hdev, HCI_OP_READ_REMOTE_FEATURES,
3154 sizeof(cp), &cp);
3155
3156 hci_req_update_scan(hdev);
3157 }
3158
3159 /* Set packet type for incoming connection */
3160 if (!conn->out && hdev->hci_ver < BLUETOOTH_VER_2_0) {
3161 struct hci_cp_change_conn_ptype cp;
3162 cp.handle = ev->handle;
3163 cp.pkt_type = cpu_to_le16(conn->pkt_type);
3164 hci_send_cmd(hdev, HCI_OP_CHANGE_CONN_PTYPE, sizeof(cp),
3165 &cp);
3166 }
3167 } else {
3168 conn->state = BT_CLOSED;
3169 if (conn->type == ACL_LINK)
3170 mgmt_connect_failed(hdev, &conn->dst, conn->type,
3171 conn->dst_type, ev->status);
3172 }
3173
3174 if (conn->type == ACL_LINK)
3175 hci_sco_setup(conn, ev->status);
3176
3177 if (ev->status) {
3178 hci_connect_cfm(conn, ev->status);
3179 hci_conn_del(conn);
3180 } else if (ev->link_type == SCO_LINK) {
3181 switch (conn->setting & SCO_AIRMODE_MASK) {
3182 case SCO_AIRMODE_CVSD:
3183 if (hdev->notify)
3184 hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
3185 break;
3186 }
3187
3188 hci_connect_cfm(conn, ev->status);
3189 }
3190
3191 unlock:
3192 hci_dev_unlock(hdev);
3193
3194 hci_conn_check_pending(hdev);
3195 }
3196
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status
2022-04-20 22:14 [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
2022-04-21 6:43 ` kernel test robot
2022-04-21 6:43 ` kernel test robot
@ 2022-04-21 15:57 ` Marcel Holtmann
2022-04-21 20:52 ` Luiz Augusto von Dentz
2 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2022-04-21 15:57 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 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.
The second handle validation should only occur if we have !status in the bottom half of that function.
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status
2022-04-21 15:57 ` Marcel Holtmann
@ 2022-04-21 20:52 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2022-04-21 20:52 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status
@ 2022-04-21 20:47 Luiz Augusto von Dentz
0 siblings, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2022-04-21 20:47 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 in 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
When the above happens it prevents to cleanup the connections 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.
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_event.c | 44 ++++++++++++++++++++-----------------
2 files changed, 25 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..a658aa4c7306 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",
+ conn->handle);
+ 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",
+ conn->handle);
+ 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,11 @@ 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", conn->handle);
+ 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
end of thread, other threads:[~2022-04-21 20:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-20 22:14 [PATCH] Bluetooth: hci_event: Fix checking for invalid handle on error status Luiz Augusto von Dentz
2022-04-21 6:43 ` kernel test robot
2022-04-21 6:43 ` kernel test robot
2022-04-21 15:57 ` Marcel Holtmann
2022-04-21 20:52 ` Luiz Augusto von Dentz
2022-04-21 20:47 Luiz Augusto von Dentz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.