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

Do not add error codes #167

Open
wants to merge 9 commits into
base: development
Choose a base branch
from
+89 −89
Open

Do not add error codes #167

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4a71260
Have a low-level error code override any high-level one
gilles-peskine-arm Jan 24, 2025
7168431
Make it possible to include psa/crypto_values.h independently
gilles-peskine-arm Sep 24, 2024
3b6557b
Remove obsolete error code layout
gilles-peskine-arm Sep 24, 2024
f916903
error_common.h: turn error code constants into aliases for PSA codes
gilles-peskine-arm Jan 24, 2025
0799704
Changelog entry for error code space unification
gilles-peskine-arm Jan 24, 2025
a81889a
Don't allow psa_xxx() to return MBEDTLS_ERR_XXX
gilles-peskine-arm Jan 30, 2025
e08cefe
Finish sentence
gilles-peskine-arm Jan 30, 2025
05e4856
Add migration guide directory
gilles-peskine-arm Jan 31, 2025
f6f7355
Migration guide for error codes
gilles-peskine-arm Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ChangeLog.d/error-unification.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
API changes
* The PSA and Mbed TLS error space are now unified. This means that
psa_xxx() functions can now potentially return MBEDTLS_ERR_xxx
mpg marked this conversation as resolved.
Show resolved Hide resolved
values and mbedtls_xxx() functions can return PSA_ERROR_xxx values.
This will not affect most applications since in both cases, the
error values are between -32767 and -1 as before.
119 changes: 30 additions & 89 deletions drivers/builtin/include/mbedtls/error_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,97 +11,22 @@
#define MBEDTLS_ERROR_COMMON_H

#include "tf-psa-crypto/build_info.h"

#include <psa/crypto_values.h>
#include <stddef.h>

/**
* Error code layout.
*
* Currently we try to keep all error codes within the negative space of 16
* bits signed integers to support all platforms (-0x0001 - -0x7FFF). In
* addition we'd like to give two layers of information on the error if
* possible.
*
* For that purpose the error codes are segmented in the following manner:
*
* 16 bit error code bit-segmentation
*
* 1 bit - Unused (sign bit)
* 3 bits - High level module ID
* 5 bits - Module-dependent error code
* 7 bits - Low level module errors
*
* For historical reasons, low-level error codes are divided in even and odd,
* even codes were assigned first, and -1 is reserved for other errors.
*
* Low-level module errors (0x0002-0x007E, 0x0001-0x007F)
*
* Module Nr Codes assigned
* ERROR 2 0x006E 0x0001
* MPI 7 0x0002-0x0010
* GCM 3 0x0012-0x0016 0x0013-0x0013
* THREADING 3 0x001A-0x001E
* AES 5 0x0020-0x0022 0x0021-0x0025
* CAMELLIA 3 0x0024-0x0026 0x0027-0x0027
* BASE64 2 0x002A-0x002C
* OID 1 0x002E-0x002E 0x000B-0x000B
* DES 2 0x0032-0x0032 0x0033-0x0033
* CTR_DBRG 4 0x0034-0x003A
* ENTROPY 3 0x003C-0x0040 0x003D-0x003F
* NET 13 0x0042-0x0052 0x0043-0x0049
* ARIA 4 0x0058-0x005E
* ASN1 7 0x0060-0x006C
* CMAC 1 0x007A-0x007A
* PBKDF2 1 0x007C-0x007C
* HMAC_DRBG 4 0x0003-0x0009
* CCM 3 0x000D-0x0011
* MD5 1 0x002F-0x002F
* RIPEMD160 1 0x0031-0x0031
* SHA1 1 0x0035-0x0035 0x0073-0x0073
* SHA256 1 0x0037-0x0037 0x0074-0x0074
* SHA512 1 0x0039-0x0039 0x0075-0x0075
* SHA-3 1 0x0076-0x0076
* CHACHA20 3 0x0051-0x0055
* POLY1305 3 0x0057-0x005B
* CHACHAPOLY 2 0x0054-0x0056
* PLATFORM 2 0x0070-0x0072
* LMS 5 0x0011-0x0019
*
* High-level module nr (3 bits - 0x0...-0x7...)
* Name ID Nr of Errors
* PEM 1 9
* PKCS#12 1 4 (Started from top)
* X509 2 20
* PKCS5 2 4 (Started from top)
* DHM 3 11
* PK 3 15 (Started from top)
* RSA 4 11
* ECP 4 10 (Started from top)
* MD 5 5
* HKDF 5 1 (Started from top)
* PKCS7 5 12 (Started from 0x5300)
* SSL 5 2 (Started from 0x5F00)
* CIPHER 6 8 (Started from 0x6080)
* SSL 6 22 (Started from top, plus 0x6000)
* SSL 7 20 (Started from 0x7000, gaps at
* 0x7380, 0x7900-0x7980, 0x7A80-0x7E80)
*
* Module dependent error code (5 bits 0x.00.-0x.F8.)
*/

#ifdef __cplusplus
extern "C" {
#endif

/** Generic error */
#define MBEDTLS_ERR_ERROR_GENERIC_ERROR -0x0001
/** This is a bug in the library */
#define MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED -0x006E
/* Generic error */
#define MBEDTLS_ERR_ERROR_GENERIC_ERROR PSA_ERROR_GENERIC_ERROR
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we have a changelog entry to call out:

  • when a specific MBEDTLS_ERR_xxx constant becomes private?
  • when a specific MBEDTLS_ERR_xxx constant remains public, but becomes an alias of another value?

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 we shoud in both cases. This is a user facing error, and people will look at the changelog first when they see something not matching what they expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

On the practical side, this may make the ChangeLog pretty verbose, so perhaps in the end we'll just want to have a generic entry like "Some MBEDTLS_ERR_ constants have become private, and others have become aliases to other values. For details, see in the migration guide." But in the meantime it's probably better to create a ChangeLog entry for every change we make, so we have a comprehensive list (while avoiding conflicts) and we can consolidate/edit in the late stages before the release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I propose to start the migration document right away. Maybe multiple files that we'll merge at the end, similar to changelog.d, to avoid conflicts during the work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone ahead and created a directory for migration document fragments, and written a fragment about error codes.

/* This is a bug in the library */
#define MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED PSA_ERROR_CORRUPTION_DETECTED

/** Hardware accelerator failed */
#define MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED -0x0070
/** The requested feature is not supported by the platform */
#define MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED -0x0072
/* Hardware accelerator failed */
#define MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED PSA_ERROR_HARDWARE_FAILURE
/* The requested feature is not supported by the platform */
#define MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED PSA_ERROR_NOT_SUPPORTED

/**
* \brief Combines a high-level and low-level error code together.
Expand Down Expand Up @@ -130,13 +55,26 @@ extern void (*mbedtls_test_hook_error_add)(int, int, const char *, int);
* error code (that denotes success) and can be combined with both a
* negative error code or another value of zero.
*
* \note The distinction between low-level and high-level error codes is
* obsolete since TF-PSA-Crypto 1.0 and Mbed TLS 4.0. It is still
* present in the code due to the heritage from Mbed TLS <=3,
* where low-level and high-level error codes could be added.
* New code should not make this distinction and should just
* propagate errors returned by lower-level modules unless there
* is a good reason to report a different error code in the
* higher-level module.
*
* \note When invasive testing is enabled via #MBEDTLS_TEST_HOOKS, also try to
* call \link mbedtls_test_hook_error_add \endlink.
*
* \param high high-level error code. See error.h for more details.
* \param low low-level error code. See error.h for more details.
* \param file file where this error code addition occurred.
* \param line line where this error code addition occurred.
* \param high High-level error code, i.e. error code from the module
* that is reporting the error.
* This can be 0 to just propagate a low-level error.
* \param low Low-level error code, i.e. error code returned by
* a lower-level function.
* This can be 0 to just return a high-level error.
* \param file file where this error code combination occurred.
* \param line line where this error code combination occurred.
*/
static inline int mbedtls_error_add(int high, int low,
const char *file, int line)
Expand All @@ -149,7 +87,10 @@ static inline int mbedtls_error_add(int high, int low,
(void) file;
(void) line;

return high + low;
/* We give priority to the lower-level error code, because this
* is usually the right choice. For example, if a low-level module
* runs out of memory, this should not be converted to a */
mpg marked this conversation as resolved.
Show resolved Hide resolved
return low ? low : high;
}

#ifdef __cplusplus
Expand Down
1 change: 1 addition & 0 deletions include/psa/crypto_values.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#ifndef PSA_CRYPTO_VALUES_H
#define PSA_CRYPTO_VALUES_H
#include "mbedtls/private_access.h"
#include <psa/crypto_types.h>

/** \defgroup error Error codes
* @{
Expand Down