From: Luiz Augusto von Dentz <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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/[email protected]/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 <[email protected]>
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
Hi Marcel,
On Thu, Apr 21, 2022 at 8:57 AM Marcel Holtmann <[email protected]> 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 <[email protected]>
> > ---
> > 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