lib: modem_key_mgmt: enable CMEE _after_ taking lock #20185

kamnxt · 2025-02-04T14:39:42Z

Wait with enabling CMEE until we have managed to acquire the lock. This prevents the following edge case:

Thread A calls a function that acquires lock before enabling CMEE, like read.
Thread B calls one that has the opposite (wrong?) order, like clear.

Thread A takes lock
Thread A enables CMEE (was_enabled = false)
Thread A happens to get scheduled out
Thread B enables CMEE (was_enabled = true)
Thread B blocks on trying to take lock
Thread A disables CMEE (since was_enabled == false) and releases lock
Thread B takes lock, tries to run command (but CMEE is disabled!)
Thread B doesn't try to disable CMEE since it was enabled before.

I am also wondering if it could make sense to take the mutex when enabling CMEE in the other functions or if the probability of that being a problem is low enough that there is no point?

NordicBuilder · 2025-02-04T14:40:01Z

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

eivindj-nordic · 2025-02-04T14:50:37Z

Hi, and thank you for your contribution!

I am also wondering if it could make sense to take the mutex when enabling CMEE in the other functions or if the probability of that being a problem is low enough that there is no point?

I think it makes sense to do this in the other functions as well!

We should also have a changelog entry for this in the changelog.

NordicBuilder · 2025-02-04T14:57:15Z

You can find the documentation preview for this PR at this link.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

kamnxt · 2025-02-04T15:08:10Z

We should also have a changelog entry for this in the changelog.

Would it make sense to put this under "Security", "Libraries" or "Developing with nRF91"?

eivindj-nordic · 2025-02-04T15:09:43Z

We should also have a changelog entry for this in the changelog.

Would it make sense to put this under "Security", "Libraries" or "Developing with nRF91"?

It should be under modem libraries. There is already an entry there for the modem key mgmt library.

kamnxt · 2025-02-04T15:50:58Z

Hope something like this works?

Also I can try to check with @joerchan since I don't think he added his changes to the changelog 😉

NordicBuilder · 2025-02-04T16:10:52Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 3

Inputs:

Sources:

sdk-nrf: PR head: e69916cc58149e8caf6e283381bb8b17dde6a73b

more details

sdk-nrf:

PR head: e69916cc58149e8caf6e283381bb8b17dde6a73b
merge base: 8638dfbb33f657d609ed44e5e5464e6fe72c1364
target head (main): 197a1089440d887ad2347457dc678877e1d46e18
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (2)

doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
lib
│  ├── modem_key_mgmt
│  │  │ modem_key_mgmt.c

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 2
✅ Integration tests
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_mosh
- ✅ test-fw-nrfconnect-nrf-iot_positioning - Skipped: Job was skipped as it succeeded in a previous run
- ⚠️ test-fw-nrfconnect-nrf-iot_cloud
- ⚠️ test-fw-nrfconnect-nrf-iot_lwm2m

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

joerchan · 2025-02-05T08:54:51Z

Hope something like this works?

Also I can try to check with @joerchan since I don't think he added his changes to the changelog 😉

My bad, will send an update to the changelog separately @peknis

peknis · 2025-02-05T08:59:23Z

Hope something like this works?
Also I can try to check with @joerchan since I don't think he added his changes to the changelog 😉

My bad, will send an update to the changelog separately @peknis

In this PR or in a separate PR?

joerchan · 2025-02-05T09:33:15Z

In this PR or in a separate PR?

Separate PR, got some changes that go with it also :)

Wait with enabling CMEE until we have managed to acquire the lock. This prevents the following edge case: Thread A calls a function that acquires lock before enabling CMEE, like `read`. Thread B calls one that has the opposite (wrong?) order, like `clear`. 1. Thread A takes lock 2. Thread A enables CMEE (was_enabled = false) 3. Thread A happens to get scheduled out 4. Thread B enables CMEE (was_enabled = true) 5. Thread B blocks on trying to take lock 6. Thread A disables CMEE (since was_enabled == false) and releases lock 7. Thread B takes lock, tries to run command (but CMEE is disabled!) 8. Thread B doesn't try to disable CMEE since it was enabled before. Signed-off-by: Kamil Krzyzanowski <[email protected]>

@peknis

Add new entry for race condition fix and move the fixes into their own list as suggested by @peknis Signed-off-by: Kamil Krzyzanowski <[email protected]>

kamnxt · 2025-02-05T09:49:41Z

Fixed the changelog as requested.

kamnxt requested review from a team as code owners February 4, 2025 14:39

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Feb 4, 2025

NordicBuilder added the external External contribution label Feb 4, 2025

kamnxt force-pushed the key_mgmt_lock_cmee_order branch 2 times, most recently from 27be2b6 to 6e2d2a2 Compare February 4, 2025 15:07

kamnxt force-pushed the key_mgmt_lock_cmee_order branch from 6e2d2a2 to 732e6c7 Compare February 4, 2025 15:49

kamnxt requested a review from a team as a code owner February 4, 2025 15:49

github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Feb 4, 2025

eivindj-nordic added the CI-Requested Approves single commit for CI tests on Internal HW label Feb 4, 2025

peknis requested changes Feb 5, 2025

View reviewed changes

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved

kamnxt added 2 commits February 5, 2025 10:47

doc: changelog: add modem_key_mgmt changes

e69916c

Add new entry for race condition fix and move the fixes into their own list as suggested by @peknis Signed-off-by: Kamil Krzyzanowski <[email protected]>

kamnxt force-pushed the key_mgmt_lock_cmee_order branch from 732e6c7 to e69916c Compare February 5, 2025 09:48

github-actions bot removed the CI-Requested Approves single commit for CI tests on Internal HW label Feb 5, 2025

peknis approved these changes Feb 5, 2025

View reviewed changes

rlubos added the CI-Requested Approves single commit for CI tests on Internal HW label Feb 5, 2025

eivindj-nordic approved these changes Feb 5, 2025

View reviewed changes

rlubos approved these changes Feb 5, 2025

View reviewed changes

rlubos merged commit 0098708 into nrfconnect:main Feb 5, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: modem_key_mgmt: enable CMEE _after_ taking lock #20185

lib: modem_key_mgmt: enable CMEE _after_ taking lock #20185

kamnxt commented Feb 4, 2025

NordicBuilder commented Feb 4, 2025

eivindj-nordic commented Feb 4, 2025

NordicBuilder commented Feb 4, 2025

kamnxt commented Feb 4, 2025

eivindj-nordic commented Feb 4, 2025

kamnxt commented Feb 4, 2025

NordicBuilder commented Feb 4, 2025 •

edited

Loading

joerchan commented Feb 5, 2025

peknis commented Feb 5, 2025

joerchan commented Feb 5, 2025

kamnxt commented Feb 5, 2025

lib: modem_key_mgmt: enable CMEE _after_ taking lock #20185

lib: modem_key_mgmt: enable CMEE _after_ taking lock #20185

Conversation

kamnxt commented Feb 4, 2025

NordicBuilder commented Feb 4, 2025

eivindj-nordic commented Feb 4, 2025

NordicBuilder commented Feb 4, 2025

kamnxt commented Feb 4, 2025

eivindj-nordic commented Feb 4, 2025

kamnxt commented Feb 4, 2025

NordicBuilder commented Feb 4, 2025 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

joerchan commented Feb 5, 2025

peknis commented Feb 5, 2025

joerchan commented Feb 5, 2025

kamnxt commented Feb 5, 2025

NordicBuilder commented Feb 4, 2025 •

edited

Loading