Feature unavailable errors #2054

RonEld · 2018-10-04T11:33:33Z

Description

Add a common error code for alternative implementations, returning MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED.
Remove all feature specific errors for Feature "Not Supported and for the HW acceleration failure.

Dependent on #1993 to be merged first
One of the PRs that supersedes #1716

Status

HOLD

Requires Backporting

NO

Migrations

If there is any API change, what's the incentive and logic for it.

YES

Removing existing error codes may result n backwards incompatibility. Developers May need to change their returned error codes for their alternative implementations. In addition, If a specific error code is expected, it should be changed as well.
The incentive is to have one common error code for a feature that is not supported by the underlying HW accelerator, which makes tests and applications simpler and readable.

This may result in future ABI break, since removed error codes might be reused in the future,

Additional comments

Any additional information that could be of interest

The following FEATURE UNAVAILABLE errors were kept, as they actually mean that a specific feature is currently unavailable, because of missing configuration or other reasons, but are not related to the alternative implementation:

MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE
MBEDTLS_ERR_PKCS12_FEATURE_UNAVAILABLE
MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE
MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE
MBEDTLS_ERR_PEM_FEATURE_UNAVAILABLE
MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE
MBEDTLS_ERR_MD_FEATURE_UNAVAILABLE
MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE
MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE

In addition, MBEDTLS_ERR_SSL_HW_ACCEL_FAILED was kept as it is a specific TLS feature.

Open question: should have MBEDTLS_ERR_THREADING_FEATURE_UNAVAILABLE been removed?

Todos

Tests
Documentation
Changelog updated
Backported

Patater · 2018-10-04T12:27:22Z

include/mbedtls/error.h

+ * GCM 2 0x0012-0x0014
+ * BLOWFISH 2 0x0016-0x0018
+ * THREADING 3 0x001C-0x001E
+ * AES 3 0x0020-0x0022 0x0021-0x0021


This looks a bit funny. Isn't 0x0021 within the range of 0x0020-0x0022 Why does it need a separate entry?

The left is the even error codes, the right are the odd error codes

Weird. THREADING doesn't do that. BLOWFISH doesn't do that.

Nevermind. BLOWFISH doesn't do this because we deleted 0x0017. THREADING also doesn't have odd error codes.

Patater · 2018-10-04T12:29:20Z

include/mbedtls/error.h

+ * BLOWFISH 2 0x0016-0x0018
+ * THREADING 3 0x001C-0x001E
+ * AES 3 0x0020-0x0022 0x0021-0x0021
+ * CAMELLIA 2 0x0024-0x002


0x002 looks wrong.

yes, it's a typo

Patater · 2018-10-04T12:30:55Z

include/mbedtls/gcm.h

- * \return #MBEDTLS_ERR_GCM_HW_ACCEL_FAILED or a cipher-specific
- * error code if the encryption or decryption failed.
+ * \return #MBEDTLS_ERR_GCM_BAD_INPUT if the lengths are not valid or
+ * a cipher-specific error code if the encryption


A platform error code is not a cipher-specific error code. What makes sense to say here? Should we list MBEDTLS_ERR_PLATFORM_HW_FAILED explicitly or say "a cipher- or platform-specific error code"? Same goes for other similar comments for other functions and files.

Correct, but the MBEDTLS_ERR_PLATFORM_HW_FAILED is a generic error that is returned by the platform. The cipher specific code is because gcm is part of the cipher module.

Where in this function's documentation do we indicate that you might receive a platform error code? Or is that universally implied or expressly written elsewhere?

It should be universally implied. It will be updated in the relevant KB article, about implementing alternative modules

RonEld · 2018-10-07T11:08:01Z

@Patater I addressed your comments

itayzafrir

LGTM

RonEld · 2018-10-09T10:01:15Z

CI failed in timing test, which is a known issue

RonEld · 2018-10-09T10:02:30Z

@Patater Thank you for your review.
Do you have additional comments? Is this approved for design?

RonEld · 2018-10-09T11:59:13Z

@Patater @itayzafrir Thank you for reviewing.

I would like to hear your comments about the additional comments in the description.

Patater · 2018-10-09T13:13:31Z

Should MBEDTLS_ERR_THREADING_FEATURE_UNAVAILABLE have been removed?

I could see that making sense, but it's also OK as-is, IMO.

RonEld · 2018-10-14T11:51:59Z

include/mbedtls/platform.h

@@ -43,6 +43,9 @@
 #include "platform_time.h"
 #endif

+#define MBEDTLS_ERR_PLATFORM_HW_FAILED -0x0080 /**< Hardware failed platform operation. */
+#define MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED -0x0082 /**< The requested feature is not supported by the platform */


The value probably needs to be changed.
I get the following:

programs/util/strerror 0x82 Last error was: -0x0082 - UNKNOWN ERROR CODE (0080) : BIGNUM - An error occurred while reading from or writing to a file

I'll change to 0x78, assuming the parsing will work

The problem is that our low level error codes are defined as:

(0x0002-0x007E, 0x0003-0x007F)

RonEld · 2018-10-22T13:57:29Z

@Patater @itayzafrir I have updated the error code value.
Please review

itayzafrir · 2018-10-24T12:42:04Z

Looks good to me. Note there's a typo in commit message for e9afdd2, rage -> range.

RonEld · 2018-11-07T12:29:26Z

As discussed offline, I have returned the removed error codes, for API compatibility sake, and made them deprecated
cc @Patater @itayzafrir

Patater · 2018-11-07T12:51:14Z

include/mbedtls/aes.h

+#define MBEDTLS_DEPRECATED
+#endif
+
+#define MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE static inline int MBEDTLS_DEPRECATED( void ){ return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED ); }


What's the name of this inline function?

Why do we need an inline function? Add a comment.

I think I'd rather update our test scripts to work with constant deprecation than to use inline functions to work around the scripts' limitations.

What's the name of this inline function?
Copy paste error. I'll add names to the functions

Why do we need an inline function? Add a comment.

I couldn't find another way to deprecate MACROs. If I add a warning, we get compilation warnings, and thus compilation errors when warnings are treated as errors. see https://stackoverflow.com/questions/2681259/how-to-deprecate-a-macro-in-gcc

I think I'd rather update our test scripts to work with constant deprecation than to use inline functions to work around the scripts' limitations.

Me too, but how do you suggest we make the constant deprecations?

Note that all.sh checks explicitely for MBEDTLS_DEPRECATED_WARNINGS and treat warnings as errors:

msg "build: make, full config + DEPRECATED_WARNING, gcc -O" # ~ 30s cleanup cp "$CONFIG_H" "$CONFIG_BAK" scripts/config.pl full scripts/config.pl set MBEDTLS_DEPRECATED_WARNING # Build with -O -Wextra to catch a maximum of issues. make CC=gcc CFLAGS='-O -Werror -Wall -Wextra' lib programs make CC=gcc CFLAGS='-O -Werror -Wall -Wextra -Wno-unused-function' tests msg "build: make, full config + DEPRECATED_REMOVED, clang -O" # ~ 30s # No cleanup, just tweak the configuration and rebuild make clean scripts/config.pl unset MBEDTLS_DEPRECATED_WARNING scripts/config.pl set MBEDTLS_DEPRECATED_REMOVED # Build with -O -Wextra to catch a maximum of issues. make CC=clang CFLAGS='-O -Werror -Wall -Wextra' lib programs make CC=clang CFLAGS='-O -Werror -Wall -Wextra -Wno-unused-function' tests

@Patater How about we go with our original plan and define the MACROS as the platform errors, but not using the MBEDTLS_DEPRECATED_WARNING?

RonEld · 2018-11-07T14:01:35Z

As discussed offline, I dropped the deprecation of the constant, and kept the commit for the build error

cc @Patater @itayzafrir

Patater · 2018-11-09T13:13:19Z

CI failure is timing-suite on FreeBSD, a known issue.

Add a common error for the feature unavailable, in the platform module.

Patater · 2018-11-09T14:34:58Z

Rebased to add back in and deprecate the deprecated errors. Updated ChangeLog wording slightly.

Old branch at https://github.com/Patater/mbedtls/tree/dev/RonEld/feature_unavailable_errors

dgreen-arm

MBEDTLS_ERR_XTEA_HW_ACCEL_FAILED was removed in the previous version, it should probably get a deprecated warning now.

Deprecate the module-specific XXX_HW_ACCEL_FAILED and XXX_FEATURE_UNAVAILABLE errors, as alternative implementations should now return `MBEDTLS_ERR_PLATFORM_HW_FAILED` and `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED`.

Add the ChangeLog entry describing the change.

Patater · 2018-11-09T15:01:55Z

Fixed in original commit

simonbutcher

I think we need to change the ChangeLog (which I can do if you agree), and we should schedule work to deprecate the constants differently at a later date (including moving the comments to doxygen).

simonbutcher · 2018-11-09T16:16:01Z

ChangeLog

@@ -2,6 +2,10 @@ mbed TLS ChangeLog (Sorted per branch, date)

 = mbed TLS x.x.x branch released xxxx-xx-xx

+API Changes
+ * Add a common error code for a feature that is not supported by the


I think we can be specific and say MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED has been added,

Yes, specific is better.

simonbutcher · 2018-11-09T16:20:46Z

ChangeLog

@@ -24,6 +28,12 @@ Features
 hash and signature sizes that comply with FIPS 186-4, including SHA-512
 with a 1024-bit key.

+New deprecations
+ * All the current module specific errors that mean a feature is not available
+ are deprecated, so the platform error should be used.


I think we can specify what we mean here. eg.

All the current module specific errors that mean a feature is not available, such as MBEDTLS_ERR_XXX_FEATURE_UNAVAILABLE, are deprecated.

simonbutcher · 2018-11-09T16:32:14Z

ChangeLog

+ * All the current module specific errors that mean a feature is not available
+ are deprecated, so the platform error should be used.
+ * All the module specific generic hardware accelaration errors that existed
+ are deprecated, so the platform error should be used.


Same comment as above. We can show an example symbol,

simonbutcher · 2018-11-09T16:33:30Z

include/mbedtls/aes.h

@@ -60,7 +60,11 @@

 /* Error codes in range 0x0021-0x0025 */
 #define MBEDTLS_ERR_AES_BAD_INPUT_DATA -0x0021 /**< Invalid input data. */
+
+/* MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE is deprecated and should not be used. */


Not a blocker because there's precedent, but why have the statement on the symbol being deprecated above it and not in the doxygen comment?

Not sure what the best way for our library to document or tell the compiler that a define is deprecated, yet. Copied the approach used previously.

If there is a deprecated feature for doxygen, I think it is better. The approach use previously was on a file that is not part of the doxygen headers.

Patater · 2018-11-09T16:54:44Z

Yes, please feel free to update the ChangeLog as desired.

simonbutcher · 2018-11-09T19:55:05Z

retest

RonEld requested a review from Patater October 4, 2018 11:33

Patater suggested changes Oct 4, 2018

View reviewed changes

RonEld added enhancement mbed TLS team needs-design-approval component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts and removed component-crypto Crypto primitives and low-level interfaces labels Oct 4, 2018

itayzafrir previously approved these changes Oct 8, 2018

View reviewed changes

Patater previously approved these changes Oct 9, 2018

View reviewed changes

Patater added approved for design and removed needs-design-approval labels Oct 9, 2018

Patater added approved Design and code approved - may be waiting for CI or backports needs-preceding-pr Requires another PR to be merged first labels Oct 9, 2018

RonEld commented Oct 14, 2018

View reviewed changes

RonEld added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Oct 14, 2018

RonEld dismissed stale reviews from Patater and itayzafrir via a7307a2 October 22, 2018 13:23

RonEld force-pushed the feature_unavailable_errors branch from a7307a2 to e9afdd2 Compare October 22, 2018 13:40

RonEld added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Oct 22, 2018

RonEld mentioned this pull request Oct 22, 2018

Feature unavailable unified error support #2124

Closed

4 tasks

RonEld dismissed Patater’s stale review via dc011c0 November 7, 2018 12:26

Patater suggested changes Nov 7, 2018

View reviewed changes

RonEld force-pushed the feature_unavailable_errors branch from dc011c0 to fd9a4b2 Compare November 7, 2018 14:00

RonEld force-pushed the feature_unavailable_errors branch 3 times, most recently from bc4dd9f to ac0fb67 Compare November 7, 2018 14:40

Patater previously approved these changes Nov 9, 2018

View reviewed changes

dgreen-arm previously approved these changes Nov 9, 2018

View reviewed changes

Add common feature unavailable error

bcca58c

Add a common error for the feature unavailable, in the platform module.

Patater dismissed stale reviews from dgreen-arm and themself via e9ed646 November 9, 2018 14:33

Patater force-pushed the feature_unavailable_errors branch from ac0fb67 to e9ed646 Compare November 9, 2018 14:33

dgreen-arm suggested changes Nov 9, 2018

View reviewed changes

Ron Eldor added 2 commits November 9, 2018 15:01

Deprecate hardware acceleration errors

9924bdc

Deprecate the module-specific XXX_HW_ACCEL_FAILED and XXX_FEATURE_UNAVAILABLE errors, as alternative implementations should now return `MBEDTLS_ERR_PLATFORM_HW_FAILED` and `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED`.

Add ChangeLog entry

6aa9fb4

Add the ChangeLog entry describing the change.

Patater force-pushed the feature_unavailable_errors branch from e9ed646 to 6aa9fb4 Compare November 9, 2018 15:01

dgreen-arm approved these changes Nov 9, 2018

View reviewed changes

simonbutcher approved these changes Nov 9, 2018

View reviewed changes

simonbutcher added needs-ci Needs to pass CI tests approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Nov 9, 2018

simonbutcher merged commit 6aa9fb4 into Mbed-TLS:development Nov 12, 2018

gilles-peskine-arm mentioned this pull request Aug 12, 2022

Document and fix handling of MBEDTLS_PLATFORM_C and platform.h #6199

Closed

Feature unavailable errors #2054

Feature unavailable errors #2054

Conversation

RonEld commented Oct 4, 2018

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Oct 7, 2018

itayzafrir left a comment

Choose a reason for hiding this comment

RonEld commented Oct 9, 2018

RonEld commented Oct 9, 2018

RonEld commented Oct 9, 2018

Patater commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Oct 22, 2018

itayzafrir commented Oct 24, 2018 • edited Loading

RonEld commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Nov 7, 2018

Patater commented Nov 9, 2018

Patater commented Nov 9, 2018

dgreen-arm left a comment

Choose a reason for hiding this comment

Patater commented Nov 9, 2018

simonbutcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patater commented Nov 9, 2018

simonbutcher commented Nov 9, 2018

itayzafrir commented Oct 24, 2018 •

edited

Loading