Bluetooth: Host: Fix not clearing IDs and keys upon bt_disable() #72680

rugeGerritsen · 2024-05-13T11:57:06Z

Bluetooth: Host: Fix not clearing IDs and keys upon bt_disable()

Expectation: After calling `bt_disable()` it is possible to
use the Bluetooth APIs as if `bt_enable()` was never called.

This was not the case for `bt_id_create()`, it was not possible
to set the default identity. This prevented an application
developer to restart the stack as a different identity.

Keys also need to be cleared to avoid the following pattern:
1. Pair two devices
2. Central calls `bt_disable()` and `bt_enable()`.
   The central will now generate a new identity address.
3. Connect the two devices.
4. Re-establish encryption. Now the central will try to use
   the previously used keys. The procedure will fail
   because the peripheral does not have any keys associated
   with the new central address.

The API documentation is updated accordingly.

tests/bsim/bluetooth/host/misc/disable/tests_scripts/disable_set_default_id.sh

include/zephyr/bluetooth/bluetooth.h

subsys/bluetooth/host/hci_core.c

cvinayak · 2024-05-14T10:20:35Z

tests/bsim/bluetooth/host/misc/disable/src/main_disable.c

@@ -35,6 +35,55 @@ static void test_disable_main(void)
 	PASS("Disable test passed\n");
 }

+static void test_disable_set_default_id(void)
+{


Suggested change

{

{

/* Provide some idle time for boot sequence to complete.

* Otherwise, it is observed that nRF POWER_CLOCK ISR does not get serviced.

*/

k_sleep(K_MSEC(1));

@aescolar any comments, why some test case work without this hack?

Not really. I would need to dig into why that is happening, but I don't have time now. It would be nice if you or Emil did.

@Thalley you are facing controller assertions, is similar workaround needed for your tests too?

No, I haven't seen it for the bt_disable tests I'm implementing right now (#72690)

I see you already have couple k_sleep() calls that would allow ISRs to be vectored into.

I added a temporary workaround. #73342 fixes the bug in the controller

tests/bsim/bluetooth/host/misc/disable/src/main_disable.c

aescolar · 2024-05-27T12:48:07Z

tests/bsim/bluetooth/host/misc/disable/src/main_disable.c

+	/* BSIM test framework requires at to run some time before marking a
+	 * test case as passed.
+	 */
+	k_sleep(K_MSEC(1));


@Thalley it is because of this: 919d27b#r142428001

rugeGerritsen · 2024-05-27T14:33:26Z

The new CIS disable test is failing because bt_disable() doesn't clear the stored keys. As a result, the central uses the previously stored keys when re-establishing security. As the device uses a new identity, this fails. We need to figure out if we want keys to be cleared as well.

Thalley · 2024-05-27T15:29:12Z

The new CIS disable test is failing because bt_disable() doesn't clear the stored keys. As a result, the central uses the previously stored keys when re-establishing security. As the device uses a new identity, this fails. We need to figure out if we want keys to be cleared as well.

If we are clearing stuff like IRKs etc. (which I believe we are/should when doing an HCI reset), then we should clear all the other keys as well, unless they are store in BT_SETTINGS.

cvinayak · 2024-05-27T20:05:38Z

tests/bsim/bluetooth/host/misc/disable/src/main_disable.c

+		/* FIXME: Temporary workaround to get around a bug in the controller
+		 * Open PR: https://github.com/zephyrproject-rtos/zephyr/pull/73342
+		 */
+		k_sleep(K_MSEC(1));


Describe the symptom of the bug so it can be reproducible. As I see, it is the POWER_CLOCK_IRQn that is not executed if a test is developed without any cpu_idle() call.

The workaround to sleep is only needed once, not needed in a loop. See the original suggestion here: https://github.com/zephyrproject-rtos/zephyr/pull/72680/files#r1599770565

Updated the description. Hopefully it can be removed soon. The linked PR should give enough information

Thalley · 2024-05-28T07:21:14Z

subsys/bluetooth/host/hci_core.c

+	bt_dev.le.rl_entries = 0;
+	bt_keys_reset();


Should bt_dev.le.rl_entries = 0; have been part of bt_keys_reset?

Or should we have a bt_id_reset that does this?

Good point. I will leave that up to you and @jori-nordic

@rugeGerritsen let's merge as-is and follow-up with another one if necessary. id.c is "here be dragons" territory.

Expectation: After calling `bt_disable()` it is possible to use the Bluetooth APIs as if `bt_enable()` was never called. This was not the case for `bt_id_create()`, it was not possible to set the default identity. This prevented an application developer to restart the stack as a different identity. Keys also need to be cleared to avoid the following pattern: 1. Pair two devices 2. Central calls `bt_disable()` and `bt_enable()`. The central will now generate a new identity address. 3. Connect the two devices. 4. Re-establish encryption. Now the central will try to use the previously used keys. The procedure will fail because the peripheral does not have any keys associated with the new central address. The API documentation is updated accordingly. Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>

rugeGerritsen · 2024-05-28T07:52:54Z

Sorry for the last pushes. I forgot to include the change in the prj.conf which I had locally

Requests addressed.

zephyrbot added area: Bluetooth platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim area: Bluetooth Host labels May 13, 2024

zephyrbot requested review from aescolar, alwa-nordic, asbjornsabo, hermabe, jhedberg, jori-nordic, sjanc, Thalley, theob-pro and Vudentz May 13, 2024 11:57

zephyrbot assigned jori-nordic May 13, 2024

aescolar reviewed May 13, 2024

View reviewed changes

tests/bsim/bluetooth/host/misc/disable/tests_scripts/disable_set_default_id.sh Outdated Show resolved Hide resolved

rugeGerritsen force-pushed the fix-bt-disable-id-count branch from 9d8ef17 to b8e00c0 Compare May 13, 2024 12:01

Thalley requested changes May 13, 2024

View reviewed changes

include/zephyr/bluetooth/bluetooth.h Outdated Show resolved Hide resolved

rugeGerritsen force-pushed the fix-bt-disable-id-count branch from b8e00c0 to 45c80b1 Compare May 13, 2024 13:15

rugeGerritsen requested a review from Thalley May 13, 2024 13:15

jori-nordic previously approved these changes May 13, 2024

View reviewed changes

Thalley previously approved these changes May 13, 2024

View reviewed changes

MarekPieta reviewed May 14, 2024

View reviewed changes

subsys/bluetooth/host/hci_core.c Show resolved Hide resolved

cvinayak requested changes May 14, 2024

View reviewed changes

rugeGerritsen dismissed stale reviews from Thalley and jori-nordic via a792cde May 27, 2024 11:07

rugeGerritsen force-pushed the fix-bt-disable-id-count branch from 45c80b1 to a792cde Compare May 27, 2024 11:07

rugeGerritsen requested a review from cvinayak May 27, 2024 11:09

rugeGerritsen commented May 27, 2024

View reviewed changes

tests/bsim/bluetooth/host/misc/disable/src/main_disable.c Outdated Show resolved Hide resolved

rugeGerritsen force-pushed the fix-bt-disable-id-count branch 2 times, most recently from 70c6ad8 to c11681c Compare May 27, 2024 12:18

aescolar reviewed May 27, 2024

View reviewed changes

cvinayak previously requested changes May 27, 2024

View reviewed changes

jori-nordic previously approved these changes May 28, 2024

View reviewed changes

rugeGerritsen dismissed jori-nordic’s stale review via b5450a2 May 28, 2024 07:02

rugeGerritsen force-pushed the fix-bt-disable-id-count branch from c11681c to b5450a2 Compare May 28, 2024 07:02

rugeGerritsen changed the title ~~Bluetooth: Host: Fix not clearing IDs upon bt_disable()~~ Bluetooth: Host: Fix not clearing IDs and keys upon bt_disable() May 28, 2024

rugeGerritsen requested a review from cvinayak May 28, 2024 07:03

rugeGerritsen force-pushed the fix-bt-disable-id-count branch from b5450a2 to 323adb4 Compare May 28, 2024 07:12

jori-nordic previously approved these changes May 28, 2024

View reviewed changes

Thalley previously approved these changes May 28, 2024

View reviewed changes

rugeGerritsen dismissed stale reviews from Thalley and jori-nordic via 9bd0e01 May 28, 2024 07:51

rugeGerritsen force-pushed the fix-bt-disable-id-count branch from 323adb4 to 9bd0e01 Compare May 28, 2024 07:51

rugeGerritsen force-pushed the fix-bt-disable-id-count branch from 9bd0e01 to a8ae1bf Compare May 28, 2024 07:52

Thalley approved these changes May 28, 2024

View reviewed changes

jori-nordic approved these changes May 28, 2024

View reviewed changes

carlescufi merged commit 3ce106c into zephyrproject-rtos:main May 28, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Host: Fix not clearing IDs and keys upon bt_disable() #72680

Bluetooth: Host: Fix not clearing IDs and keys upon bt_disable() #72680

rugeGerritsen commented May 13, 2024 •

edited

cvinayak May 14, 2024

cvinayak May 14, 2024

aescolar May 14, 2024

cvinayak May 15, 2024

Thalley May 15, 2024 •

edited

cvinayak May 16, 2024

rugeGerritsen May 27, 2024

aescolar May 27, 2024 •

edited

rugeGerritsen commented May 27, 2024

Thalley commented May 27, 2024

cvinayak May 27, 2024

rugeGerritsen May 28, 2024

Thalley May 28, 2024

Thalley May 28, 2024

rugeGerritsen May 28, 2024

jori-nordic May 28, 2024

Thalley May 28, 2024

rugeGerritsen commented May 28, 2024

-{
+{
+	/* Provide some idle time for boot sequence to complete.
+	 * Otherwise, it is observed that nRF POWER_CLOCK ISR does not get serviced.
+	 */
+	 k_sleep(K_MSEC(1));

Bluetooth: Host: Fix not clearing IDs and keys upon bt_disable() #72680

Bluetooth: Host: Fix not clearing IDs and keys upon bt_disable() #72680

Conversation

rugeGerritsen commented May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aescolar May 27, 2024 • edited

Choose a reason for hiding this comment

rugeGerritsen commented May 27, 2024

Thalley commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugeGerritsen commented May 28, 2024

rugeGerritsen commented May 13, 2024 •

edited

Thalley May 15, 2024 •

edited

aescolar May 27, 2024 •

edited