[PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Even though the HCI_Delete_Stored_Link_Key command is mandatory for 1.1
and later controllers some controllers do not seem to support it
properly as was witnessed by one Broadcom based controller:

< HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
    bdaddr 00:00:00:00:00:00 all 1
> HCI Event: Command Complete (0x0e) plen 4
    Delete Stored Link Key (0x03|0x0012) ncmd 1
    status 0x11 deleted 0
    Error: Unsupported Feature or Parameter Value

Luckily this same controller also doesn't list the command in its
supported commands bit mask (counting from 0 bit 7 of octet 6):

< HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
> HCI Event: Command Complete (0x0e) plen 68
    Read Local Supported Commands (0x04|0x0002) ncmd 1
    status 0x00
    Commands: ffffffffffff1ffffffffffff30fffff3f

Therefore, it makes sense to move sending of HCI_Delete_Stored_Link_Key
to after receiving the supported commands response and to only send it
if its respective bit in the mask is set. The downside of this is that
we no longer send the HCI_Delete_Stored_Link_Key command for Bluetooth
1.1 controllers since HCI_Read_Local_Supported_Command was introduced in
version 1.2, but this is an acceptable penalty as the command in
question shouldn't affect critical behavior.

Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d817c93..d68425c 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -341,7 +341,6 @@ static void hci_init1_req(struct hci_request *req, unsigned long opt)
 
 static void bredr_setup(struct hci_request *req)
 {
-	struct hci_cp_delete_stored_link_key cp;
 	__le16 param;
 	__u8 flt_type;
 
@@ -365,10 +364,6 @@ static void bredr_setup(struct hci_request *req)
 	param = __constant_cpu_to_le16(0x7d00);
 	hci_req_add(req, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
 
-	bacpy(&cp.bdaddr, BDADDR_ANY);
-	cp.delete_all = 0x01;
-	hci_req_add(req, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
-
 	/* Read page scan parameters */
 	if (req->hdev->hci_ver > BLUETOOTH_VER_1_1) {
 		hci_req_add(req, HCI_OP_READ_PAGE_SCAN_ACTIVITY, 0, NULL);
@@ -602,6 +597,15 @@ static void hci_init3_req(struct hci_request *req, unsigned long opt)
 	struct hci_dev *hdev = req->hdev;
 	u8 p;
 
+	if (hdev->commands[6] & 0x80) {
+		struct hci_cp_delete_stored_link_key cp;
+
+		bacpy(&cp.bdaddr, BDADDR_ANY);
+		cp.delete_all = 0x01;
+		hci_req_add(req, HCI_OP_DELETE_STORED_LINK_KEY,
+			    sizeof(cp), &cp);
+	}
+
 	if (hdev->commands[5] & 0x10)
 		hci_setup_link_policy(req);
 
-- 
1.7.10.4

Re: [PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[Pavel Machek]

> Therefore, it makes sense to move sending of HCI_Delete_Stored_Link_Key
> to after receiving the supported commands response and to only send it
> if its respective bit in the mask is set. The downside of this is that
> we no longer send the HCI_Delete_Stored_Link_Key command for Bluetooth
> 1.1 controllers since HCI_Read_Local_Supported_Command was introduced in
> version 1.2, but this is an acceptable penalty as the command in
> question shouldn't affect critical behavior.
> 
> Reported-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>

Tested-by: Pavel Machek <pavel@ucw.cz>

(Addiditonaly, if you are right and this was in 3.9 already, we
probably want to cc: stable@kernel.org).

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[Johan Hedberg]

Hi Pavel,

On Thu, Jun 13, 2013, Pavel Machek wrote:
> > Therefore, it makes sense to move sending of HCI_Delete_Stored_Link_Key
> > to after receiving the supported commands response and to only send it
> > if its respective bit in the mask is set. The downside of this is that
> > we no longer send the HCI_Delete_Stored_Link_Key command for Bluetooth
> > 1.1 controllers since HCI_Read_Local_Supported_Command was introduced in
> > version 1.2, but this is an acceptable penalty as the command in
> > question shouldn't affect critical behavior.
> > 
> > Reported-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> 
> Tested-by: Pavel Machek <pavel@ucw.cz>

Thanks!

> (Addiditonaly, if you are right and this was in 3.9 already, we
> probably want to cc: stable@kernel.org).

Even though the command itself was in 3.9 it wasn't using the request
framework that was introduced in 3.10 (which for now is depending on all
commands succeeding) so I think you're right after all that it's an
regression since 3.9. Also, because this patch uses the API of the
request framework (hci_req_add) it will not be trivially backportable to
older kernels, so I'd just forget about cc:stable.

Johan

Re: [PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2013-06-13 11:01:13 +0300]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> Even though the HCI_Delete_Stored_Link_Key command is mandatory for 1.1
> and later controllers some controllers do not seem to support it
> properly as was witnessed by one Broadcom based controller:
> 
> < HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
>     bdaddr 00:00:00:00:00:00 all 1
> > HCI Event: Command Complete (0x0e) plen 4
>     Delete Stored Link Key (0x03|0x0012) ncmd 1
>     status 0x11 deleted 0
>     Error: Unsupported Feature or Parameter Value
> 
> Luckily this same controller also doesn't list the command in its
> supported commands bit mask (counting from 0 bit 7 of octet 6):
> 
> < HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
> > HCI Event: Command Complete (0x0e) plen 68
>     Read Local Supported Commands (0x04|0x0002) ncmd 1
>     status 0x00
>     Commands: ffffffffffff1ffffffffffff30fffff3f
> 
> Therefore, it makes sense to move sending of HCI_Delete_Stored_Link_Key
> to after receiving the supported commands response and to only send it
> if its respective bit in the mask is set. The downside of this is that
> we no longer send the HCI_Delete_Stored_Link_Key command for Bluetooth
> 1.1 controllers since HCI_Read_Local_Supported_Command was introduced in
> version 1.2, but this is an acceptable penalty as the command in
> question shouldn't affect critical behavior.
> 
> Reported-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_core.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Patch has been applied to bluetooth.git. Thanks.

	Gustavo

Re: [PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[Marcel Holtmann]

Hi Johan,

> Even though the HCI_Delete_Stored_Link_Key command is mandatory for 1.1
> and later controllers some controllers do not seem to support it
> properly as was witnessed by one Broadcom based controller:
> 
> < HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
>    bdaddr 00:00:00:00:00:00 all 1
>> HCI Event: Command Complete (0x0e) plen 4
>    Delete Stored Link Key (0x03|0x0012) ncmd 1
>    status 0x11 deleted 0
>    Error: Unsupported Feature or Parameter Value
> 
> Luckily this same controller also doesn't list the command in its
> supported commands bit mask (counting from 0 bit 7 of octet 6):
> 
> < HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
>> HCI Event: Command Complete (0x0e) plen 68
>    Read Local Supported Commands (0x04|0x0002) ncmd 1
>    status 0x00
>    Commands: ffffffffffff1ffffffffffff30fffff3f
> 
> Therefore, it makes sense to move sending of HCI_Delete_Stored_Link_Key
> to after receiving the supported commands response and to only send it
> if its respective bit in the mask is set. The downside of this is that
> we no longer send the HCI_Delete_Stored_Link_Key command for Bluetooth
> 1.1 controllers since HCI_Read_Local_Supported_Command was introduced in
> version 1.2, but this is an acceptable penalty as the command in
> question shouldn't affect critical behavior.
> 
> Reported-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_core.c |   14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index d817c93..d68425c 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -341,7 +341,6 @@ static void hci_init1_req(struct hci_request *req, unsigned long opt)
> 
> static void bredr_setup(struct hci_request *req)
> {
> -	struct hci_cp_delete_stored_link_key cp;
> 	__le16 param;
> 	__u8 flt_type;
> 
> @@ -365,10 +364,6 @@ static void bredr_setup(struct hci_request *req)
> 	param = __constant_cpu_to_le16(0x7d00);
> 	hci_req_add(req, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
> 
> -	bacpy(&cp.bdaddr, BDADDR_ANY);
> -	cp.delete_all = 0x01;
> -	hci_req_add(req, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
> -
> 	/* Read page scan parameters */
> 	if (req->hdev->hci_ver > BLUETOOTH_VER_1_1) {
> 		hci_req_add(req, HCI_OP_READ_PAGE_SCAN_ACTIVITY, 0, NULL);
> @@ -602,6 +597,15 @@ static void hci_init3_req(struct hci_request *req, unsigned long opt)
> 	struct hci_dev *hdev = req->hdev;
> 	u8 p;

I would have added here a comment to explain what bit we are checking for.

> 
> +	if (hdev->commands[6] & 0x80) {
> +		struct hci_cp_delete_stored_link_key cp;
> +
> +		bacpy(&cp.bdaddr, BDADDR_ANY);
> +		cp.delete_all = 0x01;
> +		hci_req_add(req, HCI_OP_DELETE_STORED_LINK_KEY,
> +			    sizeof(cp), &cp);
> +	}
> +
> 	if (hdev->commands[5] & 0x10)
> 		hci_setup_link_policy(req);

Regards

Marcel

Re: [PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[Gustavo Padovan]

Hi Marcel,

* Marcel Holtmann <marcel@holtmann.org> [2013-06-13 12:34:09 +0200]:

> Hi Johan,
> 
> > Even though the HCI_Delete_Stored_Link_Key command is mandatory for 1.1
> > and later controllers some controllers do not seem to support it
> > properly as was witnessed by one Broadcom based controller:
> > 
> > < HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
> >    bdaddr 00:00:00:00:00:00 all 1
> >> HCI Event: Command Complete (0x0e) plen 4
> >    Delete Stored Link Key (0x03|0x0012) ncmd 1
> >    status 0x11 deleted 0
> >    Error: Unsupported Feature or Parameter Value
> > 
> > Luckily this same controller also doesn't list the command in its
> > supported commands bit mask (counting from 0 bit 7 of octet 6):
> > 
> > < HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
> >> HCI Event: Command Complete (0x0e) plen 68
> >    Read Local Supported Commands (0x04|0x0002) ncmd 1
> >    status 0x00
> >    Commands: ffffffffffff1ffffffffffff30fffff3f
> > 
> > Therefore, it makes sense to move sending of HCI_Delete_Stored_Link_Key
> > to after receiving the supported commands response and to only send it
> > if its respective bit in the mask is set. The downside of this is that
> > we no longer send the HCI_Delete_Stored_Link_Key command for Bluetooth
> > 1.1 controllers since HCI_Read_Local_Supported_Command was introduced in
> > version 1.2, but this is an acceptable penalty as the command in
> > question shouldn't affect critical behavior.
> > 
> > Reported-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > net/bluetooth/hci_core.c |   14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index d817c93..d68425c 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -341,7 +341,6 @@ static void hci_init1_req(struct hci_request *req, unsigned long opt)
> > 
> > static void bredr_setup(struct hci_request *req)
> > {
> > -	struct hci_cp_delete_stored_link_key cp;
> > 	__le16 param;
> > 	__u8 flt_type;
> > 
> > @@ -365,10 +364,6 @@ static void bredr_setup(struct hci_request *req)
> > 	param = __constant_cpu_to_le16(0x7d00);
> > 	hci_req_add(req, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
> > 
> > -	bacpy(&cp.bdaddr, BDADDR_ANY);
> > -	cp.delete_all = 0x01;
> > -	hci_req_add(req, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
> > -
> > 	/* Read page scan parameters */
> > 	if (req->hdev->hci_ver > BLUETOOTH_VER_1_1) {
> > 		hci_req_add(req, HCI_OP_READ_PAGE_SCAN_ACTIVITY, 0, NULL);
> > @@ -602,6 +597,15 @@ static void hci_init3_req(struct hci_request *req, unsigned long opt)
> > 	struct hci_dev *hdev = req->hdev;
> > 	u8 p;
> 
> I would have added here a comment to explain what bit we are checking for.

I just added:

/* Only send HCI_Delete_Stored_Link_Key if it is supported */

	Gustavo

Re: [PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[Marcel Holtmann]

Hi Gustavo,

>>> Even though the HCI_Delete_Stored_Link_Key command is mandatory for 1.1
>>> and later controllers some controllers do not seem to support it
>>> properly as was witnessed by one Broadcom based controller:
>>> 
>>> < HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
>>>   bdaddr 00:00:00:00:00:00 all 1
>>>> HCI Event: Command Complete (0x0e) plen 4
>>>   Delete Stored Link Key (0x03|0x0012) ncmd 1
>>>   status 0x11 deleted 0
>>>   Error: Unsupported Feature or Parameter Value
>>> 
>>> Luckily this same controller also doesn't list the command in its
>>> supported commands bit mask (counting from 0 bit 7 of octet 6):
>>> 
>>> < HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
>>>> HCI Event: Command Complete (0x0e) plen 68
>>>   Read Local Supported Commands (0x04|0x0002) ncmd 1
>>>   status 0x00
>>>   Commands: ffffffffffff1ffffffffffff30fffff3f
>>> 
>>> Therefore, it makes sense to move sending of HCI_Delete_Stored_Link_Key
>>> to after receiving the supported commands response and to only send it
>>> if its respective bit in the mask is set. The downside of this is that
>>> we no longer send the HCI_Delete_Stored_Link_Key command for Bluetooth
>>> 1.1 controllers since HCI_Read_Local_Supported_Command was introduced in
>>> version 1.2, but this is an acceptable penalty as the command in
>>> question shouldn't affect critical behavior.
>>> 
>>> Reported-by: Pavel Machek <pavel@ucw.cz>
>>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>>> ---
>>> net/bluetooth/hci_core.c |   14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> index d817c93..d68425c 100644
>>> --- a/net/bluetooth/hci_core.c
>>> +++ b/net/bluetooth/hci_core.c
>>> @@ -341,7 +341,6 @@ static void hci_init1_req(struct hci_request *req, unsigned long opt)
>>> 
>>> static void bredr_setup(struct hci_request *req)
>>> {
>>> -	struct hci_cp_delete_stored_link_key cp;
>>> 	__le16 param;
>>> 	__u8 flt_type;
>>> 
>>> @@ -365,10 +364,6 @@ static void bredr_setup(struct hci_request *req)
>>> 	param = __constant_cpu_to_le16(0x7d00);
>>> 	hci_req_add(req, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
>>> 
>>> -	bacpy(&cp.bdaddr, BDADDR_ANY);
>>> -	cp.delete_all = 0x01;
>>> -	hci_req_add(req, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
>>> -
>>> 	/* Read page scan parameters */
>>> 	if (req->hdev->hci_ver > BLUETOOTH_VER_1_1) {
>>> 		hci_req_add(req, HCI_OP_READ_PAGE_SCAN_ACTIVITY, 0, NULL);
>>> @@ -602,6 +597,15 @@ static void hci_init3_req(struct hci_request *req, unsigned long opt)
>>> 	struct hci_dev *hdev = req->hdev;
>>> 	u8 p;
>> 
>> I would have added here a comment to explain what bit we are checking for.
> 
> I just added:
> 
> /* Only send HCI_Delete_Stored_Link_Key if it is supported */

that is the super cheap way out. There is no harm in going into details.

	/* Some Broadcom based Bluetooth controllers do not support the
	 * Delete Stored Link Key command. They are clearly indicating its
	 * absence in the bit mask of supported commands.
	 *
	 * Check the supported commands and only if the the command is marked as
	 * supported send it. If not supported assume that the controller does not
	 * have actual support for stored link keys which makes this command
	 * redundant anyway.
	 */

Regards

Marcel

Re: [PATCH] Bluetooth: Fix conditions for HCI_Delete_Stored_Link_Key

[Gustavo Padovan]

[-- Attachment #1: Type: text/plain, Size: 3829 bytes --]

Hi Marcel,

* Marcel Holtmann <marcel@holtmann.org> [2013-06-13 12:48:51 +0200]:

> Hi Gustavo,
> 
> >>> Even though the HCI_Delete_Stored_Link_Key command is mandatory for 1.1
> >>> and later controllers some controllers do not seem to support it
> >>> properly as was witnessed by one Broadcom based controller:
> >>> 
> >>> < HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
> >>>   bdaddr 00:00:00:00:00:00 all 1
> >>>> HCI Event: Command Complete (0x0e) plen 4
> >>>   Delete Stored Link Key (0x03|0x0012) ncmd 1
> >>>   status 0x11 deleted 0
> >>>   Error: Unsupported Feature or Parameter Value
> >>> 
> >>> Luckily this same controller also doesn't list the command in its
> >>> supported commands bit mask (counting from 0 bit 7 of octet 6):
> >>> 
> >>> < HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
> >>>> HCI Event: Command Complete (0x0e) plen 68
> >>>   Read Local Supported Commands (0x04|0x0002) ncmd 1
> >>>   status 0x00
> >>>   Commands: ffffffffffff1ffffffffffff30fffff3f
> >>> 
> >>> Therefore, it makes sense to move sending of HCI_Delete_Stored_Link_Key
> >>> to after receiving the supported commands response and to only send it
> >>> if its respective bit in the mask is set. The downside of this is that
> >>> we no longer send the HCI_Delete_Stored_Link_Key command for Bluetooth
> >>> 1.1 controllers since HCI_Read_Local_Supported_Command was introduced in
> >>> version 1.2, but this is an acceptable penalty as the command in
> >>> question shouldn't affect critical behavior.
> >>> 
> >>> Reported-by: Pavel Machek <pavel@ucw.cz>
> >>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> >>> ---
> >>> net/bluetooth/hci_core.c |   14 +++++++++-----
> >>> 1 file changed, 9 insertions(+), 5 deletions(-)
> >>> 
> >>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >>> index d817c93..d68425c 100644
> >>> --- a/net/bluetooth/hci_core.c
> >>> +++ b/net/bluetooth/hci_core.c
> >>> @@ -341,7 +341,6 @@ static void hci_init1_req(struct hci_request *req, unsigned long opt)
> >>> 
> >>> static void bredr_setup(struct hci_request *req)
> >>> {
> >>> -	struct hci_cp_delete_stored_link_key cp;
> >>> 	__le16 param;
> >>> 	__u8 flt_type;
> >>> 
> >>> @@ -365,10 +364,6 @@ static void bredr_setup(struct hci_request *req)
> >>> 	param = __constant_cpu_to_le16(0x7d00);
> >>> 	hci_req_add(req, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
> >>> 
> >>> -	bacpy(&cp.bdaddr, BDADDR_ANY);
> >>> -	cp.delete_all = 0x01;
> >>> -	hci_req_add(req, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
> >>> -
> >>> 	/* Read page scan parameters */
> >>> 	if (req->hdev->hci_ver > BLUETOOTH_VER_1_1) {
> >>> 		hci_req_add(req, HCI_OP_READ_PAGE_SCAN_ACTIVITY, 0, NULL);
> >>> @@ -602,6 +597,15 @@ static void hci_init3_req(struct hci_request *req, unsigned long opt)
> >>> 	struct hci_dev *hdev = req->hdev;
> >>> 	u8 p;
> >> 
> >> I would have added here a comment to explain what bit we are checking for.
> > 
> > I just added:
> > 
> > /* Only send HCI_Delete_Stored_Link_Key if it is supported */
> 
> that is the super cheap way out. There is no harm in going into details.
> 
> 	/* Some Broadcom based Bluetooth controllers do not support the
> 	 * Delete Stored Link Key command. They are clearly indicating its
> 	 * absence in the bit mask of supported commands.
> 	 *
> 	 * Check the supported commands and only if the the command is marked as
> 	 * supported send it. If not supported assume that the controller does not
> 	 * have actual support for stored link keys which makes this command
> 	 * redundant anyway.
> 	 */

Too late, I just sent out the pull request, I'll queue a patch here just to
add this comment and apply it once this lands on bluetooth-next.

	Gustavo

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]