diff mbox series

Bluetooth: hci_event: Fix checking for invalid handle on error status

Message ID 20220420221433.2933868-1-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Headers show
Series Bluetooth: hci_event: Fix checking for invalid handle on error status | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint fail 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)"
tedd_an/subjectprefix success PASS
tedd_an/buildkernel fail Build Kernel make FAIL: net/bluetooth/hci_event.c: In function ‘hci_conn_complete_evt’: net/bluetooth/hci_event.c:3071:7: 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:7: 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:7: 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 make[2]: *** [scripts/Makefile.build:288: net/bluetooth/hci_event.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [scripts/Makefile.build:550: net/bluetooth] Error 2 make: *** [Makefile:1834: net] Error 2
tedd_an/buildkernel32 fail Build Kernel32 make FAIL: net/bluetooth/hci_event.c: In function ‘hci_conn_complete_evt’: net/bluetooth/hci_event.c:3071:7: 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:7: 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:7: 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 make[2]: *** [scripts/Makefile.build:288: net/bluetooth/hci_event.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [scripts/Makefile.build:550: net/bluetooth] Error 2 make: *** [Makefile:1834: net] Error 2
tedd_an/incremental_build pending Incremental Build Kernel SKIP(Build Fail)
tedd_an/testrunnersetup fail Test Runner Setup Build Kernel FAIL

Commit Message

Luiz Augusto von Dentz April 20, 2022, 10:14 p.m. UTC 
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(-)

Comments

kernel test robot April 21, 2022, 6:43 a.m. UTC | #1 
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
kernel test robot April 21, 2022, 6:43 a.m. UTC | #2 
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
Marcel Holtmann April 21, 2022, 3:57 p.m. UTC | #3 
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
Luiz Augusto von Dentz April 21, 2022, 8:52 p.m. UTC | #4 
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
>
diff mbox series

Patch

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;
 	}