Skip to content

Conversation

@jhedberg
Copy link
Member

@jhedberg jhedberg commented Jan 20, 2025

Remove the HCI command & event emulation layer for ECDH commands and events. This means that we always do the necessary operations in the host. The existing BT_ECC Kconfig option stays, but now gets automatically enabled when necessary (e.g. based on the BT_SMP option), which is why this PR removes so many explicit assignments in prj.conf files.

@jhedberg jhedberg force-pushed the remove_hci_ecc branch 3 times, most recently from 14321fa to 43c5f19 Compare January 21, 2025 08:41
@jhedberg jhedberg marked this pull request as ready for review January 21, 2025 08:41
@zephyrbot zephyrbot added area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests platform: nRF Nordic nRFx area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Samples Samples area: Bluetooth Audio area: Bluetooth Release Notes To be mentioned in the release notes area: Bluetooth Controller area: Bluetooth HCI Bluetooth HCI Driver labels Jan 21, 2025
@jhedberg jhedberg self-assigned this Jan 21, 2025
Remove the HCI command & event emulation layer for ECDH commands and
events. This means that we always do the necessary operations in the host.
The existing BT_ECC Kconfig option stays, but now gets automatically
enabled when necessary (e.g. based on the BT_SMP option), which is why this
commit removes so many explicit assignments in prj.conf files.

Signed-off-by: Johan Hedberg <[email protected]>
This option only exposes internal APIs, so there should be no need to allow
applications to set an explicit value. Instead, users of the API should
select it through Kconfig.

Signed-off-by: Johan Hedberg <[email protected]>
Mention the removed prompt for BT_ECC in the migration guide, and also add
a note about the removed HCI emulation layer to the release notes.

Signed-off-by: Johan Hedberg <[email protected]>
@alxelax
Copy link
Contributor

alxelax commented Jan 22, 2025

Shouldn't Host ECC functionality instantiate multiple DH key pairs? Due to single pair, it is not possible to use Host implementation in Mesh when Mesh and SMP work in parallel (for example in DFU).
At the moment we have to keep two ECC implementations, one in Mesh and one in Host.

@kartben kartben removed their assignment Jan 22, 2025
@jhedberg
Copy link
Member Author

Shouldn't Host ECC functionality instantiate multiple DH key pairs? Due to single pair, it is not possible to use Host implementation in Mesh when Mesh and SMP work in parallel (for example in DFU). At the moment we have to keep two ECC implementations, one in Mesh and one in Host.

Yes. This is why I was trying to call out for some requirements gathering in #84216. I think even the Host itself would benefit from being able to handle multiple public/private key pairs, in order to support the multi-identity feature. I think this PR is a good step in fixing these issues, since it simplifies the general implementation in the Host. Once it's in we can look at doing more substantial changes to the API, and since it's internal (for now) we can quite freely mold it to our requirements, both from the Host and Mesh side.

select MBEDTLS if !BUILD_WITH_TFM
select MBEDTLS_PSA_CRYPTO_C if !BUILD_WITH_TFM
select PSA_WANT_ALG_ECDH
select PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE
Copy link
Contributor

@frkv frkv Jan 22, 2025

Choose a reason for hiding this comment

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

Suggested change
select PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE
select PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE
select PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT
select PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_EXPORT

Missing some required key-pair feature types. Please add these

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are not present in the current tree, and this PR doesn't add any new requirements wrt PSA APIs or functionality, so I'm wondering are they really needed or if something else (by chance) happens to provide them:

config BT_SEND_ECC_EMULATION
bool "Emulate ECDH in the Host using PSA Crypto API library"
select MBEDTLS if !BUILD_WITH_TFM
select MBEDTLS_PSA_CRYPTO_C if !BUILD_WITH_TFM
select PSA_WANT_ALG_ECDH
select PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE
select PSA_WANT_ECC_SECP_R1_256
imply MBEDTLS_PSA_P256M_DRIVER_ENABLED if MBEDTLS_PSA_CRYPTO_C
select BT_LONG_WQ
depends on BT_ECC && (BT_HCI_RAW || BT_HCI_HOST)
default y if HAS_BT_CTLR && !BT_CTLR_ECDH
help

As such I could argue that this is really a separate enhancement, independent from this PR, so in that sense it could be done as a follow-up as well (once it's first understood why the current upstream tree seems to work just fine without them).

Copy link
Contributor

Choose a reason for hiding this comment

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

It "works just fine" because Mbed TLS has defined some types without enforcing usage. They have been converting to KEY_PAIR_BASIC and made that the thing that is currently being used in code.

This "just works" right now when using the original Mbed TLS distribution, but it is preventing optimizations that is possible to do for anyone implementing PSA crypto APIs making use of the intent of these configurations.

We make use of these configurations in Nordics own SDK and they are intently added into PSA crypto configurations scope in Zephyr, we are also cooperating with Trusted-Firmware organization (both in TF-M and Mbed TLS) to ensure that build optimizations are part of the final deliverables. These configurations were added for a reason.

Adding them here conforms to the intent, similar to how it is done done in BLE mesh

We will add a subsequent PR with these configurations added :)

@kartben kartben merged commit b3c6151 into zephyrproject-rtos:main Jan 23, 2025
27 checks passed
kartben pushed a commit that referenced this pull request Feb 4, 2025
In #84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
RuibinChang pushed a commit to RuibinChang/zephyr that referenced this pull request Feb 10, 2025
In zephyrproject-rtos#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

(cherry picked from commit 706938d)

Original-Signed-off-by: Sean Madigan <[email protected]>
GitOrigin-RevId: 706938d
Cr-Build-Id: 8723822246285105201
Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8723822246285105201
Copybot-Job-Name: zephyr-main-copybot-downstream
Change-Id: Ic0840ec7b9b2a9510051410509eace6250a41ee4
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/6231798
Tested-by: Jeremy Bettis <[email protected]>
Tested-by: ChromeOS Prod (Robot) <[email protected]>
Commit-Queue: Jeremy Bettis <[email protected]>
Reviewed-by: Jeremy Bettis <[email protected]>
sean-madigan added a commit to sean-madigan/sdk-zephyr that referenced this pull request Feb 13, 2025
In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
sean-madigan added a commit to sean-madigan/sdk-zephyr that referenced this pull request Feb 13, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
m-alperen-sener pushed a commit to m-alperen-sener/sdk-zephyr that referenced this pull request Feb 17, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
rlubos pushed a commit to nrfconnect/sdk-zephyr that referenced this pull request Feb 18, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
m-alperen-sener pushed a commit to m-alperen-sener/sdk-zephyr that referenced this pull request Feb 18, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
m-alperen-sener pushed a commit to m-alperen-sener/sdk-zephyr that referenced this pull request Feb 18, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
theob-pro pushed a commit to theob-pro/sdk-zephyr that referenced this pull request Mar 19, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
bjarki-andreasen pushed a commit to bjarki-andreasen/sdk-zephyr that referenced this pull request Apr 23, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
(cherry picked from commit eb1ae9f)
bjarki-andreasen pushed a commit to bjarki-andreasen/sdk-zephyr that referenced this pull request Apr 23, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
(cherry picked from commit eb1ae9f)
bjarki-andreasen pushed a commit to bjarki-andreasen/sdk-zephyr that referenced this pull request Apr 23, 2025
…BT_HCI_RAW

In zephyrproject-rtos/zephyr#84268
the ability to use the controller for ECDH was removed from
the host.

This means that BT_CTLR_ECDH is now only useful when using
BT_HCI_RAW.

Signed-off-by: Sean Madigan <[email protected]>
(cherry picked from commit 706938d)
(cherry picked from commit eb1ae9f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Audio area: Bluetooth Controller area: Bluetooth HCI Bluetooth HCI Driver area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Bluetooth area: Samples Samples platform: nRF Nordic nRFx Release Notes To be mentioned in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.