gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

[2/3] Unify generic error codes to PSA equivalents #422

Closed
felixc-arm wants to merge 13 commits from unify-error-codes into development
pull from: unify-error-codes
merge into: gh-Mbed-TLS:development
Conversation 6 Commits 13 Files changed 34 +220 -171
felixc-arm commented 2025-08-18 04:13:43 -05:00 (Migrated from github.com)

Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/330

Dependency Chain:
[1/3] https://github.com/Mbed-TLS/mbedtls/pull/10363
[2/3] This PR
[3/3] https://github.com/Mbed-TLS/mbedtls/pull/10369

Parameterized job with https://github.com/Mbed-TLS/mbedtls/pull/10363 & this PR passing here: https://mbedtls.trustedfirmware.org/blue/organizations/jenkins/mbed-tls-restricted-pr-test-parametrized/detail/mbed-tls-restricted-pr-test-parametrized/189

Replaces MEDTLS_ERR_xxx codes with PSA_ERROR_xxx codes where the MBEDTLS_ERR codes are generic/share the same function, but are from a different module. (e.g. MBEDTLS_ERR_xxx_ALLOC_FAILED)

If the error codes were found in private headers, they were just replaced with the PSA equivalent, but if they were in public headers, then they were moved to a new header file (include/psa/crypto_unified_errors.h) and any mention of the old MBEDTLS error code was replaced in public documentation by the new PSA error code. Also adds compat-3.h which includes this new file for users who want to include the old error aliases in their programs.

n.b. I was fairly conservative with the codes and so there are probably more that could be replaced, I just stuck to the following which clearly had duplicates across modules - If there are any more that reviewers think should be done in this PR then let me know:
MBEDTLS_ERR_xxx_BAD_INPUT_DATA / BAD_INPUT -> PSA_ERROR_INVALID_ARGUMENT
MBEDTLS_ERR_xxx_ALLOC_FAILED -> PSA_ERROR_INSUFFICIENT_MEMORY
MBEDTLS_ERR_xxx_AUTH_FAILED / VERIFY_FAILED -> PSA_ERROR_INVALID_SIGNATURE
MBEDTLS_ERR_xxx_BUFFER_TOO_SMALL / OUTPUT_TOO_LARGE -> PSA_ERROR_BUFFER_TOO_SMALL
MBEDTLS_ERR_xxx_INVALID_PADDING -> PSA_ERROR_INVALID_PADDING

PR checklist

  • changelog provided
  • framework PR not required
  • mbedtls development PR provided: https://github.com/Mbed-TLS/mbedtls/pull/10363
  • mbedtls 3.6 PR not required because: 4.0/1.0 API change
  • tests not required because: no functional change
Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/330 Dependency Chain: [1/3] https://github.com/Mbed-TLS/mbedtls/pull/10363 [2/3] This PR [3/3] https://github.com/Mbed-TLS/mbedtls/pull/10369 Parameterized job with https://github.com/Mbed-TLS/mbedtls/pull/10363 & this PR passing here: https://mbedtls.trustedfirmware.org/blue/organizations/jenkins/mbed-tls-restricted-pr-test-parametrized/detail/mbed-tls-restricted-pr-test-parametrized/189 Replaces `MEDTLS_ERR_xxx` codes with `PSA_ERROR_xxx` codes where the `MBEDTLS_ERR` codes are generic/share the same function, but are from a different module. (e.g. `MBEDTLS_ERR_xxx_ALLOC_FAILED`) If the error codes were found in private headers, they were just replaced with the `PSA` equivalent, but if they were in public headers, then they were moved to a new header file (`include/psa/crypto_unified_errors.h`) and any mention of the old `MBEDTLS` error code was replaced in public documentation by the new `PSA` error code. Also adds `compat-3.h` which includes this new file for users who want to include the old error aliases in their programs. n.b. I was fairly conservative with the codes and so there are probably more that could be replaced, I just stuck to the following which clearly had duplicates across modules - If there are any more that reviewers think should be done in this PR then let me know: `MBEDTLS_ERR_xxx_BAD_INPUT_DATA / BAD_INPUT` -> `PSA_ERROR_INVALID_ARGUMENT` `MBEDTLS_ERR_xxx_ALLOC_FAILED` -> `PSA_ERROR_INSUFFICIENT_MEMORY` `MBEDTLS_ERR_xxx_AUTH_FAILED / VERIFY_FAILED` -> `PSA_ERROR_INVALID_SIGNATURE` `MBEDTLS_ERR_xxx_BUFFER_TOO_SMALL / OUTPUT_TOO_LARGE` -> `PSA_ERROR_BUFFER_TOO_SMALL` `MBEDTLS_ERR_xxx_INVALID_PADDING` -> `PSA_ERROR_INVALID_PADDING` ## PR checklist - [ ] **changelog** provided - [ ] **framework PR** not required - [ ] **mbedtls development PR** provided: https://github.com/Mbed-TLS/mbedtls/pull/10363 - [ ] **mbedtls 3.6 PR** not required because: 4.0/1.0 API change - **tests** not required because: no functional change
davidhorstmann-arm (Migrated from github.com) requested changes 2025-08-27 11:20:47 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment

One very small nit and a question about md.h, but otherwise looks good.

The error codes that were previously translated differently are strange, but I don't think they are problems and I've noted them just to have a record somewhere.

One very small nit and a question about `md.h`, but otherwise looks good. The error codes that were previously translated differently are strange, but I don't think they are problems and I've noted them just to have a record somewhere.
core/psa_crypto.c
@ -403,4 +375,3 @@
return PSA_ERROR_INVALID_SIGNATURE;
case MBEDTLS_ERR_CIPHER_INVALID_CONTEXT:
return PSA_ERROR_CORRUPTION_DETECTED;
#endif
davidhorstmann-arm (Migrated from github.com) commented 2025-08-27 10:54:21 -05:00

Strange that this one was converted to PSA_ERROR_NOT_SUPPORTED previously - I wonder why that was.

Strange that this one was converted to `PSA_ERROR_NOT_SUPPORTED` previously - I wonder why that was.
drivers/builtin/include/mbedtls/md.h
@ -29,3 +28,4 @@
#define MBEDTLS_ERR_MD_ALLOC_FAILED PSA_ERROR_INSUFFICIENT_MEMORY
/** Opening or reading of file failed. */
#define MBEDTLS_ERR_MD_FILE_IO_ERROR -0x5200
davidhorstmann-arm (Migrated from github.com) commented 2025-08-27 10:50:03 -05:00

Unless I'm mistaken I think some part of md.h is staying public (hence why this header is not yet private). Would it be better to define this alias in crypto_unified_errors.h?

Unless I'm mistaken I think some part of `md.h` is staying public (hence why this header is not yet private). Would it be better to define this alias in `crypto_unified_errors.h`?
davidhorstmann-arm (Migrated from github.com) commented 2025-08-27 11:16:50 -05:00

Actually it looks like we are planning to keep this part of md.h public in 4.0, would it still be worth removing the error codes, or should we keep them?

Actually it [looks like](https://github.com/Mbed-TLS/mbedtls/issues/8450#issuecomment-3012545419) we are planning to keep this part of `md.h` public in 4.0, would it still be worth removing the error codes, or should we keep them?
drivers/builtin/src/psa_crypto_pake.c
@ -103,15 +103,15 @@
#if defined(MBEDTLS_PSA_BUILTIN_ALG_JPAKE)
davidhorstmann-arm (Migrated from github.com) commented 2025-08-27 10:58:35 -05:00

Nit: Technically to be consistent with the comment this should be:

    if (ret > -0x1000 && ret <= -0x80) {
Nit: Technically to be consistent with the comment this should be: ```suggestion if (ret > -0x1000 && ret <= -0x80) { ```
drivers/builtin/src/psa_crypto_pake.c
davidhorstmann-arm (Migrated from github.com) commented 2025-08-27 11:00:52 -05:00

Again, what appears to be a misplaced error code, odd.

Again, what appears to be a misplaced error code, odd.
felixc-arm (Migrated from github.com) reviewed 2025-08-28 10:52:40 -05:00
drivers/builtin/include/mbedtls/md.h
@ -29,3 +28,4 @@
#define MBEDTLS_ERR_MD_ALLOC_FAILED PSA_ERROR_INSUFFICIENT_MEMORY
/** Opening or reading of file failed. */
#define MBEDTLS_ERR_MD_FILE_IO_ERROR -0x5200
felixc-arm (Migrated from github.com) commented 2025-08-28 10:52:40 -05:00

I've treated it as a private file for now, I guess if it (or part of it) becomes public then the definition can be moved then...

I've treated it as a private file for now, I guess if it (or part of it) becomes public then the definition can be moved then...
bjwtaylor (Migrated from github.com) reviewed 2025-09-09 05:05:01 -05:00
core/psa_crypto.c
@ -403,4 +375,3 @@
return PSA_ERROR_INVALID_SIGNATURE;
case MBEDTLS_ERR_CIPHER_INVALID_CONTEXT:
return PSA_ERROR_CORRUPTION_DETECTED;
#endif
bjwtaylor (Migrated from github.com) commented 2025-09-09 05:05:01 -05:00

So, it appears this was added in a mass change of the error codes. Not sure if there is a source of truth for these, I've asked @gilles-peskine-arm on this. However the suggested PSA_ERROR_INVALID_ARGUMENT seems more appropriate. I suppose the only question would be by presenting PSA_ERROR_INVALID_ARGUMENT are we leaking information that could be exploited. If this is the case you would wonder why we have the code at all though.

So, it appears this was added in a mass change of the error codes. Not sure if there is a source of truth for these, I've asked @gilles-peskine-arm on this. However the suggested PSA_ERROR_INVALID_ARGUMENT seems more appropriate. I suppose the only question would be by presenting PSA_ERROR_INVALID_ARGUMENT are we leaking information that could be exploited. If this is the case you would wonder why we have the code at all though.
bjwtaylor (Migrated from github.com) reviewed 2025-09-09 05:12:23 -05:00
core/psa_crypto.c
@ -403,4 +375,3 @@
return PSA_ERROR_INVALID_SIGNATURE;
case MBEDTLS_ERR_CIPHER_INVALID_CONTEXT:
return PSA_ERROR_CORRUPTION_DETECTED;
#endif
bjwtaylor (Migrated from github.com) commented 2025-09-09 05:12:23 -05:00

I've confirmed with gilles and it sounds like invalid argument is intended for when the arguments are invalid. Not supported is more for when a feature exists but is disabled. So I think the conversion to PSA_ERROR_INVALID_ARGUMENT is appropriate here and the previous PSA_ERROR_NOT_SUPPORTED is best removed. Let me know if your happy with this.

I've confirmed with gilles and it sounds like invalid argument is intended for when the arguments are invalid. Not supported is more for when a feature exists but is disabled. So I think the conversion to PSA_ERROR_INVALID_ARGUMENT is appropriate here and the previous PSA_ERROR_NOT_SUPPORTED is best removed. Let me know if your happy with this.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-09 05:20:37 -05:00
core/psa_crypto.c
@ -403,4 +375,3 @@
return PSA_ERROR_INVALID_SIGNATURE;
case MBEDTLS_ERR_CIPHER_INVALID_CONTEXT:
return PSA_ERROR_CORRUPTION_DETECTED;
#endif
gilles-peskine-arm (Migrated from github.com) commented 2025-09-09 05:20:37 -05:00

Strange that this one was converted to PSA_ERROR_NOT_SUPPORTED previously - I wonder why that was.

I don't know. It was done in 892cd6df70 which was just a merge and then the history is hard to follow. It looks like MBEDTLS_ERR_CAMELLIA_BAD_INPUT_DATA was partly replacing the more specific error code MBEDTLS_ERR_CAMELLIA_INVALID_KEY_LENGTH, so maybe “not supported” could have meant “this key size is not enabled in this built”? Except that we never supported partial key length support with Camellia, only with AES. So maybe a more likely explanation is that it just made some test case pass. Anyway I think PSA_ERROR_INVALID_ARGUMENT is the right choice by default for a MBEDTLS_ERR_xxx_BAD_INPUT_DATA. It's what was meant to have the same meaning.

> Strange that this one was converted to PSA_ERROR_NOT_SUPPORTED previously - I wonder why that was. I don't know. It was done in 892cd6df7044e6166a69c45b9b2b48f7927cac96 which was just a merge and then the history is hard to follow. It looks like `MBEDTLS_ERR_CAMELLIA_BAD_INPUT_DATA` was partly replacing the more specific error code `MBEDTLS_ERR_CAMELLIA_INVALID_KEY_LENGTH`, so maybe “not supported” could have meant “this key size is not enabled in this built”? Except that we never supported partial key length support with Camellia, only with AES. So maybe a more likely explanation is that it just made some test case pass. Anyway I think `PSA_ERROR_INVALID_ARGUMENT` is the right choice by default for a `MBEDTLS_ERR_xxx_BAD_INPUT_DATA`. It's what was meant to have the same meaning.
valeriosetti commented 2025-09-16 06:51:38 -05:00 (Migrated from github.com)

This PR is continued in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/459 because I cannot push to felixc-arm:unify-error-codes.

This PR is continued in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/459 because I cannot push to `felixc-arm:unify-error-codes`.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
davidhorstmann-arm
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
needs-review
Every commit must be reviewed by at least two team members

  
needs-reviewer
This PR needs someone to pick it up for review

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!422
No description provided.