Skip to content

Conversation

petrip-slab
Copy link
Contributor

Added three functions to support the EAD peripheral:
- Setting and storing key materials for EAD
- Encrypting data for EAD advertisements
- Decrypting data from EAD advertisements

This is new auto-pts command needed to encrypted adv qualification

@zephyrbot zephyrbot added area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Tests Issues related to a particular existing or missing test area: Bluetooth labels Oct 7, 2025
@Thalley Thalley requested a review from alwa-nordic October 7, 2025 11:11
@Thalley
Copy link
Contributor

Thalley commented Oct 7, 2025

Reference to the autopts PR: auto-pts/auto-pts#1539

@Thalley Thalley requested a review from Copilot October 7, 2025 11:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for Encrypted Advertising Data (EAD) peripheral functionality to the Bluetooth tester, enabling encrypted advertisement qualification tests for auto-PTS.

  • Added three new BTP GAP commands for EAD key material management, data encryption, and data decryption
  • Implemented functions to handle EAD operations including key storage, encryption, and decryption of advertisement data
  • Added corresponding BTP protocol definitions and command structures for EAD operations

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/bluetooth/tester/src/btp_gap.c Implements EAD functionality with key material storage and encrypt/decrypt functions
tests/bluetooth/tester/src/btp/btp_gap.h Defines BTP protocol structures and opcodes for EAD commands

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@petrip-slab petrip-slab force-pushed the encrypted_adv_for_tester branch from c82c748 to 6919db6 Compare October 8, 2025 08:05
Copy link
Contributor

@Thalley Thalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks OK to me with some formatting and code style comments

@@ -389,6 +389,7 @@ struct btp_gap_bis_broadcast_cmd {
} __packed;

#define BTP_GAP_SET_RPA_TIMEOUT 0x30

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this extra line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed idI don't I have done anything there. That empty line been there quite a while. Abd seen to separate two commands

a7eeea1 (Lyle Zhu 2025-07-22 18:17:00 +0800 389) } __packed;
a7eeea1 (Lyle Zhu 2025-07-22 18:17:00 +0800 390)
c8e28d1 (Evgenii Kosenko 2025-02-19 13:55:36 +0200 391) #define BTP_GAP_SET_RPA_TIMEOUT 0x30

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new commands should be added here

#define BTP_GAP_EAD_DECRYPT_ADV_DATA 0x33

#define BTP_GAP_EAD_KEY_SIZE 16
#define BTP_GAP_EAD_IV_SIZE 8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typically we name flags with command prefix, ie here it would be
BTP_GAP_EAD_SET_KEY_MATERIAL_KEY_SIZE
BTP_GAP_EAD_SET_KEY_MATERIAL_IV_SIZE

same for structure names (cmd and rp)
struct btp_gap_ead_encrypt_adv_data_cmd
struct btp_gap_ead_decrypt_adv_data_cmd

#if defined(CONFIG_BT_EAD)
{
.opcode = BTP_GAP_EAD_SET_KEY_MATERIAL,
.expect_len = BTP_HANDLER_LENGTH_VARIABLE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this command has fixed size

@Thalley
Copy link
Contributor

Thalley commented Oct 8, 2025

We do actually also have self-tests of BTP and the BT Tester in BSIM now - Might be a good idea to add testing of these commands in a BSIM test

@petrip-slab petrip-slab force-pushed the encrypted_adv_for_tester branch 2 times, most recently from 3507839 to 27202db Compare October 8, 2025 10:45
Copy link
Contributor

@Thalley Thalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the guard in the header file :)

Comment on lines +617 to +618
} __packed;

struct btp_gap_ead_encrypt_adv_data_rp {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} __packed;
struct btp_gap_ead_encrypt_adv_data_rp {
} __packed;
struct btp_gap_ead_encrypt_adv_data_rp {

@Thalley Thalley requested a review from Copilot October 8, 2025 11:40
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@petrip-slab petrip-slab force-pushed the encrypted_adv_for_tester branch from 27202db to c316c95 Compare October 9, 2025 08:03
@petrip-slab petrip-slab requested a review from Thalley October 9, 2025 08:03
@petrip-slab
Copy link
Contributor Author

I will do the bsim test in separate pull request. Saves trouble of retesting all the time but takes so study to do them properly.

@@ -599,3 +599,34 @@ int tester_gap_padv_stop(struct bt_le_ext_adv *ext_adv);
int tester_gap_padv_create_sync(struct bt_le_per_adv_sync_param *create_params);
int tester_gap_padv_stop_sync(void);
#endif /* defined(CONFIG_BT_EXT_ADV) */

#define BTP_GAP_EAD_SET_KEY_MATERIAL_KEY_SIZE 16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

al this should be next to other commands definition

return BTP_STATUS_FAILED;
}

err = bt_ead_encrypt(ead_key_materials.session_key, ead_key_materials.initialization_vector,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 * @param[out] encrypted_payload Encrypted Ad Data including the Randomizer and
 *             the MIC. Size must be at least @ref BT_EAD_RANDOMIZER_SIZE + @p
 *             payload_size + @ref BT_EAD_MIC_SIZE. Use @ref
 *             BT_EAD_ENCRYPTED_PAYLOAD_SIZE to get the right size.

Should this also be validated ? if if BT_EAD_ENCRYPTED_PAYLOAD_SIZE(cp->adv_data_len) would fit in response? to avoid invalid memory writes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same, but figured that with ADV sizes of max 254 and the current MTU of 1024, this wouldn't be an issue, but Ideally we'd ensure that the response size is something like BT_EAD_ENCRYPTED_PAYLOAD_SIZE(UINT8_MAX) if EAD is enabled (can be done via a a BUILD_ASSERT)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a check on getting in the plain text after verifying that size of message is correct. Maximum seems to be 246

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maximum seems to be 246

Oh yeah, that makes sense, since the encrypted advertising data + MAC needs to be in the LTV

    Added three functions to support the EAD peripheral:
    - Setting and storing key materials for EAD
    - Encrypting data for EAD advertisements
    - Decrypting data from EAD advertisements

Signed-off-by: Petri Pitkanen <petri.pitkanen@silabs.com>
@petrip-slab petrip-slab force-pushed the encrypted_adv_for_tester branch from c316c95 to 1794b7e Compare October 9, 2025 10:30
Copy link

sonarqubecloud bot commented Oct 9, 2025

Copy link
Contributor

@sjanc sjanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move commands definition to proper place in btp.h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Bluetooth area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants