Modify PSA related error codes and types #55

davidsaada · 2019-02-13T13:03:45Z

PSA error codes and storage related types were changed in the PSA spec. So this PR:

Changes the values of PSA error codes according to PSA spec, eliminating ITS specific error codes.
Changes ITS related types to the more generic ones (psa_storage_*), as defined in the PSA spec.

Patater · 2019-02-13T16:35:04Z

include/psa/crypto_values.h


 /** An error occurred that does not correspond to any defined
 * failure cause.
 *
 * Implementations may use this error code if none of the other standard
 * error codes are applicable. */
-#define PSA_ERROR_UNKNOWN_ERROR         ((psa_status_t)1)
+#define PSA_ERROR_GENERIC_ERROR         ((psa_status_t)-132)


The allocated_errors.h header file says this error is defined in error.h. Should we move it to a new error.h file? The header seems to expect certain errors coming from headers with a certain name.

#define PSA_ERROR_GENERIC_ERROR ((psa_status_t)-132) /* Defined in <psa/error.h> */ /* ... */ #define PSA_ERROR_INVALID_PADDING ((psa_status_t)-150) /* Defined in <psa/crypto.h> */

CC @gilles-peskine-arm Are you aware of the expectations on PSA implementations here?

We're standardizing psa_status_t and a common set of status codes across PSA services, to be defined in a header file psa/error.h that all services provide and which Arm will manage centrally (so if you end up with several copies of psa/error.h, there's always a total order between versions and you can pick the most recent one). The publishing strategy is that error.h will be distributed by every service that uses it. See internal discussions at https://github.com/ARMmbed/PSA-IPC-doc/issues/133 and https://github.com/ARMmbed/PSA-IPC-doc/pull/177

I'm wondering if this commit should then grab and add that error.h file instead, leaving only crypto.h specified errors in crypto.h.

The second commit in this PR can update the ITS types.

Patater · 2019-02-13T16:36:57Z

library/psa_crypto_storage_its.c

-    psa_its_uid_t data_identifier = psa_its_identifier_of_slot( key );
-    struct psa_its_info_t data_identifier_info;
+    psa_status_t ret;
+    psa_storage_uid_t data_identifier = psa_its_identifier_of_slot( key );


This commit appears to be mixing ITS type updates with error status updates. Could you please make the identifier update changes in a separate commit?

Patater · 2019-02-13T17:15:01Z

This is failing the psa_constant_names test and either this PR attempts to introduce a bug, or we need to adapt the psa_constant_names test to accommodate this PR's changes.

gilles-peskine-arm

Please break this up into meaningful commits as requested by Jaeden.
This breaks the ITS build (psa_status_t is defined twice).
This reveals a pre-existing defect in psa_constant_names and test_psa_constant_names.py: they don't fully support negative constants. This is signalled by CI, so we need to fix it. Fix available in psa_constant_names: status is signed #56

gilles-peskine-arm · 2019-02-13T16:49:36Z

include/psa/crypto_values.h

- *
- * Note that psa_defs.h must be included before this header!
+/* PSA error codes */
+/* List should comply with the ones defined in https://github.com/ARMmbed/PSA-IPC-doc/blob/master/allocated_errors.h


This is a private link. Please do not include private links in published code or documentation.

gilles-peskine-arm · 2019-02-13T16:53:43Z

library/psa_crypto_storage_its.c

@@ -27,6 +27,7 @@

 #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

+#include "psa/error.h"


As long as psa/crypto_types.h (included via psa/crypto.h) defines psa_status_t, including psa/error.h as well will cause a compilation error due to the duplicate definition of psa_status_t. We either need to make the definition of psa_status_t conditional in psa/crypto_types.h or to start distributing an official version of psa/error.h.

Not sure why this would fail the build. Type definitions in both files are the same, meaning that the compiler should not fail this duplication. I build it this way in mbed-os without issues.
Is this an empirical or a theoretical problem? I ask because I only added psa/error.h to my current mbed-os PR, so if this is empirical, than where do you take psa/error.h from?

Theoretical in this instance, but empirical from a previous version. We had to add guards around the definition of psa_status_t so that it would only be defined in one place. You can have multiple #define for the same macro as long as they expand to the same token sequence, but some compilers warn about it. You can't have multiple typedef statements defining the same type name, even if the definitions are identical.

In your build, what exactly does psa/error.h contain? Who's defining psa_status_t and how do you avoid defining it twice?

psa/error.h is part of the PSA include directory. I'm adding it now to the parallel PR in mbed-os. Please note that psa_status_t is not a define but a typedef. As opposed to defines, compilers will not warn about duplicate typedefs as long as the referenced type is identical. In addition, as it's a typedef and not a define, one can't ask whether it's defined using #ifdef.

Duplicate definitions of a typedef are forbidden in C89 and C99. From N1256 (C99 TC3), §6.7 “Declarations”:

3. If an identifier has no linkage, there shall be no more than one declaration of the identifier
(in a declarator or type specifier) with the same scope and in the same name space, except
for tags as specified in 6.7.2.3.

Clang does warn about it if you ask it to use C89 or C99.

$ cat a.c typedef int foo; typedef int foo; $ clang -std=c99 -Wall -Wextra -Werror -O -c a.c a.c:2:13: error: redefinition of typedef 'foo' is a C11 feature [-Werror,-Wtypedef-redefinition] typedef int foo; ^ a.c:1:13: note: previous definition is here typedef int foo; ^ 1 error generated.

Neither Clang nor GCC warn about it in non-picky mode, but I remember we've hard this error in at least one of the CC+CFLAGS combination we test in our CI, and we do aim to stick to standard C in Mbed TLS and Mbed Crypto.

OK, understood (didn't go that far).
How do you suggest to resolve this? It's a bit of a pickle, as there's no #ifdef for typedefs. I could ask whether PSA_SUCCESS is defined, assuming that PSA_SUCCESS and psa_status_t are both defined in psa/error.h, but that just seems pretty awkward to me.

That's the hack we had for a long time, and getting rid of that hack was my motivation for requesting a unified psa/error.h (https://github.com/ARMmbed/PSA-IPC-doc/issues/133) as the only place where psa_status_t is defined. Crypto is planning to switch from defining psa_status_t in crypto_types.h to including error.h as soon as version 1 of error.h has stabilized.

OK, will add this hack.

davidsaada · 2019-02-13T20:14:23Z

* Please break this up into meaningful commits as requested by Jaeden.

* This breaks the ITS build (`psa_status_t` is defined twice).

* This reveals a pre-existing defect in `psa_constant_names` and `test_psa_constant_names.py`: they don't fully support negative constants. This is signalled by CI, so we need to fix it. Fix available in #56

Will do (will also remove the private link). It's still not clear whether I should move all error codes to a new error.h file or keep them under crypto_values.h

davidsaada · 2019-02-14T12:13:06Z

@Patater @gilles-peskine-arm Commits are now split to two: one for error codes, one for the change of ITS types to the generic types.
Error codes are still in crypto_values.h, as it seems you're not sure whether you want to keep them there or move them into a new error.h file. It can be done later on I guess.
All comments addressed except for the psa_status_t duplication.

davidsaada · 2019-02-14T13:32:34Z

Added the handling of psa_status_t duplication. All comments are addressed now.

Patater

Code LGTM

I think the commit message for "Change ITS specific types to the more generic ones (defined in PSA spec)" needs some work, though.

How about the following, but after you fill in xxx with the version of the ITS spec this PR updates to?

psa: Update types to version xxx of PSA ITS

This replaces psa_its_status_t with the more generic psa_status_t,
psa_its_info_t with the new psa_storage_info_t, and psa_its_uid_t with
the new psa_storage_uid_t. This is necessary in order to integrate with
the newer implementation of PSA ITS landing in Mbed OS soon.

davidsaada · 2019-02-14T20:35:26Z

Code LGTM

I think the commit message for "Change ITS specific types to the more generic ones (defined in PSA spec)" needs some work, though.

How about the following, but after you fill in xxx with the version of the ITS spec this PR updates to?
psa: Update types to version xxx of PSA ITS

This replaces psa_its_status_t with the more generic psa_status_t,
psa_its_info_t with the new psa_storage_info_t, and psa_its_uid_t with
the new psa_storage_uid_t. This is necessary in order to integrate with
the newer implementation of PSA ITS landing in Mbed OS soon.

Cool. Commit message modified in the spirit of your recommendation. Didn't see any change to the PSA spec version though, so left this one out.

Patater · 2019-02-15T10:02:56Z

Sorry for the nitpicking, but it looks like you forgot to mention

- psa_storage_uid_t replaces psa_its_uid_t

Also, please don't end commit messages with a : or ., as per our git commit style guide

Thanks

davidsaada · 2019-02-15T10:09:03Z

Sorry for the nitpicking, but it looks like you forgot to mention
- psa_storage_uid_t replaces psa_its_uid_t
Also, please don't end commit messages with a : or ., as per our git commit style guide

Thanks

This ain't nitpicking, it was a mistake I made there. Fixed now.

Patater

LGTM

davidsaada · 2019-02-15T12:32:01Z

@Patater Thanks for approving. Can you also approve PR #9708 in the mbed-os repo? It's the corresponding PR in mbed-os, including the same mbed-crypto changes.

gilles-peskine-arm

The numerical values and double-definition guards are ok. Validated with a checkout of the private repository containing the PSA internal allocte error list at ../../PSA-IPC-doc and running

printf '#include <%s.h>\n' allocated_errors error crypto_types crypto_values | cpp -I include/psa -I ../../PSA-IPC-doc/include/psa -I ../../PSA-IPC-doc >/dev/null
printf '#include <%s.h>\n' crypto_types crypto_values allocated_errors error | cpp -I include/psa -I ../../PSA-IPC-doc/include/psa -I ../../PSA-IPC-doc >/dev/null

This pull request keeps some old names and gives them values that don't seem to come from anywhere. Having not-yet-renamed error codes is ok, but some errors are duplicated instead of renamed (EMPTY_SLOT and DOES_NOT_EXIST, OCCUPIED_SLOT and ALREADY_EXISTS), and some errors have values that may clash with values that others are using. Both problems aren't problems for the crypto library itself but they are problems for users of the crypto library.

gilles-peskine-arm · 2019-02-15T16:58:31Z

include/psa/crypto_values.h


 /** A slot is occupied, but must be empty to carry out the
 * requested action.
 *
 * If a handle is invalid, it does not designate an occupied slot.
 * The error for an invalid handle is #PSA_ERROR_INVALID_HANDLE.
 */
-#define PSA_ERROR_OCCUPIED_SLOT         ((psa_status_t)5)
+#define PSA_ERROR_OCCUPIED_SLOT         ((psa_status_t)-153)


PSA_ERROR_OCCUPIED_SLOT is to be renamed to PSA_ERROR_ALREADY_EXISTS from the base list.

gilles-peskine-arm · 2019-02-15T16:59:01Z

include/psa/crypto_values.h


 /** A slot is empty, but must be occupied to carry out the
 * requested action.
 *
 * If a handle is invalid, it does not designate an empty slot.
 * The error for an invalid handle is #PSA_ERROR_INVALID_HANDLE.
 */
-#define PSA_ERROR_EMPTY_SLOT            ((psa_status_t)6)
+#define PSA_ERROR_EMPTY_SLOT            ((psa_status_t)-154)


PSA_ERROR_EMPTY_SLOT is to be renamed to PSA_ERROR_DOES_NOT_EXIST from the base list.

gilles-peskine-arm · 2019-02-15T16:59:46Z

include/psa/crypto_values.h


 /** The generator has insufficient capacity left.
 *
 * Once a function returns this error, attempts to read from the
 * generator will always return this error. */
-#define PSA_ERROR_INSUFFICIENT_CAPACITY ((psa_status_t)18)
+#define PSA_ERROR_INSUFFICIENT_CAPACITY ((psa_status_t)-155)


PSA_ERROR_INSUFFICIENT_CAPACITY is to be renamed to PSA_ERROR_INSUFFICIENT_DATA from the base list.

davidsaada · 2019-02-15T20:04:47Z

Thanks @gilles-peskine-arm. Wasn't sure about these error codes myself. Replaced them according to your suggestion.

Patater

LGTM

gilles-peskine-arm

Blocker: please update psa_its_status_t and PSA_ITS_xxx in tests/suites/test_suite_psa_crypto_entropy.function as well. Make sure it's tested before merging this PR.

While you're at it, please update the comment in test_suite_psa_crypto.function.

gilles-peskine-arm · 2019-02-18T10:53:26Z

tests/suites/test_suite_psa_crypto.function

@@ -422,7 +422,7 @@ static psa_status_t key_agreement_with_self( psa_crypto_generator_t *generator,
    /* Return UNKNOWN_ERROR if something other than the final call to


Comment needs updating

gilles-peskine-arm · 2019-02-18T10:57:45Z

include/psa/crypto_values.h

- * The error for an invalid handle is #PSA_ERROR_INVALID_HANDLE.
- */
-#define PSA_ERROR_OCCUPIED_SLOT         ((psa_status_t)5)
+ * Implementations should return this error, when attempting


The descriptions of PSA_ERROR_ALREADY_EXISTS, PSA_ERROR_DOES_NOT_EXIST and PSA_ERROR_INSUFFICIENT_DATA need work (grammaticality, specific meaning for crypto) but this is not a blocker, I can fix it later.

PSA spec now defines more generic PSA storage types instead of the ITS specific ones. This is necessary in order to integrate with the newer implementation of PSA ITS landing in Mbed OS soon. Changes include the following: - psa_status_t replaces psa_its_status_t - psa_storage_info_t replaces psa_its_info_t - psa_storage_uid_t replaces psa_its_uid_t

davidsaada · 2019-02-18T11:59:47Z

@gilles-peskine-arm Pushed a fix according to your comments. Left the descriptions for the new error codes out (thought it would be better if an mbed crypto specialist would do that).

gilles-peskine-arm

Looks fine.

Approved provided that this has been tested (including running test_suite_psa_crypto_entropy) on Mbed OS in configurations with ITS with and without entropy injection. @Patater Is this tested on CI at the moment?

davidsaada · 2019-02-18T12:25:25Z

Looks fine.

Approved provided that this has been tested (including running test_suite_psa_crypto_entropy) on Mbed OS in configurations with ITS with and without entropy injection. @Patater Is this tested on CI at the moment?

Thanks for the approval.
As for the tests - I currently test it locally, but regarding CI, changes aren't currently tested yet. There's a pending PR in mbed-os, which includes these changes as well. This PR awaits your approval in order to start CI. ~~I'll copy the latest changes from here to there and then will appreciate your quick approval for it.~~ Changes from here copied there - again temporarily, just in order to start moving the CI chains.

Patater · 2019-02-18T15:22:18Z

@Patater Is this tested on CI at the moment?

No, otherwise we'd have seen a failure in CI.

davidsaada · 2019-02-18T15:33:34Z

@gilles-peskine-arm @Patater Sorry for nagging, but can you approve PR #9708 in mbed-os? The mbed-crypto part changes there are identical to the ones in this PR (they are all concentrated in the second commit). They will be discarded before that PR is merged.

Patater · 2019-02-18T15:36:46Z

Travis failure is DTLS 3d Proxy. Jenkins failure is FreeBSD timing test. Both are known issues.

Pre-merge test failed because this PR is not based on a version of development recent enough to have psa_constant_names that handles signed error codes (#56).

davidsaada changed the title ~~Modify PSA related error codes and types:~~ Modify PSA related error codes and types Feb 13, 2019

davidsaada mentioned this pull request Feb 13, 2019

Implement PSA protected storage & restructure PSA storage implementation ARMmbed/mbed-os#9708

Merged

davidsaada force-pushed the david_its_ps_err_codes branch from 7361243 to 00d44c7 Compare February 13, 2019 16:18

Patater suggested changes Feb 13, 2019

View reviewed changes

gilles-peskine-arm requested changes Feb 13, 2019

View reviewed changes

gilles-peskine-arm mentioned this pull request Feb 13, 2019

psa_constant_names: status is signed #56

Merged

davidsaada force-pushed the david_its_ps_err_codes branch from 00d44c7 to 4d79d88 Compare February 14, 2019 11:55

davidsaada force-pushed the david_its_ps_err_codes branch from 4d79d88 to 089daeb Compare February 14, 2019 13:31

Patater reviewed Feb 14, 2019

View reviewed changes

davidsaada force-pushed the david_its_ps_err_codes branch from 089daeb to 1793d8a Compare February 14, 2019 20:33

davidsaada force-pushed the david_its_ps_err_codes branch from 1793d8a to 1e7b27c Compare February 15, 2019 10:08

Patater previously approved these changes Feb 15, 2019

View reviewed changes

Patater added needs: review The pull request is ready for review. This generally means that it has no known issues. compliance Discrepancy between the library and the PSA Crypto API specification labels Feb 15, 2019

gilles-peskine-arm requested changes Feb 15, 2019

View reviewed changes

davidsaada dismissed Patater’s stale review via d19a7b7 February 15, 2019 20:03

davidsaada force-pushed the david_its_ps_err_codes branch from 1e7b27c to d19a7b7 Compare February 15, 2019 20:03

Patater previously approved these changes Feb 18, 2019

View reviewed changes

gilles-peskine-arm requested changes Feb 18, 2019

View reviewed changes

David Saada added 2 commits February 18, 2019 13:53

Replace PSA error code definitions with the ones defined in PSA spec

b4ecc27

davidsaada dismissed Patater’s stale review via a2523b2 February 18, 2019 11:57

davidsaada force-pushed the david_its_ps_err_codes branch from d19a7b7 to a2523b2 Compare February 18, 2019 11:57

gilles-peskine-arm approved these changes Feb 18, 2019

View reviewed changes

Patater approved these changes Feb 18, 2019

View reviewed changes

Patater merged commit 9654e11 into ARMmbed:development Feb 18, 2019

Patater removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Feb 18, 2019

This was referenced Apr 12, 2019

Merge development into API spec branch #92

Merged

[Warning] psa_client.h@25,0: "PSA_SUCCESS" redefined ARMmbed/mbed-os#10251

Closed

		@@ -27,6 +27,7 @@

		#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

		#include "psa/error.h"

		@@ -422,7 +422,7 @@ static psa_status_t key_agreement_with_self( psa_crypto_generator_t *generator,
		/* Return UNKNOWN_ERROR if something other than the final call to

Modify PSA related error codes and types #55

Modify PSA related error codes and types #55

Uh oh!

Conversation

davidsaada commented Feb 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater Feb 13, 2019 • 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!

Patater commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm left a comment • 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!

davidsaada commented Feb 13, 2019

Uh oh!

davidsaada commented Feb 14, 2019

Uh oh!

davidsaada commented Feb 14, 2019

Uh oh!

Patater left a comment • 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!

davidsaada commented Feb 14, 2019

Uh oh!

Patater commented Feb 15, 2019

Uh oh!

davidsaada commented Feb 15, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

davidsaada commented Feb 15, 2019

Uh oh!

gilles-peskine-arm 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!

davidsaada commented Feb 15, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Patater Feb 13, 2019 •

edited

Loading

Patater commented Feb 13, 2019 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading

Patater left a comment •

edited

Loading

davidsaada commented Feb 18, 2019 •

edited

Loading