[PATCH 0/8] Bluetooth: Various mgmt fixes

[PATCH 0/8] Bluetooth: Various mgmt fixes

[Johan Hedberg]

Hi,

This this set of patches fixes various issues with the management
interface that have been found in the past few weeks. It also improves
the pass-rate of the user space mgmt-tester to all but one test case
(which I'm not 100% sure is correct anyway):

Total: 35, Passed: 34 (97.1%), Failed: 1, Not Run: 0

Johan Hedberg (8):
      Bluetooth: Fix missing command complete event for mgmt_confirm_name
      Bluetooth: Fix missing command complete for mgmt_load_long_term_keys
      Bluetooth: Fix checking for valid device class values
      Bluetooth: Fix accepting set_dev_class for non-BR/EDR controllers
      Bluetooth: Fix returning proper command status for start_discovery
      Bluetooth: Move non-critical sections outside of the dev lock
      Bluetooth: Fix checking for exact values of boolean mgmt parameters
      Bluetooth: Fix sending incorrect new_settings for mgmt_set_powered

 net/bluetooth/mgmt.c |  124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 91 insertions(+), 33 deletions(-)

Johan

[PATCH 1/8] Bluetooth: Fix missing command complete event for mgmt_confirm_name

[Johan Hedberg]

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

All management commands are expected to indicate successful completion
through a command complete event however the confirm name command was
missing it. This patch add the sending of the missing event.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f559b96..3bcb63e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2510,7 +2510,8 @@ static int confirm_name(struct sock *sk, struct hci_dev *hdev, void *data,
 		hci_inquiry_cache_update_resolve(hdev, e);
 	}
 
-	err = 0;
+	err = cmd_complete(sk, hdev->id, MGMT_OP_CONFIRM_NAME, 0, &cp->addr,
+			   sizeof(cp->addr));
 
 failed:
 	hci_dev_unlock(hdev);
-- 
1.7.10.4

[PATCH 2/8] Bluetooth: Fix missing command complete for mgmt_load_long_term_keys

[Johan Hedberg]

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

All management events are expected to indicate successful completion
through a command complete event, however  the load long term keys
command was missing this. This patch adds the missing event.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3bcb63e..aaf9ce6 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2665,7 +2665,7 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
 {
 	struct mgmt_cp_load_long_term_keys *cp = cp_data;
 	u16 key_count, expected_len;
-	int i;
+	int i, err;
 
 	key_count = __le16_to_cpu(cp->key_count);
 
@@ -2699,9 +2699,12 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
 			    key->enc_size, key->ediv, key->rand);
 	}
 
+	err = cmd_complete(sk, hdev->id, MGMT_OP_LOAD_LONG_TERM_KEYS, 0,
+			   NULL, 0);
+
 	hci_dev_unlock(hdev);
 
-	return 0;
+	return err;
 }
 
 static const struct mgmt_handler {
-- 
1.7.10.4

[PATCH 3/8] Bluetooth: Fix checking for valid device class values

[Johan Hedberg]

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

The two lowest bits of the minor device class value are reserved and
should be zero, and the three highest bits of the major device class
likewise. The management code should therefore test for this and return
a proper "invalid params" error if the condition is not met.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index aaf9ce6..2785de2 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1430,6 +1430,12 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
+	if ((cp->minor & 0x03) != 0 || (cp->major & 0xe0) != 0) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
+				 MGMT_STATUS_INVALID_PARAMS);
+		goto unlock;
+	}
+
 	hdev->major_class = cp->major;
 	hdev->minor_class = cp->minor;
 
-- 
1.7.10.4

[PATCH 4/8] Bluetooth: Fix accepting set_dev_class for non-BR/EDR controllers

[Johan Hedberg]

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

The concept of Class of Device only exists for BR/EDR controllers. The
mgmt_set_dev_class command should therefore return a proper "not
supported" error if it is attempted for a controller that doesn't
support BR/EDR (e.g. a single mode LE-only one).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 2785de2..6e6de9e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1424,6 +1424,12 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_dev_lock(hdev);
 
+	if (!lmp_bredr_capable(hdev)) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
+				 MGMT_STATUS_NOT_SUPPORTED);
+		goto unlock;
+	}
+
 	if (test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
 		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
 				 MGMT_STATUS_BUSY);
-- 
1.7.10.4

[PATCH 5/8] Bluetooth: Fix returning proper command status for start_discovery

[Johan Hedberg]

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

Management commands should whenever possible fail with proper command
status or command complete events. This patch fixes the
mgmt_start_discovery command to do this for the failure cases where an
incorrect parameter value was passed to it ("not supported" if the
parameter value was valid but the controller doesn't support it and
"invalid params" if it isn't valid at all).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |   35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6e6de9e..4f60540 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2377,31 +2377,46 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 
 	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_BREDR:
-		if (lmp_bredr_capable(hdev))
+		if (lmp_bredr_capable(hdev)) {
 			err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
-		else
-			err = -ENOTSUPP;
+		} else {
+			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
+					 MGMT_STATUS_NOT_SUPPORTED);
+			mgmt_pending_remove(cmd);
+			goto failed;
+		}
 		break;
 
 	case DISCOV_TYPE_LE:
-		if (lmp_host_le_capable(hdev))
+		if (lmp_host_le_capable(hdev)) {
 			err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
 					  LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
-		else
-			err = -ENOTSUPP;
+		} else {
+			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
+					 MGMT_STATUS_NOT_SUPPORTED);
+			mgmt_pending_remove(cmd);
+			goto failed;
+		}
 		break;
 
 	case DISCOV_TYPE_INTERLEAVED:
-		if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
+		if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev)) {
 			err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
 					  LE_SCAN_WIN,
 					  LE_SCAN_TIMEOUT_BREDR_LE);
-		else
-			err = -ENOTSUPP;
+		} else {
+			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
+					 MGMT_STATUS_NOT_SUPPORTED);
+			mgmt_pending_remove(cmd);
+			goto failed;
+		}
 		break;
 
 	default:
-		err = -EINVAL;
+		err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
+				 MGMT_STATUS_INVALID_PARAMS);
+		mgmt_pending_remove(cmd);
+		goto failed;
 	}
 
 	if (err < 0)
-- 
1.7.10.4

[PATCH 6/8] Bluetooth: Move non-critical sections outside of the dev lock

[Johan Hedberg]

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

This patch fixes sections of code that do not need hci_lock_dev to be
outside of the lock. Such sections include code that do not touch the
hdev at all as well as sections which just read a single byte from the
supported_features value (i.e. all lmp_*_capable() macros).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |   46 ++++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 4f60540..69221ce 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1133,13 +1133,11 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 	BT_DBG("request for %s", hdev->name);
 
-	hci_dev_lock(hdev);
+	if (!lmp_ssp_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
+				  MGMT_STATUS_NOT_SUPPORTED);
 
-	if (!lmp_ssp_capable(hdev)) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
-				 MGMT_STATUS_NOT_SUPPORTED);
-		goto failed;
-	}
+	hci_dev_lock(hdev);
 
 	val = !!cp->val;
 
@@ -1217,13 +1215,11 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 	BT_DBG("request for %s", hdev->name);
 
-	hci_dev_lock(hdev);
+	if (!lmp_le_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
+				  MGMT_STATUS_NOT_SUPPORTED);
 
-	if (!lmp_le_capable(hdev)) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
-				 MGMT_STATUS_NOT_SUPPORTED);
-		goto unlock;
-	}
+	hci_dev_lock(hdev);
 
 	val = !!cp->val;
 	enabled = lmp_host_le_capable(hdev);
@@ -1422,25 +1418,19 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
-	hci_dev_lock(hdev);
+	if (!lmp_bredr_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
+				  MGMT_STATUS_NOT_SUPPORTED);
 
-	if (!lmp_bredr_capable(hdev)) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
-				 MGMT_STATUS_NOT_SUPPORTED);
-		goto unlock;
-	}
+	if (test_bit(HCI_PENDING_CLASS, &hdev->dev_flags))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
+				  MGMT_STATUS_BUSY);
 
-	if (test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
-				 MGMT_STATUS_BUSY);
-		goto unlock;
-	}
+	if ((cp->minor & 0x03) != 0 || (cp->major & 0xe0) != 0)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
+				  MGMT_STATUS_INVALID_PARAMS);
 
-	if ((cp->minor & 0x03) != 0 || (cp->major & 0xe0) != 0) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
-				 MGMT_STATUS_INVALID_PARAMS);
-		goto unlock;
-	}
+	hci_dev_lock(hdev);
 
 	hdev->major_class = cp->major;
 	hdev->minor_class = cp->minor;
-- 
1.7.10.4

[PATCH 7/8] Bluetooth: Fix checking for exact values of boolean mgmt parameters

[Johan Hedberg]

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

All mgmt_set_* commands that take a boolean value encoded in the form of
a byte should only accept the values 0x00 and 0x01. This patch adds the
necessary checks for this and returns "invalid params" responses if
anything else is provided as the value.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 69221ce..eb9d48a 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -777,6 +777,10 @@ static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_POWERED,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags)) {
@@ -872,6 +876,10 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
 				 MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	timeout = __le16_to_cpu(cp->timeout);
 	if (!cp->val && timeout > 0)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
@@ -971,6 +979,10 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	if (!hdev_is_powered(hdev)) {
@@ -1041,6 +1053,10 @@ static int set_pairable(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_PAIRABLE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	if (cp->val)
@@ -1073,6 +1089,10 @@ static int set_link_security(struct sock *sk, struct hci_dev *hdev, void *data,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	if (!hdev_is_powered(hdev)) {
@@ -1137,6 +1157,10 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	val = !!cp->val;
@@ -1197,6 +1221,10 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	if (cp->val)
 		set_bit(HCI_HS_ENABLED, &hdev->dev_flags);
 	else
@@ -1219,6 +1247,10 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	val = !!cp->val;
@@ -2630,6 +2662,10 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	if (!hdev_is_powered(hdev))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
 				  MGMT_STATUS_NOT_POWERED);
-- 
1.7.10.4

[PATCH 8/8] Bluetooth: Fix sending incorrect new_settings for mgmt_set_powered

[Johan Hedberg]

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

The socket from which a mgmt_set_powered command was received should
only receive the command response but no new_settings event.

The mgmt_powered() function which is used to handle the situation with
the HCI_AUTO_OFF flag tries to check for a pending command to know which
socket to skip the event for, but since the pending command hasn't been
added this will not happen.

This patch fixes the issue by adding the pending command for the
HCI_AUTO_OFF case and thereby ensures that mgmt_powered() will skip the
right socket when sending the new_settings event, but still send the
proper response to the socket where the command came from.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index eb9d48a..60afaee 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -787,8 +787,9 @@ static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
 		cancel_delayed_work(&hdev->power_off);
 
 		if (cp->val) {
-			err = send_settings_rsp(sk, MGMT_OP_SET_POWERED, hdev);
-			mgmt_powered(hdev, 1);
+			mgmt_pending_add(sk, MGMT_OP_SET_POWERED, hdev,
+					 data, len);
+			err = mgmt_powered(hdev, 1);
 			goto failed;
 		}
 	}
-- 
1.7.10.4

Re: [PATCH 7/8] Bluetooth: Fix checking for exact values of boolean mgmt parameters

[Anderson Lizardo]

Hi Johan,

On Wed, Jan 9, 2013 at 9:29 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> @@ -1219,6 +1247,10 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>                 return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
>                                   MGMT_STATUS_NOT_SUPPORTED);
>
> +       if (cp->val != 0x00 && cp->val != 0x01)
> +               return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
> +                                 MGMT_STATUS_INVALID_PARAMS);
> +

I think you meant "MGMT_OP_SET_LE" here.

Can mgmt-tester be modified to check for this as well?

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 7/8] Bluetooth: Fix checking for exact values of boolean mgmt parameters

[Johan Hedberg]

Hi Lizardo,

On Wed, Jan 09, 2013, Anderson Lizardo wrote:
> Hi Johan,
> 
> On Wed, Jan 9, 2013 at 9:29 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > @@ -1219,6 +1247,10 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> >                 return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> >                                   MGMT_STATUS_NOT_SUPPORTED);
> >
> > +       if (cp->val != 0x00 && cp->val != 0x01)
> > +               return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
> > +                                 MGMT_STATUS_INVALID_PARAMS);
> > +
> 
> I think you meant "MGMT_OP_SET_LE" here.

Good catch. A copy-paste mistake I should have caught.

> Can mgmt-tester be modified to check for this as well?

Yes, I don't think it has any set_le test cases yet. Patches are
welcome!

Johan

Re: [PATCH 7/8] Bluetooth: Fix checking for exact values of boolean mgmt parameters

[Anderson Lizardo]

Hi Johan,

On Wed, Jan 9, 2013 at 9:48 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> Can mgmt-tester be modified to check for this as well?
>
> Yes, I don't think it has any set_le test cases yet. Patches are
> welcome!

Unless someone else takes care of this, I can check after finishing
the SDP unit tests I was working on.

>
> Johan

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

[PATCH 7/8 v2] Bluetooth: Fix checking for exact values of boolean mgmt parameters

[Johan Hedberg]

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

All mgmt_set_* commands that take a boolean value encoded in the form of
a byte should only accept the values 0x00 and 0x01. This patch adds the
necessary checks for this and returns "invalid params" responses if
anything else is provided as the value.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v2: Fix s/SET_SSP/SET_LE/ copy-paste issue

 net/bluetooth/mgmt.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 69221ce..3cf7e1d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -777,6 +777,10 @@ static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_POWERED,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags)) {
@@ -872,6 +876,10 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
 				 MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	timeout = __le16_to_cpu(cp->timeout);
 	if (!cp->val && timeout > 0)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
@@ -971,6 +979,10 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	if (!hdev_is_powered(hdev)) {
@@ -1041,6 +1053,10 @@ static int set_pairable(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_PAIRABLE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	if (cp->val)
@@ -1073,6 +1089,10 @@ static int set_link_security(struct sock *sk, struct hci_dev *hdev, void *data,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	if (!hdev_is_powered(hdev)) {
@@ -1137,6 +1157,10 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	val = !!cp->val;
@@ -1197,6 +1221,10 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	if (cp->val)
 		set_bit(HCI_HS_ENABLED, &hdev->dev_flags);
 	else
@@ -1219,6 +1247,10 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 
 	val = !!cp->val;
@@ -2630,6 +2662,10 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
+				  MGMT_STATUS_INVALID_PARAMS);
+
 	if (!hdev_is_powered(hdev))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
 				  MGMT_STATUS_NOT_POWERED);
-- 
1.7.10.4

Re: [PATCH 1/8] Bluetooth: Fix missing command complete event for mgmt_confirm_name

[Marcel Holtmann]

Hi Johan,

> All management commands are expected to indicate successful completion
> through a command complete event however the confirm name command was
> missing it. This patch add the sending of the missing event.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 2/8] Bluetooth: Fix missing command complete for mgmt_load_long_term_keys

[Marcel Holtmann]

Hi Johan,

> All management events are expected to indicate successful completion
> through a command complete event, however  the load long term keys
> command was missing this. This patch adds the missing event.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 3/8] Bluetooth: Fix checking for valid device class values

[Marcel Holtmann]

Hi Johan,

> The two lowest bits of the minor device class value are reserved and
> should be zero, and the three highest bits of the major device class
> likewise. The management code should therefore test for this and return
> a proper "invalid params" error if the condition is not met.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index aaf9ce6..2785de2 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1430,6 +1430,12 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
>  		goto unlock;
>  	}
>  
> +	if ((cp->minor & 0x03) != 0 || (cp->major & 0xe0) != 0) {
> +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
> +				 MGMT_STATUS_INVALID_PARAMS);
> +		goto unlock;
> +	}

we could do

	if ((cp->minor & 0x03) || ...) {

However I am not sure what is preferred and I am fine both ways.

> +
>  	hdev->major_class = cp->major;
>  	hdev->minor_class = cp->minor;
>  

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 4/8] Bluetooth: Fix accepting set_dev_class for non-BR/EDR controllers

[Marcel Holtmann]

Hi Johan,

> The concept of Class of Device only exists for BR/EDR controllers. The
> mgmt_set_dev_class command should therefore return a proper "not
> supported" error if it is attempted for a controller that doesn't
> support BR/EDR (e.g. a single mode LE-only one).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |    6 ++++++
>  1 file changed, 6 insertions(+)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 5/8] Bluetooth: Fix returning proper command status for start_discovery

[Marcel Holtmann]

Hi Johan,

> Management commands should whenever possible fail with proper command
> status or command complete events. This patch fixes the
> mgmt_start_discovery command to do this for the failure cases where an
> incorrect parameter value was passed to it ("not supported" if the
> parameter value was valid but the controller doesn't support it and
> "invalid params" if it isn't valid at all).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |   35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 6e6de9e..4f60540 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2377,31 +2377,46 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>  
>  	switch (hdev->discovery.type) {
>  	case DISCOV_TYPE_BREDR:
> -		if (lmp_bredr_capable(hdev))
> +		if (lmp_bredr_capable(hdev)) {
>  			err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> -		else
> -			err = -ENOTSUPP;
> +		} else {
> +			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> +					 MGMT_STATUS_NOT_SUPPORTED);
> +			mgmt_pending_remove(cmd);
> +			goto failed;
> +		}

I would have done this the other way around.

	if (!lmp_bredr_capable(...)) {
		...
		got failed;
	}

	err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
	break;

>  		break;
>  
>  	case DISCOV_TYPE_LE:
> -		if (lmp_host_le_capable(hdev))
> +		if (lmp_host_le_capable(hdev)) {
>  			err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
>  					  LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
> -		else
> -			err = -ENOTSUPP;
> +		} else {
> +			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> +					 MGMT_STATUS_NOT_SUPPORTED);
> +			mgmt_pending_remove(cmd);
> +			goto failed;
> +		}

And same here.

>  		break;
>  
>  	case DISCOV_TYPE_INTERLEAVED:
> -		if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
> +		if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev)) {
>  			err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
>  					  LE_SCAN_WIN,
>  					  LE_SCAN_TIMEOUT_BREDR_LE);
> -		else
> -			err = -ENOTSUPP;
> +		} else {
> +			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> +					 MGMT_STATUS_NOT_SUPPORTED);
> +			mgmt_pending_remove(cmd);
> +			goto failed;
> +		}
>  		break;

Also here.

>  
>  	default:
> -		err = -EINVAL;
> +		err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> +				 MGMT_STATUS_INVALID_PARAMS);
> +		mgmt_pending_remove(cmd);
> +		goto failed;
>  	}
>  
>  	if (err < 0)

Regards

Marcel

Re: [PATCH 6/8] Bluetooth: Move non-critical sections outside of the dev lock

[Marcel Holtmann]

Hi Johan,

> This patch fixes sections of code that do not need hci_lock_dev to be
> outside of the lock. Such sections include code that do not touch the
> hdev at all as well as sections which just read a single byte from the
> supported_features value (i.e. all lmp_*_capable() macros).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |   46 ++++++++++++++++++----------------------------
>  1 file changed, 18 insertions(+), 28 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 7/8 v2] Bluetooth: Fix checking for exact values of boolean mgmt parameters

[Marcel Holtmann]

Hi Johan,

> All mgmt_set_* commands that take a boolean value encoded in the form of
> a byte should only accept the values 0x00 and 0x01. This patch adds the
> necessary checks for this and returns "invalid params" responses if
> anything else is provided as the value.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> v2: Fix s/SET_SSP/SET_LE/ copy-paste issue
> 
>  net/bluetooth/mgmt.c |   36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 7/8 v2] Bluetooth: Fix checking for exact values of boolean mgmt parameters

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2013-01-09 16:05:19 +0200]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> All mgmt_set_* commands that take a boolean value encoded in the form of
> a byte should only accept the values 0x00 and 0x01. This patch adds the
> necessary checks for this and returns "invalid params" responses if
> anything else is provided as the value.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> v2: Fix s/SET_SSP/SET_LE/ copy-paste issue
> 
>  net/bluetooth/mgmt.c |   36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)

All patches but 5 and 8 were applied to bluetooth-next. Thanks.

	Gustavo

Re: [PATCH 8/8] Bluetooth: Fix sending incorrect new_settings for mgmt_set_powered

[Marcel Holtmann]

Hi Johan,

> The socket from which a mgmt_set_powered command was received should
> only receive the command response but no new_settings event.
> 
> The mgmt_powered() function which is used to handle the situation with
> the HCI_AUTO_OFF flag tries to check for a pending command to know which
> socket to skip the event for, but since the pending command hasn't been
> added this will not happen.
> 
> This patch fixes the issue by adding the pending command for the
> HCI_AUTO_OFF case and thereby ensures that mgmt_powered() will skip the
> right socket when sending the new_settings event, but still send the
> proper response to the socket where the command came from.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

[PATCH 5/8 v2] Bluetooth: Fix returning proper command status for start_discovery

[Johan Hedberg]

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

Management commands should whenever possible fail with proper command
status or command complete events. This patch fixes the
mgmt_start_discovery command to do this for the failure cases where an
incorrect parameter value was passed to it ("not supported" if the
parameter value was valid but the controller doesn't support it and
"invalid params" if it isn't valid at all).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v2: Use proposed logic for testing for not supported parameters

 net/bluetooth/mgmt.c |   46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6cff286..37add53 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2383,31 +2383,45 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 
 	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_BREDR:
-		if (lmp_bredr_capable(hdev))
-			err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
-		else
-			err = -ENOTSUPP;
+		if (!lmp_bredr_capable(hdev)) {
+			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
+					 MGMT_STATUS_NOT_SUPPORTED);
+			mgmt_pending_remove(cmd);
+			goto failed;
+		}
+
+		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
 		break;
 
 	case DISCOV_TYPE_LE:
-		if (lmp_host_le_capable(hdev))
-			err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
-					  LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
-		else
-			err = -ENOTSUPP;
+		if (!lmp_host_le_capable(hdev)) {
+			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
+					 MGMT_STATUS_NOT_SUPPORTED);
+			mgmt_pending_remove(cmd);
+			goto failed;
+		}
+
+		err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
+				  LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
 		break;
 
 	case DISCOV_TYPE_INTERLEAVED:
-		if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
-			err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
-					  LE_SCAN_WIN,
-					  LE_SCAN_TIMEOUT_BREDR_LE);
-		else
-			err = -ENOTSUPP;
+		if (!lmp_host_le_capable(hdev) || !lmp_bredr_capable(hdev)) {
+			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
+					 MGMT_STATUS_NOT_SUPPORTED);
+			mgmt_pending_remove(cmd);
+			goto failed;
+		}
+
+		err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT, LE_SCAN_WIN,
+				  LE_SCAN_TIMEOUT_BREDR_LE);
 		break;
 
 	default:
-		err = -EINVAL;
+		err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
+				 MGMT_STATUS_INVALID_PARAMS);
+		mgmt_pending_remove(cmd);
+		goto failed;
 	}
 
 	if (err < 0)
-- 
1.7.10.4

Re: [PATCH 5/8 v2] Bluetooth: Fix returning proper command status for start_discovery

[Marcel Holtmann]

Hi Johan,

> Management commands should whenever possible fail with proper command
> status or command complete events. This patch fixes the
> mgmt_start_discovery command to do this for the failure cases where an
> incorrect parameter value was passed to it ("not supported" if the
> parameter value was valid but the controller doesn't support it and
> "invalid params" if it isn't valid at all).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> v2: Use proposed logic for testing for not supported parameters
> 
>  net/bluetooth/mgmt.c |   46 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 16 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 5/8 v2] Bluetooth: Fix returning proper command status for start_discovery

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2013-01-10 14:54:09 +0200]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> Management commands should whenever possible fail with proper command
> status or command complete events. This patch fixes the
> mgmt_start_discovery command to do this for the failure cases where an
> incorrect parameter value was passed to it ("not supported" if the
> parameter value was valid but the controller doesn't support it and
> "invalid params" if it isn't valid at all).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> v2: Use proposed logic for testing for not supported parameters
> 
>  net/bluetooth/mgmt.c |   46 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 16 deletions(-)

Patch has been applied. Thanks.

	Gustavo

Re: [PATCH 8/8] Bluetooth: Fix sending incorrect new_settings for mgmt_set_powered

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2013-01-09 15:29:40 +0200]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> The socket from which a mgmt_set_powered command was received should
> only receive the command response but no new_settings event.
> 
> The mgmt_powered() function which is used to handle the situation with
> the HCI_AUTO_OFF flag tries to check for a pending command to know which
> socket to skip the event for, but since the pending command hasn't been
> added this will not happen.
> 
> This patch fixes the issue by adding the pending command for the
> HCI_AUTO_OFF case and thereby ensures that mgmt_powered() will skip the
> right socket when sending the new_settings event, but still send the
> proper response to the socket where the command came from.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Patch has been applied, thanks.

	Gustavo