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.

Sign up for GitHub

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

Jump to bottom

[nrf noup] For Attestation, use message instead of hash #180

Draft
wants to merge 73 commits into
base: main
Choose a base branch
from
Draft

[nrf noup] For Attestation, use message instead of hash #180

wants to merge 73 commits into from

Conversation

hellesvik-nordic
Copy link

Also updated T_COSE library to support this

SebastianBoe and others added 30 commits August 16, 2024 09:24
@SebastianBoe @frkv
[nrf fromtree] Platform: Nordic: refactor spu_peripheral_config to us…
0fbe454 
…e base addr

Refactor spu_peripheral_config to use base addresses instead of IDs as
future platforms will need the base address to identify which spu
instance to use.

(Cherry picked from commit b60bdb6)

Signed-off-by: Sebastian Bøe <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SebastianBoe @frkv
[nrf fromtree] Platform: Nordic: Add function for calculating spu ins…
bdde9a9 
…tances

Add a function to return the SPU instance that can be used to configure
the peripheral at a given base address.

Signed-off-by: Sebastian Bøe <[email protected]>
Change-Id: Ib1e442a54d599c4e42e74903d49920f24e9d8ec9
(Cherry picked from commit 5d8b824)
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@nordic-mare @frkv
[nrf fromtree] Platform: nordic_nrf: Don't configure NRF_VMC as non-s…
1f7aa03 
…ecure

Dont configure the volatile memory controller as a non-secure peripheral

(cherry picked from commit c670a6a)

Change-Id: I2489defaf6deb89beba7447ba079ea3e5afebca5
Signed-off-by: Markus Rekdal <[email protected]>
@Vge0rge @frkv
[nrf fromtree] platform: nordic: Add platform memory write service
2000539 
There are some hardware registers in Nordic platforms
which are mapped as secure only. In order to allow the
non-secure application to control these registers I added
here a secure service which allows 32-bit writes to secure
mapped memory. The writes are only allowed on  addresses and
masks defined in a header list. It is also possible to
provide an allowed_values list in order to further limit
the accepted values.

Renamed:  tfm_read_ranges.h -> tfm_platform_user_memory_ranges.h
since now it can be used for both reads and writes.

The list in the current platforms is empty and might be populated
later.

Signed-off-by: Georgios Vasilakis <[email protected]>
Change-Id: Ifa31ba73ec07b216a7e987653255fcc6e9d3989c
(cherry picked from commit 57b3342)
@vili-nordic @frkv
[nrf fromtree] ITS: Fix checks for PS usage
90ff6d8 
The check for whether file should be encrypted, and be fully written
missed some PS usage.

Signed-off-by: Vidar Lillebø <[email protected]>
Change-Id: Ifa7fe00e511a6071b2b5c455df84b8e4f0535c84
(Cherry picked from commit dc77905)
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@MarkusLassila @frkv
[nrf fromtree] platform: nordic_nrf: APPROTECT to lock debugging
7c5a541 
NRF_APPROTECT and NRF_SECURE_APPROTECT
to take precedence over other mechanisms when configuring
debugging for TF-M.

For nRF53 and nRF91x1 the actual locking of firmware is done
elsewhere. This further locks the UICR.

nRF9160 supports only hardware APPROTECT. This will lock the
APPROTECT / SECUREAPPROTECT in the next boot, when the above
settings are configured.

Change-Id: I5e304be0f8a34c0016488d9ec09929bbcb38481f
Signed-off-by: Markus Lassila <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
(Cherry picked from commit 734a51d)
@SeppoTakalo @frkv
[nrf fromtree] platform: nordic: Allow reading UICR registers
b61cc79 
On certain nRF plaforms, like nRF9160, reading UICR registers
might need special handling, which is already implemented in
nrfx_nvmc_uicr_word_read() so use that, instead on memcpy().

For more information, see nRF9160 Errata 7.

Change-Id: Iea9d0bf4184decd5650b4d4b620fbef0c64a55f6
Signed-off-by: Seppo Takalo <[email protected]>
(cherry picked from commit ca03e40)
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SeppoTakalo @frkv
[nrf fromtree] platform: nordic: Guard nvmc header with soc series
487231c 
The anomaly only appears on nRF91 platforms and some
platforms do not have NVMC so the header cannot be
included.

Change-Id: I02c73c9a752599ca9be9320dc19f390aea0f767a
Signed-off-by: Seppo Takalo <[email protected]>
(cherry picked from commit 539dd89)
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SebastianBoe @frkv
[nrf noup] Platform: Nordic: Port spu_peripheral_config
6755e5d 
Port spu_peripheral_config to also support the new API.

Signed-off-by: Sebastian Bøe <[email protected]>
Change-Id: I1763874ce74ad39cbf0ef256ef8edc669038d226
(Cherry-picked from the commit 3f49abf)
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SebastianBoe @frkv
[nrf noup] crypto: Have CRYPTO_HW_ACCELERATOR support nrf_security
eec5298 
Adjust CRYPTO_HW_ACCELERATOR build scripts to also support
nrf_security.

Signed-off-by: Sebastian Bøe <[email protected]>
Signed-off-by: Joakim Andersson <[email protected]>
(cherry picked from commit c136210)
(cherry picked from commit 3834117)
Signed-off-by: Joakim Andersson <[email protected]>
(cherry picked from commit 2bdad64)
Signed-off-by: Markus Swarowsky <[email protected]>
Change-Id: Ied8e378ef55fe398ea4e45f65b3c270e9e9cd030
Signed-off-by: Markus Swarowsky <[email protected]>
(cherry picked from commit 5903966)
Signed-off-by: Markus Swarowsky <[email protected]>
(cherry picked from commit a3a03e5)
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SebastianBoe @frkv
[nrf noup] platform: nrf_nordic: Add temp fix for for secure FPU on 9161
7ff00fa 
The MDK for nRF9120 used in the nRF9161 target doesn't define the Secure FPU
as it doesn't exist, but for other platforms like the 9160 it has a dummy
define, with an UNUSED field in the type.
The long plan is to get this fixed in the MDK but until then, to make
the nrfxlib 3.1.0 update possible this tempfix is applied.

 Ref: NCSDK-23046

Signed-off-by: Markus Swarowsky <[email protected]>
Change-Id: I44042ee9aada99c59a5930440306bb6c40ae4880
(cherry picked from commit 6ad9c58)
Signed-off-by: Markus Swarowsky <[email protected]>
(cherry picked from commit a489e9f)
Signed-off-by: Markus Swarowsky <[email protected]>
@frkv
[nrf noup] secure_fw: Add option to log output on a shared UART insta…
813a981 
…nce.

Add an option to send the log output from the secure firmware on a
UART instance that would be shared with the non-secure application.

This option is added where the number of UART instances is limited
and the application only cares about the receiving the TF-M log
on fatal errors.

To allow this option to be enabled the log is disabled in the boot
process before the non-secure application is started.
It is enabled again when an unrecoverable exception has occurred in
the secure firmware.

Here is an abandoned upstream PR (with some of the fixes):
https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/25905

Note: This has removed any information about cherry-picked items
as this is not valid since it is combining efforts form multiple
commits

Ref: NCSDK-18595
Ref: NCSDK-28740

Signed-off-by: Joakim Andersson <[email protected]>
Signed-off-by: Markus Swarowsky <[email protected]>
Signed-off-by: Sebastian Bøe <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
[nrf noup] Do not pass PSA_CONFIG again to crypto library
b6ddb1c 
The MBEDTLS_PSA_CRYPTO_CONFIG_FILE gets already defined in the
mbedtls_common target and is included in the nrf-config.h file.
TF-M adds the compile definition again, causing a redefined warning when
building

We may want to refactor this to align better with upstream project

Ref: NCSDK-28740

Signed-off-by: Markus Swarowsky <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SebastianBoe @frkv
[nrf noup] Remove check for p256-m
a195051 
TF-M checks if p256-m is available during build time using
MBEDCRYPTO_PATH which is set to the TF-M repo to use custom
Mbed TLS cmake configurations, but this means the script can not be
found. But as Mbed TLS software crypto is not used anyway we can
hardcode p256-m to be disabled.

Ref: NCSDK-28740

Signed-off-by: Sebastian Bøe <[email protected]>
Signed-off-by: Markus Swarowsky <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SebastianBoe @frkv
[nrf noup] cmake: tools: Dont add a custom command for running the ma…
566102c 
…nifest

This commit is [nrf noup] because I would like to user-test this for a
few months in case of unintended side-effects before upstreaming.

In the TF-M build scripts we run the manifest tool twice, first from
CMake and then from ninja.

It is bad practice to configure CMake projects like this. Instead, if
configuration from CMake is necessary, one should configure from CMake
only, and then re-run CMake when necessary, not just the command.

This organization has been causing problems for our users as they have
been required to rebuild TF-M twice.

This is due to this scenario playing out:

CMake generates config_impl.cmake by invoking the manifest tool at
Configure time.

CMake generates build.ninja.

Ninja generates config_impl.cmake by invoking the manifest tool at
build time.

When the user then invokes ninja a second time config_impl.cmake will
be newer than build.ninja. But CMake is supposed to be includ'ing
config_impl.cmake, so build.ninja is now considered out-of-date
wrt. config_impl.cmake.

ninja therefore invokes CMake again, and then ninja afterwards.

Ref: NCSDK-28740

Signed-off-by: Sebastian Bøe <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
[nrf noup] crypto: Add PAKE support
34785d9 
This is noup commit as upstream TF-M relies on the mbed TLS PSA Core
hat does not support the PAKE API's according to 1.2 at the moment.
Once this exists then this can be up streamed, or removed if TF-M adds
it themself.

Added PAKE API support accoding the PSA crypto spec 1.2

Ref: NCSDK-22416
Ref: NCSDK-28740

Signed-off-by: Markus Swarowsky <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SebastianBoe @frkv
[nrf noup] platform: nordic_nrf: include autoconf.h from target_cfg.c
026dc04 
Include autoconf.h from target_cfg.c so we can configure the TF-M
image based on the non-secure image's Kconfig.

Signed-off-by: Sebastian Bøe <[email protected]>
Change-Id: I2212f2ec3428f16618334c5583b0e641aa30ea08
@frkv
[nrf noup] Support CMAC KDF and custom builtin solution
b859d7a 
Allows custom key-loader to be used for the PSA core and allows
configuring CMAC KDF usage for PS.

noup-reason: PSA_ALG_SP800_108_COUNTER_CMAC is not available in upstream.
After testing and verifying the solution (determining if we need further
changes) we should try to upstream this.

Ref: NCSDK-28740

Signed-off-by: Vidar Lillebø <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@SebastianBoe @frkv
[nrf noup] crypto: Detect wrong headers being included
56b2e7c 
This commit is a noup because we want an NCS specific error message.

Detect wrong headers being included. See comment for details.

Ref: NCSDK-28740

Signed-off-by: Sebastian Bøe <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
[nrf noup] platform: nordic_nrf: Configure XL1/2 pin based on Kconfig
5cc51b0 
For Secure only builds on 53 there exists the Kconfig
CONFIG_SOC_ENABLE_LFXO to define if the XL1 and XL2 pin should be
configured to used for the LFXO oscillator. TF-M should have the same
behavior, to enable the possibility to use these pins for something else

[nrf noup] as we don't have the NCS Kconfigs available in upstream TF-M.
The CONFIG_ prefixed Kconfigs is made available in the noup commit:

Ref: NCSDK-20678
Ref: NCSDK-28740

Signed-off-by: Markus Swarowsky <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@vili-nordic @frkv
[nrf noup] Fix version warning
96fe5a5 
Version check depends on upstream's tagging scheme which differs
from NCS's

Signed-off-by: Vidar Lillebø <[email protected]>
@frkv
[nrf noup] platform: nordic_nrf: Add support shared UART and using UA…
94f544e 
…RT0 instance

Add support for selecting which UART instance to use as the secure UART
instance. The supported options are UART0 and UART1.

Add support for the secure UART instance being shared with the non-secure
application.
The UART instance is configured as non-secure after it has been
uninitialized, and configured as secure when it is initialized again
on a fatal error.

Note: device-specific target_cfg.h was provided here, which has been
dropped from the commit

Fixup: The spu_peripheral_config_(non_)secure calls takes the
ID of the peripheral as the argument and not the register
address.

Ref: NCSDK-18595
Ref: NCSDK-28740

Signed-off-by: Joakim Andersson <[email protected]>
(cherry picked from commit b2346e8)
Signed-off-by: Joakim Andersson <[email protected]>
(cherry picked from commit 97224b0)
Signed-off-by: Markus Swarowsky <[email protected]>
Change-Id: I2da826ec4817143ece52baeceaab14999f0d2d96
Signed-off-by: Markus Swarowsky <[email protected]>
(cherry picked from commit d2a1b89)
Signed-off-by: Markus Swarowsky <[email protected]>
Signed-off-by: Georgios Vasilakis <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
[nrf noup] platform: nordic_nrf: Add support for nRF54L
e0b5686 
Add support for nRF54L

Signed-off-by: Sebastian Bøe <[email protected]>
Signed-off-by: Joakim Andersson <[email protected]>
Signed-off-by: Markus Swarowsky <[email protected]>
Signed-off-by: Georgios Vasilakis <[email protected]>
Signed-off-by: Vidar Lillebø <[email protected]>
Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
[nrf noup] tf-m: Enable support for external PSA core
e637605 
-This commit adds support for externally built PSA core in TF-M
 by checking for the CMake variable (cached) PSA_CRYPTO_EXTERNAL_CORE.
 By setting this define, then a platform-target file called
 external_core.cmake as well as external_core_install.cmake is called
 to allow for the following:
 - Early include of necessary replacement include folders
 - Support for using generated configuration files for TF-M build
-This commit also tries to make psa_crypto_config and
 psa_crypto_library_config linked in first to ensure that certain
 folders are included as early as possible in the build

Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
WIP: TF-M: Fixing deprecated autoconf.h includes
abc2127 
-This changes includes from autoconf.h to zephyr/autoconf.h as the former
 has been deprecated

Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
WIP: Fixing the last remaining issues for L15
ccb7244 
This commit will be reworked

Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
[nrf noup] attestation: Prevent errors of multiple defs of ARRAY_LENGTH
4392e16 
-The macro ARRAY_LENGTH is defined without checking if there is already
 a definition. This commit can be reverted once the proposed fix
 is handled upstream
-This fixes ARRAY_LENGTH in s_io_sorage_tests.c

Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
WIP: Adding the previous methodology of using base addresses
6d6229a 
-The upstream code is using peripheral-ids, but is lacking
 the ability to resolve SPU entries for the peripheral.
 This WIP commit sets it back to the way it is in sdk-trusted-firmware-m
 prior to TF-M 2.1.0

Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
[nrf noup]: Add definitions for builtin key support
45ae077 
-This adds MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS and
 PSA_CRYPTO_DRIVER_TFM_BUILTIN_KEY to tfm_psa_rot_partition_crypto

Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@frkv
tf-m: Add missing include for generic configurations
d7a564d 
-Hopefully fixes TF-M shared UART issues

Signed-off-by: Frank Audun Kvamtrø <[email protected]>
@tomi-font @rlubos
Revert "[nrf noup] platform: nordic_nrf: 54l15: clean up RRAM write b…
0ec0fe8 
…uffer size setting"

This reverts commit 9f9b4fc.
@tomi-font @rlubos
[nrf noup] platform: nordic_nrf: make RRAM writes unbuffered
cc9a53f 
Temporarily set RRAMC.CONFIG.WRITEBUFSIZE to 0 to use unbuffered mode
when writing to RRAM in TF-M.
This is done to reduce the interrupt latency increases provoked when
writing to RRAM.
Do not set it permanently so that it remains
CONFIG_NRF_RRAM_WRITE_BUFFER_SIZE for when the NS image code writes
to RRAM.

Signed-off-by: Tomi Fontanilles <[email protected]>
hellesvik-nordic
hellesvik-nordic commented Nov 18, 2024
View reviewed changes
secure_fw/partitions/initial_attestation/attest_token_encode.c Outdated
@@ -241,6 +241,9 @@ attest_token_encode_start(struct attest_token_encode_ctx *me,
me->opt_flags = opt_flags;
me->key_select = key_select;

if (opt_flags & TOKEN_OPT_SIGN_MESSAGE) {
Copy link
Author

Choose a reason for hiding this comment

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

Should I make sure this is only added for nRF54L15, or should we use sign_message for all chips?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it makes sense to keep the default as is and not use sign_message.

But we need to have this as an option in TFM. We can sync with a call tomorrow or something and we can find a way to do that.

Vge0rge
Vge0rge reviewed Nov 18, 2024
View reviewed changes
lib/ext/t_cose/src/t_cose_sign1_sign.c Outdated Show resolved Hide resolved
lib/ext/t_cose/crypto_adapters/t_cose_psa_crypto.c Outdated Show resolved Hide resolved
lib/ext/t_cose/inc/t_cose_common.h Outdated Show resolved Hide resolved
lib/ext/t_cose/inc/t_cose_sign1_sign.h Outdated Show resolved Hide resolved
lib/ext/t_cose/src/t_cose_crypto.h Outdated Show resolved Hide resolved
secure_fw/partitions/initial_attestation/attest_token_encode.c Outdated
@@ -241,6 +241,9 @@ attest_token_encode_start(struct attest_token_encode_ctx *me,
me->opt_flags = opt_flags;
me->key_select = key_select;

if (opt_flags & TOKEN_OPT_SIGN_MESSAGE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it makes sense to keep the default as is and not use sign_message.

But we need to have this as an option in TFM. We can sync with a call tomorrow or something and we can find a way to do that.

@hellesvik-nordic hellesvik-nordic force-pushed the t_cose_add_message_sign branch 14 times, most recently from dc8b931 to 9c1a5ad Compare November 22, 2024 12:53
@NordicBuilder NordicBuilder mentioned this pull request Nov 22, 2024
Open
tomi-font
tomi-font reviewed Nov 25, 2024
View reviewed changes
lib/ext/t_cose/tfm_t_cose.cmake Outdated
Copy link
Contributor

@tomi-font tomi-font Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why are we making changes to lib/ext/t_cose as a noup while it exists upstream?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I will make a PR upstream instead, and then fix this PR accordingly.

Copy link
Author

Choose a reason for hiding this comment

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

Here is the upstream PR: https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/33713.
I will make this PR a draft so I can use it to test the changes for the upstream one.

@hellesvik-nordic hellesvik-nordic marked this pull request as draft December 4, 2024 08:23
@hellesvik-nordic hellesvik-nordic force-pushed the t_cose_add_message_sign branch 6 times, most recently from 98757cc to 4cdcc6e Compare December 6, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomi-font tomi-font tomi-font left review comments

@Vge0rge Vge0rge Vge0rge left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.