Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions subsys/net/ip/Kconfig.ipv6
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,10 @@ config NET_IPV6_IID_EUI_64
config NET_IPV6_IID_STABLE
bool "Generate stable IID [EXPERIMENTAL]"
select MBEDTLS
select MBEDTLS_MD
select MBEDTLS_PSA_CRYPTO_C
Comment on lines 226 to +227
Copy link
Contributor

Choose a reason for hiding this comment

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

I know #96415 is not merged yet, but are you planning to update this (and similar occurrences)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, #96415 will likely be merged before this one. Once this will be done I will update this PR accordingly. I didn't do this for now because I don't want spurious failures in the CI.

select PSA_WANT_KEY_TYPE_HMAC
select PSA_WANT_ALG_HMAC
select PSA_WANT_ALG_SHA_256
select EXPERIMENTAL
depends on !NET_6LO
help
Expand All @@ -246,7 +249,10 @@ endchoice
config NET_IPV6_PE
bool "Privacy extension (RFC 8981) support [EXPERIMENTAL]"
select MBEDTLS
select MBEDTLS_MD
select MBEDTLS_PSA_CRYPTO_C
select PSA_WANT_KEY_TYPE_HMAC
select PSA_WANT_ALG_HMAC
select PSA_WANT_ALG_SHA_256
select EXPERIMENTAL
select NET_MGMT
select NET_MGMT_EVENT
Expand Down
44 changes: 24 additions & 20 deletions subsys/net/ip/ipv6.c
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ LOG_MODULE_REGISTER(net_ipv6, CONFIG_NET_IPV6_LOG_LEVEL);

#if defined(CONFIG_NET_IPV6_IID_STABLE)
#include <zephyr/random/random.h>
#include <mbedtls/md.h>
#include <psa/crypto.h>
#endif /* CONFIG_NET_IPV6_IID_STABLE */

#include <zephyr/net/net_core.h>
Expand Down Expand Up @@ -875,10 +875,12 @@ static int gen_stable_iid(uint8_t if_index,
size_t stable_iid_len)
{
#if defined(CONFIG_NET_IPV6_IID_STABLE)
const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type(MBEDTLS_MD_SHA256);
mbedtls_md_context_t ctx;
psa_key_id_t key_id;
psa_key_attributes_t key_attr = PSA_KEY_ATTRIBUTES_INIT;
psa_mac_operation_t mac_op = PSA_MAC_OPERATION_INIT;
psa_status_t status;
uint8_t digest[32];
int ret;
size_t digest_len;
static bool once;
static uint8_t secret_key[16]; /* Min 128 bits, RFC 7217 ch 5 */
struct {
Expand Down Expand Up @@ -909,28 +911,30 @@ static int gen_stable_iid(uint8_t if_index,
once = true;
}

mbedtls_md_init(&ctx);
ret = mbedtls_md_setup(&ctx, md_info, true);
if (ret != 0) {
NET_DBG("Cannot %s hmac (%d)", "setup", ret);
psa_set_key_type(&key_attr, PSA_KEY_TYPE_HMAC);
psa_set_key_algorithm(&key_attr, PSA_ALG_HMAC(PSA_ALG_SHA_256));
psa_set_key_usage_flags(&key_attr, PSA_KEY_USAGE_SIGN_MESSAGE);
status = psa_import_key(&key_attr, secret_key, sizeof(secret_key), &key_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if instead of importing the key every time we could just have the key ID as static in this function and import or generate it just once directly in PSA Crypto. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When should we then destroy that key? The drawback I see in doing what you are suggesting is that this key will occupy a slot in PSA core until is destroyed or the device is reset.
How often are IPv6 addresses supposed to change (with/without privacy extension)? Unless this happens quite often, having a key slot always occupied might require a larger number of key slots in the PSA core.
I'm not against the requested change, but I would like to know if you have any strong preference on this footprint/performance compromise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the RAM usage would probably be bigger because of the metadata. And the key would never be destroyed, just as the static buffer holding the key material is always reserved. No strong opinion, just food for thought, the way it's done is just not PSA-idiomatic. cc @rlubos @jukkar

if (status != PSA_SUCCESS) {
NET_DBG("Cannot %s hmac (%d)", "import key", status);
goto err;
}

ret = mbedtls_md_hmac_starts(&ctx, secret_key, sizeof(secret_key));
if (ret != 0) {
NET_DBG("Cannot %s hmac (%d)", "start", ret);
status = psa_mac_sign_setup(&mac_op, key_id, PSA_ALG_HMAC(PSA_ALG_SHA_256));
if (status != PSA_SUCCESS) {
NET_DBG("Cannot %s hmac (%d)", "setup", status);
goto err;
}

ret = mbedtls_md_hmac_update(&ctx, (uint8_t *)&buf, sizeof(buf));
if (ret != 0) {
NET_DBG("Cannot %s hmac (%d)", "update", ret);
status = psa_mac_update(&mac_op, (uint8_t *)&buf, sizeof(buf));
if (status != PSA_SUCCESS) {
NET_DBG("Cannot %s hmac (%d)", "update", status);
goto err;
}

ret = mbedtls_md_hmac_finish(&ctx, digest);
if (ret != 0) {
NET_DBG("Cannot %s hmac (%d)", "finish", ret);
status = psa_mac_sign_finish(&mac_op, digest, sizeof(digest), &digest_len);
if (status != PSA_SUCCESS) {
NET_DBG("Cannot %s hmac (%d)", "finish", status);
goto err;
}

Expand All @@ -940,14 +944,14 @@ static int gen_stable_iid(uint8_t if_index,
if (unlikely(check_reserved(stable_iid, stable_iid_len))) {
LOG_HEXDUMP_DBG(stable_iid, stable_iid_len,
"Generated IID is reserved");
ret = -EINVAL;
goto err;
}

err:
mbedtls_md_free(&ctx);
psa_mac_abort(&mac_op);
psa_destroy_key(key_id);

return ret;
return (status == PSA_SUCCESS) ? 0 : -EIO;
#else
return -ENOTSUP;
#endif
Expand Down
43 changes: 24 additions & 19 deletions subsys/net/ip/ipv6_pe.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ LOG_MODULE_REGISTER(net_ipv6_pe, CONFIG_NET_IPV6_PE_LOG_LEVEL);
#include <zephyr/kernel.h>
#include <zephyr/random/random.h>

#include <mbedtls/md.h>
#include <psa/crypto.h>

#include <zephyr/net/net_core.h>
#include <zephyr/net/net_pkt.h>
Expand Down Expand Up @@ -223,10 +223,12 @@ static int gen_temporary_iid(struct net_if *iface,
uint8_t *temporary_iid,
size_t temporary_iid_len)
{
const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type(MBEDTLS_MD_SHA256);
mbedtls_md_context_t ctx;
psa_key_id_t key_id;
psa_key_attributes_t key_attr = PSA_KEY_ATTRIBUTES_INIT;
psa_mac_operation_t mac_op = PSA_MAC_OPERATION_INIT;
psa_status_t status;
uint8_t digest[32];
int ret;
size_t digest_len;
static bool once;
static uint8_t secret_key[16]; /* Min 128 bits, RFC 8981 ch 3.3.2 */
struct {
Expand Down Expand Up @@ -255,37 +257,40 @@ static int gen_temporary_iid(struct net_if *iface,
once = true;
}

mbedtls_md_init(&ctx);
ret = mbedtls_md_setup(&ctx, md_info, true);
if (ret != 0) {
NET_DBG("Cannot %s hmac (%d)", "setup", ret);
psa_set_key_type(&key_attr, PSA_KEY_TYPE_HMAC);
psa_set_key_algorithm(&key_attr, PSA_ALG_HMAC(PSA_ALG_SHA_256));
psa_set_key_usage_flags(&key_attr, PSA_KEY_USAGE_SIGN_MESSAGE);
status = psa_import_key(&key_attr, secret_key, sizeof(secret_key), &key_id);
if (status != PSA_SUCCESS) {
NET_DBG("Cannot %s hmac (%d)", "import key", status);
goto err;
}

ret = mbedtls_md_hmac_starts(&ctx, secret_key, sizeof(secret_key));
if (ret != 0) {
NET_DBG("Cannot %s hmac (%d)", "start", ret);
status = psa_mac_sign_setup(&mac_op, key_id, PSA_ALG_HMAC(PSA_ALG_SHA_256));
if (status != PSA_SUCCESS) {
NET_DBG("Cannot %s hmac (%d)", "setup", status);
goto err;
}

ret = mbedtls_md_hmac_update(&ctx, (uint8_t *)&buf, sizeof(buf));
if (ret != 0) {
NET_DBG("Cannot %s hmac (%d)", "update", ret);
status = psa_mac_update(&mac_op, (uint8_t *)&buf, sizeof(buf));
if (status != PSA_SUCCESS) {
NET_DBG("Cannot %s hmac (%d)", "update", status);
goto err;
}

ret = mbedtls_md_hmac_finish(&ctx, digest);
if (ret != 0) {
NET_DBG("Cannot %s hmac (%d)", "finish", ret);
status = psa_mac_sign_finish(&mac_op, digest, sizeof(digest), &digest_len);
if (status != PSA_SUCCESS) {
NET_DBG("Cannot %s hmac (%d)", "finish", status);
goto err;
}

memcpy(temporary_iid, digest, MIN(sizeof(digest), temporary_iid_len));

err:
mbedtls_md_free(&ctx);
psa_mac_abort(&mac_op);
psa_destroy_key(key_id);

return ret;
return (status == PSA_SUCCESS) ? 0 : -EIO;
}

void net_ipv6_pe_start(struct net_if *iface, const struct in6_addr *prefix,
Expand Down
1 change: 1 addition & 0 deletions tests/net/iface/prj.conf
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,4 @@ CONFIG_ZTEST=y
CONFIG_NET_IF_MAX_IPV4_COUNT=4
CONFIG_NET_IF_MAX_IPV6_COUNT=4
CONFIG_TEST_USERSPACE=y
CONFIG_MAIN_STACK_SIZE=2048
2 changes: 1 addition & 1 deletion west.yml
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ manifest:
revision: b03edc8e6282a963cd312cd0b409eb5ce263ea75
path: modules/lib/gui/lvgl
- name: mbedtls
revision: 85440ef5fffa95d0e9971e9163719189cf34d979
revision: pull/76/head
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to know what you're pulling in the commit title (e.g. manifest: mbedtls: pull X fix) + typo: "alloacated"

Copy link
Contributor

Choose a reason for hiding this comment

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

pull PR 76 doesn't really give out information 🙂 One can easily trace back what PR the commit came from. I rather meant something like pull fix for [...], but anyway you'll be updating this commit later when the PR is merged.

path: modules/crypto/mbedtls
groups:
- crypto
Expand Down
Loading