BLE: Add privacy trace #14127

pan- · 2021-01-07T16:53:13Z

Summary of changes

Add traces to the privacy controller and its associated PAL.
Traces of the host resolution operation are set at the debug level to not clutter the output.

Preceding PR: #14118

Impact of changes

Migration actions required

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

- Prefix with Connection <id> - when appropriate. - Display parameters after `:`. - If multiple parameters should be displayed name then and print the value after =. They are separated by a `,`.

Only the first 8 bytes were generated from the stack.

Address resolution is set at the debug level as it is a very common operation and may clutter the output.

paul-szczepanek-arm · 2021-01-08T10:06:29Z

connectivity/FEATURE_BLE/source/cordio/source/PalPrivateAddressControllerImpl.cpp

        return BLE_ERROR_NOT_IMPLEMENTED;
    }
+
+    tr_info("Add RPA to LL resolving list: peer address=%s, type=%s, peer irk=%s, local irk=%s",
+        to_string(peer_address_type), to_string(peer_identity_address), to_string(peer_irk), to_string(local_irk));


address and type swapped

Suggested change

to_string(peer_address_type), to_string(peer_identity_address), to_string(peer_irk), to_string(local_irk));

to_string(peer_identity_address), to_string(peer_address_type), to_string(peer_irk), to_string(local_irk));

paul-szczepanek-arm · 2021-01-08T10:34:04Z

connectivity/FEATURE_BLE/source/cordio/source/PalPrivateAddressControllerImpl.cpp

        return BLE_ERROR_NOT_IMPLEMENTED;
    }

+    tr_info("Remove RPA from LL resolving list: peer address=%s, type=%s",
+        to_string(peer_address_type), to_string(peer_identity_address));


Suggested change

to_string(peer_address_type), to_string(peer_identity_address));

to_string(peer_identity_address), to_string(peer_address_type));

paul-szczepanek-arm · 2021-01-08T10:36:11Z

connectivity/FEATURE_BLE/source/cordio/source/PalPrivateAddressControllerImpl.cpp

+#include "mbed-trace/mbed_trace.h"
+#include "common/ble_trace_helpers.h"
+
+#define TRACE_GROUP "BLSM"


maybe it's worth using a different tag: BLPR?

paul-szczepanek-arm · 2021-01-08T10:39:03Z

connectivity/FEATURE_BLE/source/cordio/source/PalSecurityManagerImpl.cpp

@@ -444,6 +482,7 @@ ble_error_t PalSecurityManager::cancel_pairing(
 ble_error_t PalSecurityManager::get_random_data(byte_array_t<8> &random_data)
 {
    SecRand(random_data.data(), random_data.size());
+    tr_info("Stack generated random data: %s", to_string(random_data));


don't think this is useful, if the data is relevant the caller will log it

That still useful, the caller will log things like set CSRK: ..... It doesn't say much about the origin of the bytes.
I'd keep it, it help me in identifying the issue fixed by #14125

paul-szczepanek-arm · 2021-01-08T10:42:11Z

connectivity/FEATURE_BLE/source/cordio/source/PalSecurityManagerImpl.cpp

@@ -697,6 +752,7 @@ bool PalSecurityManager::sm_handler(const wsfMsgHdr_t *msg)
        }

        case DM_SEC_LTK_REQ_IND: {
+            tr_info("Handling event DM_SEC_LTK_REQ_IND");


traces stop here, but there are a few more events below

Thanks, I don't know what I was thinking.

paul-szczepanek-arm · 2021-01-08T10:46:50Z

connectivity/FEATURE_BLE/source/generic/PrivateAddressController.cpp

@@ -340,6 +370,7 @@ ble_error_t PrivateAddressController::resolve_address_on_host(

 void PrivateAddressController::on_resolving_list_action_complete()
 {
+    tr_info("Resolving list action conpleted");


Suggested change

tr_info("Resolving list action conpleted");

tr_info("Resolving list action completed");

paul-szczepanek-arm · 2021-01-08T11:11:38Z

connectivity/FEATURE_BLE/source/generic/SecurityManagerImpl.cpp

-    if (!_db) return BLE_ERROR_INITIALIZATION_INCOMPLETE;
+    tr_info("Connection %d - Accept pairing request", connection);
+    if (!_db) {
+        tr_error("Failure, db not initialized");


So it can be collapsed with the rest of them

Suggested change

tr_error("Failure, db not initialized");

tr_error("Failure, DB not initialized");

I guess same below, some copy pasting would solve this to avoid accidental differences in lines.

paul-szczepanek-arm · 2021-01-08T11:16:34Z

connectivity/FEATURE_BLE/source/generic/SecurityManagerImpl.cpp

        return;
    }

    SecurityDistributionFlags_t* flags = _db->get_distribution_flags(cb->db_entry);
    if (!flags) {
+        tr_error("Connection %d - Cannot enable encryption, no distribution flags found for entry %p", cb->connection, cb->db_entry);


honestly, if you're fixing things, this whole if statement should just be an MBED_ASSERT(flags);

Same below and other instances

The fix would be to validate the DB when it is loaded. I'm not doing it now.

paul-szczepanek-arm · 2021-01-08T11:25:00Z

connectivity/FEATURE_BLE/source/generic/SecurityManagerImpl.cpp

+    PasskeyAscii ascii_passkey(passkey);
+    tr_info("Connection %d - Received passkey display %.*s",
+        connection, ascii_passkey.PASSKEY_LEN, ascii_passkey.value());
+    eventHandler->passkeyDisplay(connection, ascii_passkey.value());


should we null check eventHandler? you added the check to other places

and another one below

paul-szczepanek-arm · 2021-01-08T11:26:31Z

connectivity/FEATURE_BLE/source/generic/SecurityManagerImpl.cpp

    set_mitm_performed(connection);
+    if (!eventHandler) {
+        tr_error("Impossible to forward Keypress notification to application, no event handler registered");


the call below proceeds, needs to be in else block

paul-szczepanek-arm

we should just merge and I'll make a separate PR to fix the issues listed above

- Put privacy traces in BLPR - Add missing traces in PALSecurityManagerImpl.cpp - Add missing EventHandler null pointer check - Typo and parameters order fix.

pan- · 2021-01-08T11:59:30Z

Thanks @paul-szczepanek-arm . I addressed your changes.

Add traces to the Bluetooth Security Manager and Privacy controller. The traces are made to be comprehensive to improve the ux when users submit bugs. A centralized set of helpers has been created to convert BLE types to string. * BLE: Add SM traces * BLE: Add traces to Security Manager PAL * BLE: Make SM traces consistent. - Prefix with Connection <id> - when appropriate. - Display parameters after `:`. - If multiple parameters should be displayed name then and print the value after =. They are separated by a `,`. * BLE: Fix SM random number generation. Only the first 8 bytes were generated from the stack. * BLE: Regenerate CSRK if it is all zeroes. * BLE: Add trace into privacy modules Address resolution is set at the debug level as it is a very common operation and may clutter the output. * Address trace review for SM: - Put privacy traces in BLPR - Add missing traces in PALSecurityManagerImpl.cpp - Add missing EventHandler null pointer check - Typo and parameters order fix.

pan- added 8 commits January 6, 2021 16:20

WIP: SM Bluetooth traces

45b4ee7

BLE: Move traces helper into their own file

fde273e

BLE: Move traces helper into their own file

860f21b

BLE: Add traces to Security Manager PAL

e50adec

BLE: Make SM traces consistent.

73ca2ba

- Prefix with Connection <id> - when appropriate. - Display parameters after `:`. - If multiple parameters should be displayed name then and print the value after =. They are separated by a `,`.

BLE: Fix SM random number generation.

1648061

Only the first 8 bytes were generated from the stack.

BLE: Regenerate CSRK if it is all zeroes.

bab5bb3

BLE: Add trace into privacy modules

580b44a

Address resolution is set at the debug level as it is a very common operation and may clutter the output.

pan- added component: ble needs: preceding PR labels Jan 7, 2021

pan- requested review from paul-szczepanek-arm and a user January 7, 2021 16:53

mergify bot added the release-version: feature branch label Jan 7, 2021

paul-szczepanek-arm reviewed Jan 8, 2021

View reviewed changes

paul-szczepanek-arm approved these changes Jan 8, 2021

View reviewed changes

Address trace review for SM:

30a2355

- Put privacy traces in BLPR - Add missing traces in PALSecurityManagerImpl.cpp - Add missing EventHandler null pointer check - Typo and parameters order fix.

pan- merged commit fd3a4a7 into ARMmbed:feature-bluetooth-traces Jan 8, 2021

	to_string(peer_address_type), to_string(peer_identity_address), to_string(peer_irk), to_string(local_irk));
	to_string(peer_identity_address), to_string(peer_address_type), to_string(peer_irk), to_string(local_irk));

	tr_info("Resolving list action conpleted");
	tr_info("Resolving list action completed");

	tr_error("Failure, db not initialized");
	tr_error("Failure, DB not initialized");

BLE: Add privacy trace #14127

BLE: Add privacy trace #14127

Uh oh!

Conversation

pan- commented Jan 7, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm left a comment

Choose a reason for hiding this comment

Uh oh!

pan- commented Jan 8, 2021

Uh oh!

Uh oh!

paul-szczepanek-arm Jan 8, 2021 •

edited

Loading