sys/psa_crypto: Fix build problems by Einhornhool · Pull Request #19992 · RIOT-OS/RIOT

Einhornhool · 2023-10-19T11:48:12Z

Contribution description

This fixes several problems:

1. Empty union in cipher context when `MODULE_PSA_CIPHER` is not selected.

PSA operations are now separated into modules. Functions and contexts are only built when the corresponding module is selected. This way there won't be problems with missing or unitialized structures in unused modules anymore.

2. Zero-size array when using secure elements and `PSA_MAX_KEY_DATA_SIZE == 0`

I added a condition to the psa_key_slot_t structure in psa_key_slot_management.h.
Also the existence of key slot management functions and key slot structures now depends on the number of allocated key slots instead of selected modules.
This way key structures will not exist unless they are used.

Testing procedure

Add the following to examples/hello_world/Makefile and call make :

USEMODULE += psa_crypto
USEMODULE += psa_hash
USEMODULE += psa_hash_sha_256
USEMODULE += psa_secure_element

Output on Master:

"make" -C /home/lena/work/RIOT/boards/common/init
"make" -C /home/lena/work/RIOT/boards/native
"make" -C /home/lena/work/RIOT/boards/native/drivers
"make" -C /home/lena/work/RIOT/core
"make" -C /home/lena/work/RIOT/core/lib
"make" -C /home/lena/work/RIOT/cpu/native
"make" -C /home/lena/work/RIOT/cpu/native/periph
"make" -C /home/lena/work/RIOT/cpu/native/stdio_native
"make" -C /home/lena/work/RIOT/drivers
"make" -C /home/lena/work/RIOT/drivers/periph_common
"make" -C /home/lena/work/RIOT/sys
"make" -C /home/lena/work/RIOT/sys/auto_init
"make" -C /home/lena/work/RIOT/sys/libc
"make" -C /home/lena/work/RIOT/sys/luid
"make" -C /home/lena/work/RIOT/sys/preprocessor
"make" -C /home/lena/work/RIOT/sys/psa_crypto
In file included from /home/lena/work/RIOT/sys/include/psa_crypto/psa/crypto.h:39,
                 from /home/lena/work/RIOT/sys/psa_crypto/psa_crypto_algorithm_dispatch.c:23:
/home/lena/work/RIOT/sys/include/psa_crypto/psa/crypto_struct.h:137:11: error: union has no members [-Werror=pedantic]
  137 |     union cipher_context {
      |           ^~~~~~~~~~~~~~
In file included from /home/lena/work/RIOT/sys/psa_crypto/include/psa_crypto_operation_encoder.h:32,
                 from /home/lena/work/RIOT/sys/psa_crypto/psa_crypto_algorithm_dispatch.c:28:
/home/lena/work/RIOT/sys/psa_crypto/include/psa_crypto_slot_management.h:82:17: error: ISO C forbids zero-size array ‘data’ [-Werror=pedantic]
   82 |         uint8_t data[PSA_MAX_KEY_DATA_SIZE];    /**< Key data buffer */
      |                 ^~~~
cc1: all warnings being treated as errors
make[3]: *** [/home/lena/work/RIOT/Makefile.base:146: /home/lena/work/RIOT/examples/hello-world/bin/native/psa_crypto/psa_crypto_algorithm_dispatch.o] Error 1
make[2]: *** [/home/lena/work/RIOT/Makefile.base:31: ALL--/home/lena/work/RIOT/sys/psa_crypto] Error 2
make[1]: *** [/home/lena/work/RIOT/Makefile.base:31: ALL--/home/lena/work/RIOT/sys] Error 2
make: *** [/home/lena/work/RIOT/examples/hello-world/../../Makefile.include:761: application_hello-world.module] Error 2

Output with fixes:

"make" -C /home/lena/work/RIOT/boards/common/init
"make" -C /home/lena/work/RIOT/boards/native
"make" -C /home/lena/work/RIOT/boards/native/drivers
"make" -C /home/lena/work/RIOT/core
"make" -C /home/lena/work/RIOT/core/lib
"make" -C /home/lena/work/RIOT/cpu/native
"make" -C /home/lena/work/RIOT/cpu/native/periph
"make" -C /home/lena/work/RIOT/cpu/native/stdio_native
"make" -C /home/lena/work/RIOT/drivers
"make" -C /home/lena/work/RIOT/drivers/periph_common
"make" -C /home/lena/work/RIOT/sys
"make" -C /home/lena/work/RIOT/sys/auto_init
"make" -C /home/lena/work/RIOT/sys/libc
"make" -C /home/lena/work/RIOT/sys/luid
"make" -C /home/lena/work/RIOT/sys/preprocessor
"make" -C /home/lena/work/RIOT/sys/psa_crypto
"make" -C /home/lena/work/RIOT/sys/psa_crypto/psa_key_slot_mgmt
"make" -C /home/lena/work/RIOT/sys/psa_crypto/psa_se_mgmt
"make" -C /home/lena/work/RIOT/sys/random
/usr/bin/ld: warning: /home/lena/work/RIOT/examples/hello-world/bin/native/hello-world.elf has a LOAD segment with RWX permissions
   text	   data	    bss	    dec	    hex	filename
  29764	    584	  47856	  78204	  1317c	/home/lena/work/RIOT/examples/hello-world/bin/native/hello-world.elf

riot-ci · 2023-10-20T12:55:51Z

Murdock results

✔️ PASSED

98e9016 tests/sys/psa_crypto_*: Update BOARD_INSUFFICIENT_MEMORY

Success	Failures	Total	Runtime
8082	0	8082	11m:51s

Artifacts

Documentation preview

MrKevinWeiss

Though I am not really in the PSA field I think it looks OK, and this does fix known issues. only nitpick comments there... Maybe it would be good to post the configurations that were tested. If anyone else wants to go more in-depth that would be appreciated but it would be nice to get this backported for the release. ping @Teufelchen1 maybe?

MrKevinWeiss · 2023-10-23T08:58:27Z

pkg/cryptoauthlib/Makefile.include

@@ -29,5 +29,7 @@ endif

 ifneq (,$(filter psa_crypto,$(USEMODULE)))


I actually don't think we need the ifneq for PSEUDOMODULES, this can be done in another PR but also... there is a lot of cleanup happening anyway.

MrKevinWeiss · 2023-10-23T09:24:44Z

sys/include/psa_crypto/psa/crypto_struct.h

-/**
- * @brief   Structure containing a hash context and algorithm
- */
-struct psa_hash_operation_s {
-    psa_algorithm_t alg;        /**< Operation algorithm */
-#if IS_USED(MODULE_PSA_HASH)
-    psa_hash_context_t ctx;     /**< Operation hash context */
-#endif
-};
-
-/**
- * @brief   This macro returns a suitable initializer for a hash operation object of type
- *          @ref psa_hash_operation_t.
- */
-#define PSA_HASH_OPERATION_INIT { 0 }
-
-/**
- * @brief   Return an initial value for a hash operation object.
- *
- * @return  struct psa_hash_operation_s
- */
-static inline struct psa_hash_operation_s psa_hash_operation_init(void)
-{
-    const struct psa_hash_operation_s v = PSA_HASH_OPERATION_INIT;
-
-    return v;
-}
-


Did you need to move this chunk below? Less changes that better (at least from the review perspective).

I wanted to sort them by alphabet, but of course I can change this back

I think it makes sense to maintain the new order to only have one continuous block of hash-related things to be guarded by the if-directive.

mguetschow · 2023-10-23T17:05:09Z

I get a lot of compile errors with the following configuration (copied from the initial post without the secure element):

USEMODULE += psa_crypto
USEMODULE += psa_hash
USEMODULE += psa_hash_sha_256

psa/crypto.h uses struct psa_key_attributes_s which is dependent on MODULE_PSA_KEY_MANAGEMENT.

Maybe the functions in psa/crypto.h should also be guarded by #ifs, following the example of psa_crypto.c? That would also prevent users to reference a function that is not compiled (i.e. of which the respective module is not selected).

mguetschow

Here a somewhat more in-depth review. As discussed (off-thread) before, I am in favor of the suggested changes which compile more code only on-demand, hidden behind a small set of PSA_<FEATURE> module flags, which are quite easy to reason about.

The example of the failed compilation above is just another pointer to the fact that we definitely need some sort of configuration tests in a future PR.

mguetschow · 2023-10-23T16:24:34Z

sys/psa_crypto/Makefile.dep

-  USEMODULE += psa_key_slot_mgmt
+  USEMODULE += psa_key_management


Why do we have both psa_key_slot_mgmt and psa_key_management now? Seems to me like the latter is only sort of an alias for the former?

It's not really the same. I separate between the PSA key management functions which are described by the specification and are exposed to the user in psa_crypto.c and psa/crypto.h vs. the key slot management module, which contains the whole key slot logic and is its own backend module.

They are dependent on each other, so if you build the key management functions, the key slot management is built, but they are separate things.

mguetschow · 2023-10-23T16:33:17Z

sys/psa_crypto/doc.txt

 * your sourcefiles, the module name will be the same as the directory name. It is possible
 * to change that by declaring a new module name in the Makefile by adding the line
- * your_module_name`.
+ * `MODULE := your_module_name`.


Kind of unrelated here, but while reading this I was wondering if there is more information about how paths are matched to module names in RIOT. Is it path/to/module => path_to_module in general? Maybe linking to further documentation, if existing, or giving an example might be worth it.

It is indirectly mentioned here, but I haven't found a more detailed explanation, yet. I kind of figured this out on my own by using modules and submodules.

according to @miri64 and how I understand the wiki, it's actually path/to/module => module (just for future reference)

mguetschow · 2023-10-23T16:35:59Z

sys/psa_crypto/doc.txt

 * @endcode
+ * This needs to be done for all other supported operations (e.g. ATECCX08 operations in
+ * `pkg/cryptoauthlib/Makefile.include`, `pkg/cryptoauthlib/Makefile.dep` and
+ * `sys/psa_crypto/psa_se_mgmt/Kconfig` Now the secure element should be available for use


Suggested change

* `sys/psa_crypto/psa_se_mgmt/Kconfig` Now the secure element should be available for use

* `sys/psa_crypto/psa_se_mgmt/Kconfig`. Now the secure element should be available for use

mguetschow · 2023-10-23T16:46:35Z

sys/psa_crypto/Kconfig.keys

 config PSA_SINGLE_KEY_COUNT
    int "Specifies number of allocated single key slots"
-    default 5 if PSA_MAX_KEY_SIZE != 0
+    default 5 if MODULE_PSA_KEY_SLOT_MGMT


in crypto_sizes.h, you've changed the default to 0 (same as for PSA_ASYMMETRIC_KEYPAIR_COUNT).

mguetschow · 2023-10-23T16:51:50Z

sys/psa_crypto/psa_crypto.c

    unlock_status = psa_unlock_key_slot(slot);
    return ((status == PSA_SUCCESS) ? unlock_status : status);
 }
+#endif /* MODULE_PSA_ASYMMETRIC */


Maybe not something for this PR, but since the CI is complaining about the long file: Should we maybe split the implementation files psa_crypto.c and also psa_crypto_{algorithm,location}_dispatch.c into smaller files, each only covering the implementation of one psa module such as MODULE_PSA_ASYMMETRIC? Since we have them already separated in blocks with the #if directives.

I have thought about it at one point and I am totally not opposed to it. But you're right, it's a thing for the future.

Tracked in #20014

mguetschow · 2023-10-23T16:53:47Z

sys/psa_crypto/psa_key_slot_mgmt/Kconfig


 config MODULE_PSA_KEY_SLOT_MGMT
    bool
+    default y if MODULE_PSA_KEY_MANAGEMENT


Shouldn't maybe MODULE_PSA_KEY_MANAGEMENT depend on MODULE_PSA_KEY_SLOT_MGMT instead? (If we want to keep the two aliases (?))

mguetschow · 2023-10-23T16:57:31Z

sys/psa_crypto/psa_key_slot_mgmt/Kconfig

 config MODULE_PSA_KEY_SLOT_MGMT
    bool
+    default y if MODULE_PSA_KEY_MANAGEMENT
    default y if PACKAGE_PSA_ARCH_TESTS


Does this package actually exist?

Einhornhool · 2023-11-20T15:22:51Z

@MrKevinWeiss I don't really understand that error thrown by Murdock. Is this something I need to take care of?

For context: I made another rebase, could that be the problem?

MrKevinWeiss · 2023-11-21T06:33:02Z

It is a unrelated issue I think. Murdock has been acting strange.

MrKevinWeiss · 2023-11-28T10:48:15Z

Looks like you need to add new boards (at least the stm32f030f4-demo to the psa_crypto_ecdasa and probably all but hashes... This moving target is really an issue. Worst case just whitelist boards instead.

benpicco · 2023-11-28T12:04:21Z

You can also run

make generate-Makefile.ci

MrKevinWeiss · 2023-11-30T19:50:58Z

whyyyyyyy!

MrKevinWeiss · 2023-12-01T08:16:34Z

Finally! Well done @Einhornhool

mguetschow · 2023-12-01T09:05:25Z

whyyyyyyy!

Sorry that was me and the SHA512 support I guess :P congrats everyone that we have it in now!

Einhornhool requested review from MrKevinWeiss, PeterKietzmann, jia200x and leandrolanzieri as code owners October 19, 2023 11:48

github-actions bot added Area: doc Area: Documentation Area: pkg Area: External package ports Area: sys Area: System Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Oct 19, 2023

Einhornhool added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) and removed Area: doc Area: Documentation Area: pkg Area: External package ports Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Oct 19, 2023

Einhornhool changed the title ~~Fix PSA Crypto Bugs~~ sys/psa_crypto: Fix build problems and Oct 19, 2023

Einhornhool changed the title ~~sys/psa_crypto: Fix build problems and~~ sys/psa_crypto: Fix build problems Oct 19, 2023

Einhornhool force-pushed the dev/psa-crypto-fixes branch from 8ef66db to 9473ef2 Compare October 19, 2023 12:30

github-actions bot added Area: doc Area: Documentation Area: pkg Area: External package ports Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Oct 19, 2023

Einhornhool force-pushed the dev/psa-crypto-fixes branch 2 times, most recently from e669c7d to 52a7186 Compare October 19, 2023 14:56

MrKevinWeiss added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 20, 2023

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 20, 2023

MrKevinWeiss reviewed Oct 23, 2023

View reviewed changes

mguetschow requested changes Oct 23, 2023

View reviewed changes

mguetschow mentioned this pull request Oct 24, 2023

psa_crypto: split implementation files #20014

Open

Einhornhool force-pushed the dev/psa-crypto-fixes branch 2 times, most recently from d504a55 to fe25fe1 Compare November 20, 2023 14:59

Einhornhool mentioned this pull request Nov 21, 2023

sys/psa_crypto: Implement persistent key storage #20099

Merged

benpicco added this pull request to the merge queue Nov 27, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 28, 2023

MrKevinWeiss force-pushed the dev/psa-crypto-fixes branch from fe25fe1 to 217707b Compare November 28, 2023 17:28

MrKevinWeiss enabled auto-merge November 28, 2023 18:06

MrKevinWeiss added this pull request to the merge queue Nov 28, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 28, 2023

MrKevinWeiss force-pushed the dev/psa-crypto-fixes branch from 217707b to 03f2bb9 Compare November 29, 2023 11:47

MrKevinWeiss enabled auto-merge November 29, 2023 11:48

MrKevinWeiss added this pull request to the merge queue Nov 29, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023

MrKevinWeiss force-pushed the dev/psa-crypto-fixes branch from 03f2bb9 to a4845bd Compare November 30, 2023 12:06

MrKevinWeiss enabled auto-merge November 30, 2023 12:06

MrKevinWeiss added this pull request to the merge queue Nov 30, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 30, 2023

Einhornhool and others added 2 commits November 30, 2023 21:02

sys/psa_crypto: Build PSA Crypto functions based on module selection

de09b2a

tests/sys/psa_crypto_*: Update BOARD_INSUFFICIENT_MEMORY

98e9016

MrKevinWeiss mentioned this pull request Dec 1, 2023

AVR-8: Implement Power Management #19784

Merged

		@@ -29,5 +29,7 @@ endif

		ifneq (,$(filter psa_crypto,$(USEMODULE)))

		USEMODULE += psa_key_slot_mgmt
		USEMODULE += psa_key_management

	* `sys/psa_crypto/psa_se_mgmt/Kconfig` Now the secure element should be available for use
	* `sys/psa_crypto/psa_se_mgmt/Kconfig`. Now the secure element should be available for use

Conversation

Einhornhool commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

1. Empty union in cipher context when MODULE_PSA_CIPHER is not selected.

2. Zero-size array when using secure elements and PSA_MAX_KEY_DATA_SIZE == 0

Testing procedure

Uh oh!

riot-ci commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

MrKevinWeiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mguetschow commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Einhornhool Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Einhornhool commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrKevinWeiss commented Nov 21, 2023

Uh oh!

Uh oh!

MrKevinWeiss commented Nov 28, 2023

Uh oh!

benpicco commented Nov 28, 2023

Uh oh!

Uh oh!

Uh oh!

MrKevinWeiss commented Nov 30, 2023

Uh oh!

Einhornhool commented Oct 19, 2023 •

edited

Loading

1. Empty union in cipher context when `MODULE_PSA_CIPHER` is not selected.

2. Zero-size array when using secure elements and `PSA_MAX_KEY_DATA_SIZE == 0`

riot-ci commented Oct 20, 2023 •

edited

Loading

mguetschow commented Oct 23, 2023 •

edited

Loading

Einhornhool Oct 24, 2023 •

edited

Loading

Einhornhool commented Nov 20, 2023 •

edited

Loading