[PATCH] Bluetooth: Fix powering on with privacy and advertising

[PATCH] Bluetooth: Fix powering on with privacy and advertising

[Andrzej Kaczmarek]

In order to enable advertising with privacy enabled, SMP has to be
registered in order to generate new RPA. During power on, it will be
registered at the very end which is the reason why advertising is not
enabled and it's not possible to enable it anymore due to mismatch
between hci_dev settings and actual controller state.

This fixes this problem by postponing advertising enable after SMP is
registered in case of power on.
---
 net/bluetooth/mgmt.c | 70 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index eca203e..d96b08c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7234,10 +7234,42 @@ static void restart_le_actions(struct hci_dev *hdev)
 	}
 }
 
-static void powered_complete(struct hci_dev *hdev, u8 status, u16 opcode)
+static void mgmt_set_powered_complete(struct hci_dev *hdev)
 {
 	struct cmd_lookup match = { NULL, hdev };
 
+	hci_dev_lock(hdev);
+
+	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
+
+	new_settings(hdev, match.sk);
+
+	hci_dev_unlock(hdev);
+
+	if (match.sk)
+		sock_put(match.sk);
+}
+
+static void powered_adv_complete(struct hci_dev *hdev, u8 status, u16 opcode)
+{
+	BT_DBG("status 0x%02x", status);
+
+	mgmt_set_powered_complete(hdev);
+}
+
+static void powered_enable_advertising(struct hci_dev *hdev,
+				       struct hci_request *req)
+{
+	if (hci_dev_test_flag(hdev, HCI_ADVERTISING))
+		enable_advertising(req);
+	else if (hci_dev_test_flag(hdev, HCI_ADVERTISING_INSTANCE) &&
+		 hdev->cur_adv_instance)
+		schedule_adv_instance(req, hdev->cur_adv_instance,
+				      true);
+}
+
+static void powered_complete(struct hci_dev *hdev, u8 status, u16 opcode)
+{
 	BT_DBG("status 0x%02x", status);
 
 	if (!status) {
@@ -7250,18 +7282,23 @@ static void powered_complete(struct hci_dev *hdev, u8 status, u16 opcode)
 
 		restart_le_actions(hdev);
 		hci_update_background_scan(hdev);
-	}
-
-	hci_dev_lock(hdev);
-
-	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
 
-	new_settings(hdev, match.sk);
+		/* In case privacy is enabled we can now try to enable
+		 * advertising as this was not done before due to SMP not
+		 * registered.
+		 */
+		if (hci_dev_test_flag(hdev, HCI_PRIVACY)) {
+			struct hci_request req;
 
-	hci_dev_unlock(hdev);
+			hci_req_init(&req, hdev);
+			powered_enable_advertising(hdev, &req);
+			if (!hci_req_run(&req, powered_adv_complete)) {
+				return;
+			}
+		}
+	}
 
-	if (match.sk)
-		sock_put(match.sk);
+	mgmt_set_powered_complete(hdev);
 }
 
 static int powered_update_hci(struct hci_dev *hdev)
@@ -7322,12 +7359,13 @@ static int powered_update_hci(struct hci_dev *hdev)
 			hdev->cur_adv_instance = adv_instance->instance;
 		}
 
-		if (hci_dev_test_flag(hdev, HCI_ADVERTISING))
-			enable_advertising(&req);
-		else if (hci_dev_test_flag(hdev, HCI_ADVERTISING_INSTANCE) &&
-			 hdev->cur_adv_instance)
-			schedule_adv_instance(&req, hdev->cur_adv_instance,
-					      true);
+		/* Only enable advertising now if privacy is not enabled,
+		 * otherwise it will fail since SMP is not yet registered - in
+		 * such case advertising will be enabled later.
+		 */
+		if (!hci_dev_test_flag(hdev, HCI_PRIVACY)) {
+			powered_enable_advertising(hdev, &req);
+		}
 	}
 
 	link_sec = hci_dev_test_flag(hdev, HCI_LINK_SECURITY);
-- 
2.6.2

[PATCH] tools/mgmt-tester: Add power on testcase with adv and privacy

[Andrzej Kaczmarek]

This test verifies if kernel can properly enable advertising during
power on with privacy also enabled. This requires to generate new RPA
which will fail if not done after SMP is registered. As a consequence
advertising is not enabled in controller which means it cannot be also
disabled (controller will reject HCI command).
---
 tools/mgmt-tester.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
index b224543..670c580 100644
--- a/tools/mgmt-tester.c
+++ b/tools/mgmt-tester.c
@@ -584,6 +584,22 @@ static const struct generic_data set_powered_on_invalid_index_test = {
 	.expect_status = MGMT_STATUS_INVALID_INDEX,
 };
 
+static uint16_t settings_powered_advertising_privacy[] = {
+						MGMT_OP_SET_PRIVACY,
+						MGMT_OP_SET_ADVERTISING,
+						MGMT_OP_SET_POWERED, 0 };
+
+static const char set_adv_off_param[] = { 0x00 };
+
+static const struct generic_data set_powered_on_privacy_adv_test = {
+	.setup_settings = settings_powered_advertising_privacy,
+	.send_opcode = MGMT_OP_SET_ADVERTISING,
+	.send_param = set_adv_off_param,
+	.send_len = sizeof(set_adv_off_param),
+	.expect_status = MGMT_STATUS_SUCCESS,
+	.expect_ignore_param = true,
+};
+
 static const uint16_t settings_powered[] = { MGMT_OP_SET_POWERED, 0 };
 
 static const char set_powered_off_param[] = { 0x00 };
@@ -4356,8 +4372,6 @@ static const struct generic_data add_advertising_success_4 = {
 	.expect_hci_len = sizeof(set_adv_data_txpwr),
 };
 
-static const char set_adv_off_param[] = { 0x00 };
-
 static const struct generic_data add_advertising_success_5 = {
 	.send_opcode = MGMT_OP_SET_ADVERTISING,
 	.send_param = set_adv_off_param,
@@ -5566,6 +5580,9 @@ proceed:
 	for (cmd = test->setup_settings; *cmd; cmd++) {
 		unsigned char simple_param[] = { 0x01 };
 		unsigned char discov_param[] = { 0x01, 0x00, 0x00 };
+		unsigned char privacy_param[] = { 0x01,
+			0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
+			0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08 };
 		unsigned char *param = simple_param;
 		size_t param_size = sizeof(simple_param);
 		mgmt_request_func_t func = NULL;
@@ -5584,6 +5601,11 @@ proceed:
 			param = discov_param;
 		}
 
+		if (*cmd == MGMT_OP_SET_PRIVACY) {
+			param_size = sizeof(privacy_param);
+			param = privacy_param;
+		}
+
 		if (*cmd == MGMT_OP_SET_LE && test->setup_nobredr) {
 			unsigned char off[] = { 0x00 };
 			mgmt_send(data->mgmt, *cmd, data->mgmt_index,
@@ -6075,6 +6097,9 @@ int main(int argc, char *argv[])
 	test_bredrle("Set powered on - Invalid index",
 				&set_powered_on_invalid_index_test,
 				NULL, test_command_generic);
+	test_le("Set powered on - Privacy and Advertising",
+				&set_powered_on_privacy_adv_test,
+				NULL, test_command_generic);
 
 	test_bredrle("Set powered off - Success",
 				&set_powered_off_success_test,
-- 
2.6.2

Re: [PATCH] Bluetooth: Fix powering on with privacy and advertising

[Johan Hedberg]

Hi Andrzej,

On Thu, Nov 12, 2015, Andrzej Kaczmarek wrote:
> In order to enable advertising with privacy enabled, SMP has to be
> registered in order to generate new RPA. During power on, it will be
> registered at the very end which is the reason why advertising is not
> enabled and it's not possible to enable it anymore due to mismatch
> between hci_dev settings and actual controller state.
> 
> This fixes this problem by postponing advertising enable after SMP is
> registered in case of power on.
> ---
>  net/bluetooth/mgmt.c | 70 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 16 deletions(-)

Instead of postponing advertising, did you consider simply moving
smp_register() earlier? Wouldn't it be possible to move it to
mgmt_powered() before powered_update_hci() is called?

Johan

Re: [PATCH] Bluetooth: Fix powering on with privacy and advertising

[Andrzej Kaczmarek]

Hi Johan,

On Fri, Nov 13, 2015 at 12:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Andrzej,
>
> On Thu, Nov 12, 2015, Andrzej Kaczmarek wrote:
>> In order to enable advertising with privacy enabled, SMP has to be
>> registered in order to generate new RPA. During power on, it will be
>> registered at the very end which is the reason why advertising is not
>> enabled and it's not possible to enable it anymore due to mismatch
>> between hci_dev settings and actual controller state.
>>
>> This fixes this problem by postponing advertising enable after SMP is
>> registered in case of power on.
>> ---
>>  net/bluetooth/mgmt.c | 70 ++++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 54 insertions(+), 16 deletions(-)
>
> Instead of postponing advertising, did you consider simply moving
> smp_register() earlier? Wouldn't it be possible to move it to
> mgmt_powered() before powered_update_hci() is called?

smp_register() was already moved in 162a3bac8d0 so I assumed this is
the right place for it and didn't want to move it again. But it seems
it could be moved as you suggested, identity address is already know
at that point. And this will make whole fix simple. I'll send v2
later, unless Marcel disagrees with this proposal.

BR,
Andrzej

Re: [PATCH] Bluetooth: Fix powering on with privacy and advertising

[Johan Hedberg]

Hi Andrzej,

On Mon, Nov 16, 2015, Andrzej Kaczmarek wrote:
> On Fri, Nov 13, 2015 at 12:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > Hi Andrzej,
> >
> > On Thu, Nov 12, 2015, Andrzej Kaczmarek wrote:
> >> In order to enable advertising with privacy enabled, SMP has to be
> >> registered in order to generate new RPA. During power on, it will be
> >> registered at the very end which is the reason why advertising is not
> >> enabled and it's not possible to enable it anymore due to mismatch
> >> between hci_dev settings and actual controller state.
> >>
> >> This fixes this problem by postponing advertising enable after SMP is
> >> registered in case of power on.
> >> ---
> >>  net/bluetooth/mgmt.c | 70 ++++++++++++++++++++++++++++++++++++++++------------
> >>  1 file changed, 54 insertions(+), 16 deletions(-)
> >
> > Instead of postponing advertising, did you consider simply moving
> > smp_register() earlier? Wouldn't it be possible to move it to
> > mgmt_powered() before powered_update_hci() is called?
> 
> smp_register() was already moved in 162a3bac8d0 so I assumed this is
> the right place for it and didn't want to move it again. But it seems
> it could be moved as you suggested, identity address is already know
> at that point. And this will make whole fix simple. I'll send v2
> later, unless Marcel disagrees with this proposal.

The place it was originally moved from (__hci_init) had no chance of
knowing the controller features that are needed to determine static
random vs public address. However, in mgmt_powered() the main HCI init
has already completed and this information should be available. There
are no HCI commands in powered_update_hci() that would provide
additional useful information for this decision.

Johan

Re: [PATCH] tools/mgmt-tester: Add power on testcase with adv and privacy

[Szymon Janc]

Hi Andrzej,

On Thursday 12 November 2015 21:43:07 Andrzej Kaczmarek wrote:
> This test verifies if kernel can properly enable advertising during
> power on with privacy also enabled. This requires to generate new RPA
> which will fail if not done after SMP is registered. As a consequence
> advertising is not enabled in controller which means it cannot be also
> disabled (controller will reject HCI command).
> ---
>  tools/mgmt-tester.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
> index b224543..670c580 100644
> --- a/tools/mgmt-tester.c
> +++ b/tools/mgmt-tester.c
> @@ -584,6 +584,22 @@ static const struct generic_data
> set_powered_on_invalid_index_test = { .expect_status =
> MGMT_STATUS_INVALID_INDEX,
>  };
> 
> +static uint16_t settings_powered_advertising_privacy[] = {
> +						MGMT_OP_SET_PRIVACY,
> +						MGMT_OP_SET_ADVERTISING,
> +						MGMT_OP_SET_POWERED, 0 };
> +
> +static const char set_adv_off_param[] = { 0x00 };
> +
> +static const struct generic_data set_powered_on_privacy_adv_test = {
> +	.setup_settings = settings_powered_advertising_privacy,
> +	.send_opcode = MGMT_OP_SET_ADVERTISING,
> +	.send_param = set_adv_off_param,
> +	.send_len = sizeof(set_adv_off_param),
> +	.expect_status = MGMT_STATUS_SUCCESS,
> +	.expect_ignore_param = true,
> +};
> +
>  static const uint16_t settings_powered[] = { MGMT_OP_SET_POWERED, 0 };
> 
>  static const char set_powered_off_param[] = { 0x00 };
> @@ -4356,8 +4372,6 @@ static const struct generic_data
> add_advertising_success_4 = { .expect_hci_len = sizeof(set_adv_data_txpwr),
>  };
> 
> -static const char set_adv_off_param[] = { 0x00 };
> -
>  static const struct generic_data add_advertising_success_5 = {
>  	.send_opcode = MGMT_OP_SET_ADVERTISING,
>  	.send_param = set_adv_off_param,
> @@ -5566,6 +5580,9 @@ proceed:
>  	for (cmd = test->setup_settings; *cmd; cmd++) {
>  		unsigned char simple_param[] = { 0x01 };
>  		unsigned char discov_param[] = { 0x01, 0x00, 0x00 };
> +		unsigned char privacy_param[] = { 0x01,
> +			0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
> +			0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08 };
>  		unsigned char *param = simple_param;
>  		size_t param_size = sizeof(simple_param);
>  		mgmt_request_func_t func = NULL;
> @@ -5584,6 +5601,11 @@ proceed:
>  			param = discov_param;
>  		}
> 
> +		if (*cmd == MGMT_OP_SET_PRIVACY) {
> +			param_size = sizeof(privacy_param);
> +			param = privacy_param;
> +		}
> +
>  		if (*cmd == MGMT_OP_SET_LE && test->setup_nobredr) {
>  			unsigned char off[] = { 0x00 };
>  			mgmt_send(data->mgmt, *cmd, data->mgmt_index,
> @@ -6075,6 +6097,9 @@ int main(int argc, char *argv[])
>  	test_bredrle("Set powered on - Invalid index",
>  				&set_powered_on_invalid_index_test,
>  				NULL, test_command_generic);
> +	test_le("Set powered on - Privacy and Advertising",
> +				&set_powered_on_privacy_adv_test,
> +				NULL, test_command_generic);
> 
>  	test_bredrle("Set powered off - Success",
>  				&set_powered_off_success_test,

This patch is now applied, thanks.

-- 
pozdrawiam
Szymon Janc