Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warning in Crypto when using boot seed injection #9566

Merged
merged 3 commits into from
Feb 7, 2019
Merged

Fix warning in Crypto when using boot seed injection #9566

merged 3 commits into from
Feb 7, 2019

Conversation

alzix
Copy link
Contributor

@alzix alzix commented Jan 31, 2019

Description

When enabling boot seed injection (e.g. FUTURE_SEQUANA_M0_PSA) entropy read & write callbacks are injected via macros and cause implicit declaration compilation warning.

Compile [ 37.1%]: entropy.c
[Warning] <command-line>@0,38: implicit declaration of function 'mbed_default_seed_write' [-Wimplicit-function-declaration]
...
Compile [ 37.1%]: entropy.c
[Warning] <command-line>@0,38: implicit declaration of function 'mbed_default_seed_write' [-Wimplicit-function-declaration]

This PR fixes the warning by adding include to a platfrom_mbed.h

In addition this PR suggests simplified and user friendly way of wiring NVSEED read/write callbacks.
MBEDTLS_ENTROPY_NV_SEED macro is sufficient since the callbacks have fixed values for all PSA targets.

The option for advanced user to inject custom version of MBEDTLS_PLATFORM_NV_SEED_READ_MACRO and MBEDTLS_PLATFORM_NV_SEED_WRITE_MACRO is preserved.

Pull request type

[x] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@netanelgonen @Patater @avolinski @sbutcher-arm

@ciarmcom
Copy link
Member

@alzix, thank you for your changes.
@netanelgonen @Patater @avolinski @sbutcher-arm @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-tls @ARMmbed/mbed-os-maintainers please review.

#ifndef __PLATFORM_MBED__H__
#define __PLATFORM_MBED__H__

#include "default_random_seed.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to include this header?

There should be no "default random seed". That sounds really dangerous. (And not very random).

Copy link
Contributor

Choose a reason for hiding this comment

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

This header contains the default read/write functions that supposed to be registered with the Mbed TLS NV_SEED feature.

@simonbutcher
Copy link
Contributor

Maintainers - I'm too busy to approve this, so I'd like to delegate this to @andresag01. When he's approved it, it's fine by me.

#ifndef __PLATFORM_MBED__H__
#define __PLATFORM_MBED__H__

#include "default_random_seed.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

This default_random_seed.h seems to be TARGET_PSA specific. How will the compiler find it when the target is not TARGET_PSA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


#if (defined(TARGET_PSA) && defined(MBEDTLS_ENTROPY_NV_SEED))

#include "default_random_seed.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this possible to be in target PSA with NV_SEED but in the non-secure side?
If yes, we will have compilation error on default_random_seed.h file would not found

Copy link
Contributor Author

@alzix alzix Feb 4, 2019

Choose a reason for hiding this comment

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

There are two use cases:

  1. PSA target (with SPE and NSPE) and we are building for NSPE. e.g. FUTURE_SEQUANA_PSA for such a targets it is not expected to have MBEDTLS_ENTROPY_NV_SEED macro enabled.
  2. PSA compliant target (NSPE only). e.g. K64F. IIRC you have tested it on such a target yourself :)

Fix warning in entropy.c caused by injecting seed read & write callbacks
Move NVSEED callbacks configuration to a header file
@alzix
Copy link
Contributor Author

alzix commented Feb 6, 2019

@0xc0170 can we proceed with it?

@NirSonnenschein
Copy link
Contributor

@alzix martin is ooo today

@NirSonnenschein
Copy link
Contributor

starting CI

@alekla01
Copy link
Contributor

alekla01 commented Feb 7, 2019

Restarted CI

@mbed-ci
Copy link

mbed-ci commented Feb 7, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 2
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 7, 2019

Labeled for 5.12 (adding new config - stated as refactor above).

One additional question - this is changing mbedtls file, is this intentional (we do not accept changes besides doing version updates) and won't be overriden by next mbedtls update?

@yanesca
Copy link
Contributor

yanesca commented Feb 7, 2019

The files in the features/mbedtls/platform, features/mbedtls/targets directories and the scripts in the features/mbedtls/importer directory are not (in the strict sense) part of Mbed TLS and are unaffected by the Mbed TLS update.

@NirSonnenschein NirSonnenschein merged commit e6c2a1d into ARMmbed:master Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet