wifi: Add WPA3-PSA support #25733
Closed
wifi: Add WPA3-PSA support #25733
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NordicBuilder
added
manifest
changelog-entry-required
Contributor
|
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
Copilot finished reviewing on behalf of
krish2718
November 23, 2025 19:42
Contributor
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 06857104c3bf0b68ba5fa0ec4003fbf0b9328518 more detailssdk-nrf:
hostap:
zephyr:
Github labels
List of changed files detected by CI (857)Outputs:ToolchainVersion: df3cc9d822 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
There was a problem hiding this comment.
Pull request overview
This PR extends PSA crypto support to WPA3-SAE (Simultaneous Authentication of Equals), enabling all personal Wi-Fi security modes to use PSA crypto APIs. The implementation provides a drop-in replacement for the original SAE implementation using PSA PAKE APIs.
Key Changes:
- Added WPA3-SAE PSA implementation with support for Group 19 (NIST P-256), H2E (Hash-to-Element), and HnP modes
- Updated Zephyr dependency to pull/3525/head for required PSA PAKE API support
- Configured build system to conditionally compile WPA3 PSA support
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| west.yml | Updates Zephyr revision to pull in PSA PAKE API support |
| subsys/net/lib/hostap_crypto/wpa3_psa.c | New 1518-line implementation of WPA3-SAE using PSA crypto APIs |
| subsys/net/lib/hostap_crypto/Kconfig | Adds HOSTAP_CRYPTO_WPA3_PSA configuration option with required PSA dependencies |
| subsys/net/lib/hostap_crypto/CMakeLists.txt | Conditionally includes wpa3_psa.c and defines CONFIG_SAE/CONFIG_ECC when WPA3 PSA is enabled |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Processing commit data of %zu bytes", commit_len); | ||
|
|
||
| /* DEBUG: Parse and dump commit message contents */ | ||
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Processing commit data of %zu bytes", commit_len); |
There was a problem hiding this comment.
Redundant duplicate debug message. Line 1139 is an exact duplicate of line 1136: "WPA3-PSA: Processing commit data of %zu bytes". One of these should be removed to avoid confusion in logs.
Suggested change
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Processing commit data of %zu bytes", commit_len); |
| borrow = 0; | ||
| for (i = 31; i >= 0; i--) { | ||
| diff = context[i] - r[i] - borrow; | ||
| if (diff > context[i] || (context[i] == r[i] && borrow)) { |
There was a problem hiding this comment.
Incorrect arithmetic in borrow detection. The condition diff > context[i] will always be false for unsigned subtraction because diff = context[i] - r[i] - borrow means diff is computed modulo 256 and cannot be greater than the original context[i]. The borrow detection logic should be: borrow = (context[i] < r[i] + borrow) ? 1 : 0; or check if (int)context[i] - (int)r[i] - (int)borrow < 0.
Suggested change
| if (diff > context[i] || (context[i] == r[i] && borrow)) { | |
| if (context[i] < r[i] + borrow) { |
Comment on lines
+1482
to
+1487
| diff = context[i] - r[i] - borrow; | ||
| if (diff > context[i] || (context[i] == r[i] && borrow)) { | ||
| borrow = 1; | ||
| } else { | ||
| borrow = 0; | ||
| } |
There was a problem hiding this comment.
Same incorrect borrow detection logic as above. The condition diff > context[i] will not correctly detect borrow in unsigned arithmetic. This needs to be fixed to properly handle underflow when subtracting with borrow.
Suggested change
| diff = context[i] - r[i] - borrow; | |
| if (diff > context[i] || (context[i] == r[i] && borrow)) { | |
| borrow = 1; | |
| } else { | |
| borrow = 0; | |
| } | |
| uint32_t sub = r[i] + borrow; | |
| if (context[i] < sub) { | |
| borrow = 1; | |
| } else { | |
| borrow = 0; | |
| } | |
| diff = context[i] - sub; |
| op->pmk_len = pmk_len; | ||
|
|
||
| /* Generate PMKID from context directly as per IEEE 802.11 specification */ | ||
| /* PMKID = L(context, 0, 128) where context is (commit-scalar + peer-commit-scalar) mod r */ |
There was a problem hiding this comment.
Misleading comment: The comment mentions "IEEE 802.11: PMKID = first 128 bits of (scalar1 + scalar2) mod r" which describes computing PMKID from scalars, but the actual implementation on line 1371 just copies the first 16 bytes of context to pmkid. The comment should clarify that context already contains (scalar1 + scalar2) mod r, computed by wpa3_psa_calculate_context().
Suggested change
| /* PMKID = L(context, 0, 128) where context is (commit-scalar + peer-commit-scalar) mod r */ | |
| /* PMKID = L(context, 0, 128), where context is (commit-scalar + peer-commit-scalar) mod r, | |
| * as computed by wpa3_psa_calculate_context() above. */ |
| /* Initialize PSA PAKE operation */ | ||
| op->pake_op = psa_pake_operation_init(); | ||
|
|
||
| /* Initialize Oberon WPA3-SAE operation */ |
There was a problem hiding this comment.
Misleading comment: The comment says "Initialize Oberon WPA3-SAE operation" but this is supposed to be a generic PSA implementation, not specific to Oberon. This comment should be updated to reflect the generic PSA approach, e.g., "Initialize PSA PAKE operation".
Suggested change
| /* Initialize Oberon WPA3-SAE operation */ | |
| /* Initialize PSA PAKE operation context */ |
| } | ||
|
|
||
| /* PSA PAKE output should be exactly 98 bytes in SAE format: [group][scalar][element] */ | ||
| if (op->shared_key_len != 98) { |
There was a problem hiding this comment.
Magic number 98 is used without explanation. This appears multiple times (lines 332, 336, 372, 1140, 1348) but is never defined as a named constant. Consider defining this as a constant like #define WPA3_PSA_SAE_COMMIT_MSG_LEN 98 with a comment explaining it's the size of [group(2)][scalar(32)][element(64)].
Comment on lines
1358
to
1354
| if (wpa3_psa_calculate_context(local_scalar, peer_scalar, context) < 0) { | ||
| wpa_printf(MSG_ERROR, "WPA3-PSA: Context calculation failed"); | ||
| psa_destroy_key(shared_key_id); | ||
| os_free(pmk_buf); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Potential resource leak: If wpa3_psa_calculate_context() fails on line 1358, the function returns -1 without calling os_free(pmk_buf) (which is freed on line 1375). This creates a memory leak. The cleanup code should be moved to ensure pmk_buf is always freed on all error paths.
Comment on lines
+102
to
+114
| # Disable standard WPA3 support and use the PSA variant | ||
| config WIFI_NM_WPA_SUPPLICANT_WPA3 | ||
| default n | ||
| config WIFI_NM_WPA_SUPPLICANT_CRYPTO_ENTERPRISE | ||
| default n | ||
|
|
||
| config WIFI_NM_WPA_SUPPLICANT_WPA3 | ||
| default n | ||
|
|
||
| config WIFI_NM_WPA_SUPPLICANT_CRYPTO_ENTERPRISE | ||
| default n | ||
|
|
||
| config HOSTAP_CRYPTO_WPA3_PSA | ||
| bool "WPA3 PSA support" | ||
| select PSA_WANT_ALG_WPA3_SAE_PT | ||
| select PSA_WANT_ALG_WPA3_SAE | ||
| select PSA_WANT_KEY_TYPE_WPA3_SAE_PT | ||
| select PSA_WANT_ALG_HKDF | ||
| default y |
There was a problem hiding this comment.
[nitpick] Indentation inconsistency: Lines 103-114 use tab indentation while the rest of the file appears to use different formatting. This creates inconsistent code style. The indentation should match the surrounding context (lines 97-99 and 115-117).
| @@ -64,7 +64,7 @@ manifest: | |||
| # https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/zephyr/guides/modules.html | |||
| - name: zephyr | |||
| repo-path: sdk-zephyr | |||
| revision: de567402f0883a5c237a7478ca6451f0e5c681b3 | |||
| revision: pull/3525/head | |||
There was a problem hiding this comment.
The revision reference pull/3525/head is not a standard Git commit SHA. This appears to be a GitHub pull request reference which is problematic for production code as:
- Pull requests are mutable and can be force-pushed
- Pull request branches may be deleted after merge
- This creates an unstable dependency
Consider using a specific commit SHA instead of a pull request reference for reproducible builds.
Suggested change
| revision: pull/3525/head | |
| revision: 7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f5a6b |
| op->pake_op = psa_pake_operation_init(); | ||
|
|
||
| /* Initialize Oberon WPA3-SAE operation */ | ||
| memset(&op->pake_op, 0, sizeof(op->pake_op)); |
There was a problem hiding this comment.
The psa_pake_operation_init() call on line 127 is immediately followed by memset(&op->pake_op, 0, sizeof(op->pake_op)) on line 130, which overwrites the initialized state. This is incorrect as it defeats the purpose of the initialization function and may lead to undefined behavior. Either remove the memset or the psa_pake_operation_init() call, but keep the proper initialization approach.
Suggested change
| memset(&op->pake_op, 0, sizeof(op->pake_op)); |
|
You can find the documentation preview for this PR here. |
sachinthegreen
approved these changes
Nov 24, 2025
Pull fix for PSA SAE in hostap. Signed-off-by: Chaitanya Tata <[email protected]>
This implements the PSA variant of WPA3, disables the internal WPA3 in the supplicant and uses the nRF Oberon's WPA3 HKDF APIs. Signed-off-by: Chaitanya Tata <[email protected]>
tejlmand
requested changes
Nov 26, 2025
Comment on lines
+87
to
+90
| zephyr_library_compile_definitions( | ||
| CONFIG_SAE | ||
| CONFIG_ECC | ||
| ) |
Contributor
There was a problem hiding this comment.
why is this needed ?
Suggested change
| zephyr_library_compile_definitions( | |
| CONFIG_SAE | |
| CONFIG_ECC | |
| ) |
Contributor
Author
There was a problem hiding this comment.
We are replacing the wpa_supplicant internal WPA3 implementation with our own, but rest of the modules still use SAE/ECC defines, so, we need to pass them to wpa_supplicant.
| # Disable standard WPA3 support and use the PSA variant | ||
| config WIFI_NM_WPA_SUPPLICANT_WPA3 | ||
| default n | ||
| config WIFI_NM_WPA_SUPPLICANT_CRYPTO_ENTERPRISE |
Contributor
There was a problem hiding this comment.
Suggested change
| config WIFI_NM_WPA_SUPPLICANT_CRYPTO_ENTERPRISE | |
| config WIFI_NM_WPA_SUPPLICANT_CRYPTO_ENTERPRISE |
| # PSA crypto is WPA2 only for now | ||
| # PSA crypto is personal security only for now | ||
| if HOSTAP_CRYPTO_ALT_PSA | ||
| # Disable standard WPA3 support and use the PSA variant |
Contributor
There was a problem hiding this comment.
this is technically not disabling the setting, it's changing the default.
What becomes the final value still depends on defconfig loaded prior to this code or prj.conf files if the symbols have prompts.
Contributor
Author
There was a problem hiding this comment.
Yeah, should I add a !depends on WIFI_NM_WPA_SUPPLICANT_WPA3 to make it explicit? Something like this
config HOSTAP_CRYPTO_WPA3_PSA
bool "WPA3 PSA support"
# Only one WPA3 implementation can be enabled at a time
depends on !WIFI_NM_WPA_SUPPLICANT_WPA3
select PSA_WANT_ALG_WPA3_SAE
select PSA_WANT_ALG_WPA3_SAE_H2E
select PSA_WANT_KEY_TYPE_WPA3_SAE_PT
Contributor
Author
|
FYI, this PR is now merged with #25346 (I have the fixes for review comments ready, will push to that PR). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This extends the PSA crypto support to WPA3, now all personal security can use PSA.