Skip to content

Conversation

@fengming-ye
Copy link
Contributor

Port crypto mbedtls wrapper for enterprise and DPP.

Porting from
https://github.com/gstrauss/hostap/blob/mbedtls/src/crypto/crypto_mbedtls.c

@fengming-ye fengming-ye marked this pull request as ready for review May 24, 2024 05:52
@fengming-ye fengming-ye force-pushed the feature/cryto_mbedtls_wrapper branch 2 times, most recently from 2cf7634 to 61fb224 Compare May 28, 2024 02:44
@fengming-ye
Copy link
Contributor Author

Hi @jukkar @krish2718
crypto_mbedtls.c and tls_mbedtls.c are both total re-write and replace.
is this okay to merge?

@fengming-ye fengming-ye force-pushed the feature/cryto_mbedtls_wrapper branch 2 times, most recently from 62e8001 to b5cc730 Compare May 29, 2024 07:32
@fengming-ye
Copy link
Contributor Author

Update as previous comments.
Add crypto_mbedtls_alt.c and tls_mbedtls_alt.c enabled by CONFIG_WIFI_NM_WPA_SUPPLICANT_CRYPTO_ALT
in this PR
zephyrproject-rtos/zephyr#73249

Add crypto_mbedtls_alt.c and tls_mbedtls_alt.c,
which have more functionality for enterprise and DPP.
Can be used by crypto backend CONFIG_WIFI_NM_WPA_SUPPLICANT_CRYPTO_ALT.
Add supp_psa_api.c to use PSA apis for HW acceleration in mbedtls 3.x.
Add wpa_supp_els_pkc_mbedtls_config.h as an example for mbedtls
user config for enterprise.

Porting from
https://github.com/gstrauss/hostap/blob/mbedtls/src/crypto/crypto_mbedtls.c

Signed-off-by: Fengming Ye <[email protected]>
@fengming-ye fengming-ye force-pushed the feature/cryto_mbedtls_wrapper branch from b5cc730 to 3884bb7 Compare May 30, 2024 02:23
@dleach02
Copy link
Member

Are we good with the copyright concerns now?

@dleach02 dleach02 requested a review from jukkar May 30, 2024 21:59
@jukkar
Copy link
Member

jukkar commented May 31, 2024

Are we good with the copyright concerns now?

Yes, looks much better now, thanks for the fixes.

@jukkar
Copy link
Member

jukkar commented Jun 4, 2024

@krish2718 please take a look

@krish2718
Copy link
Collaborator

LGTM, except for last commit, can you please elaborate on Thus when DPP enabled, unfixed warning will pop out? The fix for warnings are essential to pass tests with -Werror and the data type fix was found using coverity (downstream NCS).

@fengming-ye
Copy link
Contributor Author

LGTM, except for last commit, can you please elaborate on Thus when DPP enabled, unfixed warning will pop out? The fix for warnings are essential to pass tests with -Werror and the data type fix was found using coverity (downstream NCS).

Sry I didn't catch your point. The last commit is to fix warnings below:

modules/lib/hostap/src/common/dpp.c:3293:37: warning: unsigned conversion from 'int' to 'enum dpp_status_error' changes value from '256' to '0' [-Woverflow]
modules/lib/hostap/src/common/wpa_ctrl.h:208:24: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]

@krish2718
Copy link
Collaborator

LGTM, except for last commit, can you please elaborate on Thus when DPP enabled, unfixed warning will pop out? The fix for warnings are essential to pass tests with -Werror and the data type fix was found using coverity (downstream NCS).

Sry I didn't catch your point. The last commit is to fix warnings below:

modules/lib/hostap/src/common/dpp.c:3293:37: warning: unsigned conversion from 'int' to 'enum dpp_status_error' changes value from '256' to '0' [-Woverflow] modules/lib/hostap/src/common/wpa_ctrl.h:208:24: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]

My bad not the last commit, it's this 00c11d0 revert.

@fengming-ye
Copy link
Contributor Author

LGTM, except for last commit, can you please elaborate on Thus when DPP enabled, unfixed warning will pop out? The fix for warnings are essential to pass tests with -Werror and the data type fix was found using coverity (downstream NCS).

Sry I didn't catch your point. The last commit is to fix warnings below:
modules/lib/hostap/src/common/dpp.c:3293:37: warning: unsigned conversion from 'int' to 'enum dpp_status_error' changes value from '256' to '0' [-Woverflow] modules/lib/hostap/src/common/wpa_ctrl.h:208:24: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]

My bad not the last commit, it's this 00c11d0 revert.

Oh this is because except from some printfs %ld changed to %lld. There are more %ld unchanged. And I don't want to change much DPP original code for this. I have doubt that if %ld is changed to %lld, won't it cause type warning on Linux?

On the other hand, why would os_time_t defined in unsigned long has warnings as Linux is using unsigned long?
Could you pls share the warning?

BTW my local build has no warning with these commits.

@krish2718
Copy link
Collaborator

LGTM, except for last commit, can you please elaborate on Thus when DPP enabled, unfixed warning will pop out? The fix for warnings are essential to pass tests with -Werror and the data type fix was found using coverity (downstream NCS).

Sry I didn't catch your point. The last commit is to fix warnings below:
modules/lib/hostap/src/common/dpp.c:3293:37: warning: unsigned conversion from 'int' to 'enum dpp_status_error' changes value from '256' to '0' [-Woverflow] modules/lib/hostap/src/common/wpa_ctrl.h:208:24: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]

My bad not the last commit, it's this 00c11d0 revert.

Oh this is because except from some printfs %ld changed to %lld. There are more %ld unchanged. And I don't want to change much DPP original code for this. I have doubt that if %ld is changed to %lld, won't it cause type warning on Linux?

The fixes weren't upstreamed but this all depends on the host machine that you are building for.

On the other hand, why would os_time_t defined in unsigned long has warnings as Linux is using unsigned long? Could you pls share the warning?

This was warning from coverity, and it should be signed 64bit to capture full times resolution.

I need to dig my mailbox for the warnings, but I would suggest to remove this commit from the PR and we can handle it separately as it needs a discussion. Rest of the commits are good to go. WDYT?

BTW my local build has no warning with these commits.

Yeah, might be toolchain dependent and which flags are enabled by default.

Fix build warnings from hostap original code when DPP enabled.

Signed-off-by: Fengming Ye <[email protected]>
@fengming-ye fengming-ye force-pushed the feature/cryto_mbedtls_wrapper branch from 3884bb7 to 2b7b93d Compare June 6, 2024 10:23
@fengming-ye
Copy link
Contributor Author

LGTM, except for last commit, can you please elaborate on Thus when DPP enabled, unfixed warning will pop out? The fix for warnings are essential to pass tests with -Werror and the data type fix was found using coverity (downstream NCS).

Sry I didn't catch your point. The last commit is to fix warnings below:
modules/lib/hostap/src/common/dpp.c:3293:37: warning: unsigned conversion from 'int' to 'enum dpp_status_error' changes value from '256' to '0' [-Woverflow] modules/lib/hostap/src/common/wpa_ctrl.h:208:24: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]

My bad not the last commit, it's this 00c11d0 revert.

Oh this is because except from some printfs %ld changed to %lld. There are more %ld unchanged. And I don't want to change much DPP original code for this. I have doubt that if %ld is changed to %lld, won't it cause type warning on Linux?

The fixes weren't upstreamed but this all depends on the host machine that you are building for.

On the other hand, why would os_time_t defined in unsigned long has warnings as Linux is using unsigned long? Could you pls share the warning?

This was warning from coverity, and it should be signed 64bit to capture full times resolution.

I need to dig my mailbox for the warnings, but I would suggest to remove this commit from the PR and we can handle it separately as it needs a discussion. Rest of the commits are good to go. WDYT?

BTW my local build has no warning with these commits.

Yeah, might be toolchain dependent and which flags are enabled by default.

Okay I removed this commit.

@jukkar jukkar merged commit 20f4cac into zephyrproject-rtos:main Jun 6, 2024
@fengming-ye fengming-ye deleted the feature/cryto_mbedtls_wrapper branch June 7, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants