P2P support #25346
P2P support #25346
Conversation
NordicBuilder
added
doc-required
|
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 2 projects with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 8e82484e1c4c34ebdf7632b7af8c589176e53fce more detailssdk-nrf:
nrfxlib:
hostap:
zephyr:
Github labels
List of changed files detected by CI (78)Outputs:ToolchainVersion: 4e36fbc961 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
kapbh
force-pushed
the
p2p_support
branch
2 times, most recently
from
3ca0910 to
d2098af
Compare
| CONFIG_WIFI_NM_WPA_SUPPLICANT_P2P=y | ||
| CONFIG_WPA_CLI=y | ||
| CONFIG_WIFI_NM_WPA_SUPPLICANT_LOG_LEVEL_INF=y | ||
| CONFIG_LTO=y |
There was a problem hiding this comment.
add a comment as to why this is needed.
|
There is no doc build, please check. |
richabp
left a comment
richabp
left a comment
There was a problem hiding this comment.
Needs a Changelog entry for both the docs.
| CONFIG_WIFI_NM_WPA_SUPPLICANT_P2P=y | ||
| CONFIG_WPA_CLI=y | ||
| CONFIG_WIFI_NM_WPA_SUPPLICANT_LOG_LEVEL_INF=y | ||
| CONFIG_LTO=y |
There was a problem hiding this comment.
and why is this needed? Also why is the log level changed?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if HOSTAP_CRYPTO_ALT_PSA | ||
| # PSA doesn't work with WPA3 builtin (uses bignum) |
There was a problem hiding this comment.
The comment "PSA doesn't work with WPA3 builtin (uses bignum)" could be misleading since this PR introduces WPA3 PSA support. Consider updating the comment to clarify:
# PSA doesn't work with WPA3 builtin implementation (uses bignum)
# Use HOSTAP_CRYPTO_WPA3_PSA for WPA3 support with PSA
| # PSA doesn't work with WPA3 builtin (uses bignum) | |
| # PSA doesn't work with WPA3 builtin implementation (uses bignum) | |
| # Use HOSTAP_CRYPTO_WPA3_PSA for WPA3 support with PSA |
| @@ -16,8 +16,9 @@ tests: | |||
| sysbuild: true | |||
| build_only: true | |||
| extra_args: | |||
| - EXTRA_CONF_FILE:STRING="overlay-netusb.conf;overlay-enterprise.conf" | |||
| - EXTRA_CONF_FILE="overlay-netusb.conf;overlay-enterprise.conf" | |||
There was a problem hiding this comment.
The removal of :STRING= from the EXTRA_CONF_FILE line changes the CMake variable type specification. This could potentially affect how the variable is processed. Ensure this change is intentional and that the build system correctly handles the untyped variable assignment.
| - EXTRA_CONF_FILE="overlay-netusb.conf;overlay-enterprise.conf" | |
| - EXTRA_CONF_FILE:STRING="overlay-netusb.conf;overlay-enterprise.conf" |
| wpa_printf(MSG_DEBUG, "WPA3-PSA: PT derivation - Group 0 (auto-select) - using " | ||
| "group 19 (NIST P-256)"); | ||
| group = 19; | ||
| } else { | ||
| wpa_printf(MSG_DEBUG, | ||
| "WPA3-PSA: Unsupported group %d for PT derivation (only group 19 NIST " | ||
| "P-256 supported)", |
There was a problem hiding this comment.
The error handling logic in wpa3_psa_derive_pt_group is incorrect. When group is not 0, the function immediately returns NULL for any non-zero group value, even when group == 19 (which should be the only supported value).
The logic should be:
if (group == 0) {
wpa_printf(MSG_DEBUG, "WPA3-PSA: PT derivation - Group 0 (auto-select) - using group 19 (NIST P-256)");
group = 19;
} else if (group != 19) {
wpa_printf(MSG_DEBUG, "WPA3-PSA: Unsupported group %d for PT derivation (only group 19 NIST P-256 supported)", group);
return NULL;
}| wpa_printf(MSG_DEBUG, "WPA3-PSA: PT derivation - Group 0 (auto-select) - using " | |
| "group 19 (NIST P-256)"); | |
| group = 19; | |
| } else { | |
| wpa_printf(MSG_DEBUG, | |
| "WPA3-PSA: Unsupported group %d for PT derivation (only group 19 NIST " | |
| "P-256 supported)", | |
| wpa_printf(MSG_DEBUG, "WPA3-PSA: PT derivation - Group 0 (auto-select) - using group 19 (NIST P-256)"); | |
| group = 19; | |
| } else if (group != 19) { | |
| wpa_printf(MSG_DEBUG, | |
| "WPA3-PSA: Unsupported group %d for PT derivation (only group 19 NIST P-256 supported)", |
| if (!pt) { | ||
| return; | ||
| } | ||
|
|
||
| /* Free PT structure */ | ||
| os_free(pt); |
There was a problem hiding this comment.
Memory leak: If the PT structure is successfully allocated but there is a next PT in the chain, the function sae_deinit_pt only frees the first PT structure and doesn't traverse the linked list. This will leak memory for any additional PT structures in the chain.
The function should recursively free all PT structures in the linked list:
void sae_deinit_pt(struct sae_pt *pt)
{
struct sae_pt *next;
while (pt) {
next = pt->next;
os_free(pt);
pt = next;
}
}| if (!pt) { | |
| return; | |
| } | |
| /* Free PT structure */ | |
| os_free(pt); | |
| struct sae_pt *next; | |
| while (pt) { | |
| next = pt->next; | |
| os_free(pt); | |
| pt = next; | |
| } |
| diff = value[i] - modulus[i] - borrow; | ||
| if (diff > value[i] || (value[i] == modulus[i] && borrow)) { | ||
| borrow = 1; | ||
| } else { | ||
| borrow = 0; | ||
| } |
There was a problem hiding this comment.
Incorrect borrow detection logic. The condition (value[i] == modulus[i] && borrow) is checking if values are equal AND there's a borrow, but the borrow flag should be set when the subtraction underflows. The correct logic should check if the subtraction result is greater than the original value (indicating underflow):
diff = (uint32_t)value[i] - (uint32_t)modulus[i] - borrow;
borrow = (diff > 0xFF) ? 1 : 0;
value[i] = diff & 0xFF;The current implementation may incorrectly set the borrow flag.
| diff = value[i] - modulus[i] - borrow; | |
| if (diff > value[i] || (value[i] == modulus[i] && borrow)) { | |
| borrow = 1; | |
| } else { | |
| borrow = 0; | |
| } | |
| diff = (uint32_t)value[i] - (uint32_t)modulus[i] - borrow; | |
| borrow = (diff > 0xFF) ? 1 : 0; |
samples/wifi/wfa_qt_app/sample.yaml
Outdated
| - wfa_qt_app_SNIPPET="wifi-enterprise" | ||
| - CONFIG_NRF_WIFI_DATA_HEAP_SIZE=90000 |
There was a problem hiding this comment.
Configuration conflict: CONFIG_NRF_WIFI_DATA_HEAP_SIZE is set to 80000 in overlay-enterprise.conf (line 1) but overridden to 90000 in sample.yaml (line 21). This creates confusion about which value should be used. Consider either:
- Removing the setting from one location
- Using a single consistent value
- Documenting why different values are needed in different contexts
| - CONFIG_NRF_WIFI_DATA_HEAP_SIZE=90000 |
| @@ -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: f316ea6f7080c448f423f68b44a0f6fe78d7f455 | |||
| revision: pull/3452/head | |||
There was a problem hiding this comment.
Using pull/3452/head as a revision is not a valid permanent reference for production code. Pull request references are temporary and will become invalid once the PR is merged or closed. This should be changed to a specific commit SHA or a stable branch/tag reference.
Replace with a concrete commit SHA:
revision: <commit_sha>
| revision: pull/3452/head | |
| revision: <commit_sha> |
| if (group == 0) { | ||
| group = 19; | ||
| } else { | ||
| wpa_printf(MSG_ERROR, "WPA3-PSA: Only group 19 (NIST P-256) is supported, got %d", | ||
| group); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
The error handling logic in sae_set_group is incorrect. When group is not 0, the function immediately returns an error for any non-zero group value, even when group == 19 (which should be the only supported value).
The logic should be:
if (group == 0) {
group = 19;
} else if (group != 19) {
wpa_printf(MSG_ERROR, "WPA3-PSA: Only group 19 (NIST P-256) is supported, got %d", group);
return -1;
}| /* SAE commit message should be exactly 98 bytes: [group][scalar][element] */ | ||
| if (len != 98) { |
There was a problem hiding this comment.
The buffer size validation uses a magic number 98 without explanation. This should be defined as a named constant to improve code maintainability and clarity. Consider defining:
#define WPA3_SAE_COMMIT_MESSAGE_LEN 98 /* [group(2)][scalar(32)][element(64)] */Then use this constant throughout the code instead of the hardcoded value.
| os_memcpy(local_scalar, op->local_commit + 2, 32); | ||
| os_memcpy(peer_scalar, op->peer_commit + 2, 32); |
There was a problem hiding this comment.
The magic numbers 32 for NIST P-256 scalar/field sizes and buffer sizes appear throughout the code without named constants. This reduces maintainability. Consider defining constants such as:
#define WPA3_SAE_P256_SCALAR_LEN 32
#define WPA3_SAE_P256_ELEMENT_X_LEN 32
#define WPA3_SAE_P256_ELEMENT_Y_LEN 32This would make the code more self-documenting and easier to maintain.
Pull Wi-Fi P2P support. Signed-off-by: Ravi Dondaputi <[email protected]>
Pull in changes done to support WPS connection and allowing probe request to be sent to supplicant. Signed-off-by: Ravi Dondaputi <[email protected]>
Create snippet for P2P build. Signed-off-by: Ravi Dondaputi <[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]>
krish2718
force-pushed
the
p2p_support
branch
from
224e304 to
4386362
Compare
Add Wi-Fi direct (p2p mode) document. Signed-off-by: Kapil Bhatt <[email protected]>
Remove the type, else local twister is ignoring the overlays. Signed-off-by: Chaitanya Tata <[email protected]>
Enable LTO to fix the overflow and also to make room for future increase. Signed-off-by: Chaitanya Tata <[email protected]>
Add an entry for WPA3-SAE PSA experimental support. Signed-off-by: Chaitanya Tata <[email protected]>
krish2718
force-pushed
the
p2p_support
branch
from
4386362 to
8e82484
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * - Group 19 (NIST P-256) only | ||
| * - H2E (Hash-to-Element) support | ||
| * - HNP (Hash-and-Password) support | ||
| * - No GDH (Group-Dependent Hash) support |
There was a problem hiding this comment.
The comment states "No GDH (Group-Dependent Hash) support" but the code includes GDH support logic (see lines 996-1002 with PSA_ALG_WPA3_SAE_GDH). This documentation is inconsistent with the implementation.
| * - No GDH (Group-Dependent Hash) support | |
| * - GDH (Group-Dependent Hash) support |
|
|
||
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Derive PT - group %d (PSA only)", group); | ||
|
|
||
| if (ssid_len > 32) { |
There was a problem hiding this comment.
The hard-coded value 32 for SSID length validation should be defined as a named constant (e.g., WPA3_PSA_MAX_SSID_LEN) for better maintainability, especially since it's also used at line 55 for the ssid array size.
| static int wpa3_psa_derive_commit(struct wpa3_psa_operation *op) | ||
| { | ||
| psa_status_t status; | ||
| u8 commit_data[1024]; /* Buffer for commit message */ |
There was a problem hiding this comment.
Hard-coded buffer size 1024 is used for commit_data without justification. This should be defined as a named constant, ideally reusing the already defined WPA3_PSA_MAX_COMMIT_LEN constant from line 31.
| u8 commit_data[1024]; /* Buffer for commit message */ | |
| u8 commit_data[WPA3_PSA_MAX_COMMIT_LEN]; /* Buffer for commit message */ |
| static int wpa3_psa_process_commit(struct wpa3_psa_operation *op, const u8 *commit_data, | ||
| size_t commit_len) | ||
| { | ||
| psa_status_t status; |
There was a problem hiding this comment.
Hard-coded send_confirm_counter value {0x01, 0x00} appears to be little-endian representation of 1, but this is not documented. Add a comment explaining the byte order and why this specific encoding is required by the protocol.
| psa_status_t status; | |
| psa_status_t status; | |
| /* The send_confirm_counter is encoded as a 2-byte little-endian value. | |
| * {0x01, 0x00} represents the integer 1 in little-endian format, as required | |
| * by the WPA3 SAE protocol and the PSA PAKE API for the confirm counter field. | |
| */ |
| pmk_buf = (uint8_t *)os_malloc(32); | ||
| if (!pmk_buf) { | ||
| wpa_printf(MSG_ERROR, "WPA3-PSA: Failed to allocate memory for PMK"); | ||
| goto cleanup; | ||
| } | ||
|
|
||
| status = psa_export_key(shared_key_id, pmk_buf, 32, &pmk_len); | ||
| if (status != PSA_SUCCESS) { | ||
| wpa_printf(MSG_ERROR, "WPA3-PSA: Failed to export shared key (PMK): %d", status); | ||
| goto cleanup; | ||
| } | ||
|
|
There was a problem hiding this comment.
Memory allocation uses os_malloc(32) with a hard-coded size. This should use a named constant representing the expected PMK size, or better yet, use the size from PSA APIs to avoid assumptions about the key size.
| pmk_buf = (uint8_t *)os_malloc(32); | |
| if (!pmk_buf) { | |
| wpa_printf(MSG_ERROR, "WPA3-PSA: Failed to allocate memory for PMK"); | |
| goto cleanup; | |
| } | |
| status = psa_export_key(shared_key_id, pmk_buf, 32, &pmk_len); | |
| if (status != PSA_SUCCESS) { | |
| wpa_printf(MSG_ERROR, "WPA3-PSA: Failed to export shared key (PMK): %d", status); | |
| goto cleanup; | |
| } | |
| { | |
| psa_key_attributes_t key_attrs = PSA_KEY_ATTRIBUTES_INIT; | |
| size_t pmk_buf_size = 0; | |
| status = psa_get_key_attributes(shared_key_id, &key_attrs); | |
| if (status != PSA_SUCCESS) { | |
| wpa_printf(MSG_ERROR, "WPA3-PSA: Failed to get key attributes: %d", status); | |
| goto cleanup; | |
| } | |
| pmk_buf_size = PSA_BITS_TO_BYTES(psa_get_key_bits(&key_attrs)); | |
| psa_reset_key_attributes(&key_attrs); | |
| pmk_buf = (uint8_t *)os_malloc(pmk_buf_size); | |
| if (!pmk_buf) { | |
| wpa_printf(MSG_ERROR, "WPA3-PSA: Failed to allocate memory for PMK"); | |
| goto cleanup; | |
| } | |
| status = psa_export_key(shared_key_id, pmk_buf, pmk_buf_size, &pmk_len); | |
| if (status != PSA_SUCCESS) { | |
| wpa_printf(MSG_ERROR, "WPA3-PSA: Failed to export shared key (PMK): %d", status); | |
| goto cleanup; | |
| } | |
| } |
| u8 password[256]; | ||
| size_t password_len; | ||
| u8 shared_key[256]; /* Increased for PSA PAKE commit/confirm data */ | ||
| size_t shared_key_len; | ||
| u8 local_commit[256]; /* Store local commit for context calculation */ | ||
| size_t local_commit_len; | ||
| u8 peer_commit[256]; /* Store peer commit for context calculation */ | ||
| size_t peer_commit_len; |
There was a problem hiding this comment.
Hard-coded buffer sizes (256 bytes) are used throughout the code without clear justification. Consider defining named constants (e.g., WPA3_PSA_MAX_PASSWORD_LEN, WPA3_PSA_MAX_SHARED_KEY_LEN) to make the code more maintainable and document the rationale for these sizes.
| } | ||
|
|
||
| /* SAE commit message should be exactly 98 bytes: [group][scalar][element] */ | ||
| if (len != 98) { |
There was a problem hiding this comment.
The same hard-coded value 98 appears again for commit size validation. This duplication (also at line 349, 389, 1174) reinforces the need for a named constant to avoid maintenance issues.
| /* Use correct key type based on H2E */ | ||
| if (params->h2e) { | ||
| psa_set_key_type(&attributes, PSA_KEY_TYPE_PASSWORD); | ||
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Using PSA_KEY_TYPE_PASSWORD for H2E"); | ||
| } else { | ||
| psa_set_key_type(&attributes, PSA_KEY_TYPE_PASSWORD); | ||
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Using PSA_KEY_TYPE_PASSWORD for basic SAE"); | ||
| } |
There was a problem hiding this comment.
Redundant code: The same key type PSA_KEY_TYPE_PASSWORD is set for both H2E and non-H2E cases. Either the conditional is unnecessary, or different key types should be used for different modes. If they're truly the same, remove the conditional and consolidate the logic.
| /* Use correct key type based on H2E */ | |
| if (params->h2e) { | |
| psa_set_key_type(&attributes, PSA_KEY_TYPE_PASSWORD); | |
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Using PSA_KEY_TYPE_PASSWORD for H2E"); | |
| } else { | |
| psa_set_key_type(&attributes, PSA_KEY_TYPE_PASSWORD); | |
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Using PSA_KEY_TYPE_PASSWORD for basic SAE"); | |
| } | |
| /* Use PSA_KEY_TYPE_PASSWORD for both H2E and basic SAE */ | |
| psa_set_key_type(&attributes, PSA_KEY_TYPE_PASSWORD); | |
| wpa_printf(MSG_DEBUG, "WPA3-PSA: Using PSA_KEY_TYPE_PASSWORD for SAE (H2E: %s)", params->h2e ? "enabled" : "disabled"); |
| /* Initialize operation fields */ | ||
| op->state = SAE_NOTHING; | ||
| op->password_key = PSA_KEY_ID_NULL; | ||
| op->send_confirm = 1; /* Initialize send_confirm counter to 1 (not 0) */ |
There was a problem hiding this comment.
The send_confirm field is initialized to 1 with a comment stating "not 0", but this magic number needs clarification. Consider adding a more detailed comment explaining why the initial value must be 1 according to the SAE protocol specification.
| op->send_confirm = 1; /* Initialize send_confirm counter to 1 (not 0) */ | |
| /* | |
| * Initialize send_confirm counter to 1 (not 0). | |
| * According to the SAE protocol specification (IEEE Std 802.11-2020, Section 12.4.5.4), | |
| * the confirm message counter starts at 1 for the first confirm exchange. | |
| * This ensures protocol compliance and prevents replay attacks. | |
| */ | |
| op->send_confirm = 1; |
| :local: | ||
| :depth: 2 | ||
|
|
||
| Wi-Fi Direct® (also known as Wi-Fi P2P or peer-to-peer mode) enables direct device-to-device connections without requiring a traditional access point. |
There was a problem hiding this comment.
[nitpick] Inconsistent hyphenation: The documentation uses "peer-to-peer" (with hyphens) in line 10 but "device-to-device" (also with hyphens) in the same sentence. While both are correct, consider using consistent terminology throughout. The industry standard term is "peer-to-peer" or "P2P".
| Wi-Fi Direct® (also known as Wi-Fi P2P or peer-to-peer mode) enables direct device-to-device connections without requiring a traditional access point. | |
| Wi-Fi Direct® (also known as Wi-Fi P2P or peer-to-peer mode) enables direct peer-to-peer connections without requiring a traditional access point. |
FrancescoSer
left a comment
FrancescoSer
left a comment
There was a problem hiding this comment.
I'm approving this not to block rc1, however, I think the wifi direct page might need another in-depth docreview after the tagging.
|
Integrated in #25877 |
15 participants
Brings in P2P support.