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.

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

add HW error codes to tfm.h, sp_int.h, integer.h #6565

Closed
wants to merge 1 commit into from

Conversation

gojimmypi
Copy link
Contributor

Description

Adds new hardware-acceleration-specific return error code definitions to math library header files.

Also makes return codes consistent between tfm.h, sp_int.h, and integer.h.

The intention is to allow better visibility to math errors with hardware acceleration. This change will also allow the ability to fall back to SW if hardware fails, is busy, etc.

These changes are needed for my current WIP for Espressif HW acceleration.

Fixes zd# n/a

Testing

How did you test?

ran wolfcrypt benchmark and test apps.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Why are we adding return codes that are not used anywhere? Why not use the error codes / values from error-crypt.h?

@gojimmypi
Copy link
Contributor Author

Why are we adding return codes that are not used anywhere?

This is in prep for my upcoming Espressif hardware acceleration changes, PR soon. The new error codes will allow a graceful fallback to software math.

Why not use the error codes / values from error-crypt.h?

I had considered that, but I thought other platforms that might use hardware acceleration would also use these same error codes, instead of having something specific to each platform. This would allow propagation of math errors to higher levels for improved visibility as to root causes of problems.

For example: I ended up working on the math acceleration only because some corner cases in the math acceleration did not work properly. The original symptom was an RSA padding error. It would have been tremendously helpful if the RSA error actually indicated there was an underlying math problem.

@gojimmypi gojimmypi requested a review from dgarske July 3, 2023 17:59
@gojimmypi gojimmypi mentioned this pull request Jul 7, 2023
4 tasks
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Please include this change with the PR that will actually use these. My preference is to use error-crypt values, not these math values.

@gojimmypi
Copy link
Contributor Author

ok. that's why this was in a separate PR: so that we could discuss issues in advance and without cluttering the big PR.

I'll renumber the exit code values to include in esp32-crypt.h, perhaps offset by 100 to ensure no future collisions with SP exit values:

#define MP_HW_ERROR    (-106) /* hardware error, consider SW fallback  */
#define MP_HW_BUSY     (-107) /* assigned -7 to match SP_HW_BUSY       */
#define MP_HW_FALLBACK (-108) /* signal to caller to fall back to SW   */
#define MP_HW_VALIDATION_ACTIVE (-109) /* optional HW validation active */

What do you think of the other consistency changes? (i.e. all the lines without "HW")

gojimmypi added a commit to gojimmypi/wolfssl that referenced this pull request Jul 10, 2023
@dgarske
Copy link
Contributor

dgarske commented Jul 10, 2023

ok. that's why this was in a separate PR: so that we could discuss issues in advance and without cluttering the big PR.

I'll renumber the exit code values to include in esp32-crypt.h, perhaps offset by 100 to ensure no future collisions with SP exit values:

#define MP_HW_ERROR    (-106) /* hardware error, consider SW fallback  */
#define MP_HW_BUSY     (-107) /* assigned -7 to match SP_HW_BUSY       */
#define MP_HW_FALLBACK (-108) /* signal to caller to fall back to SW   */
#define MP_HW_VALIDATION_ACTIVE (-109) /* optional HW validation active */

What do you think of the other consistency changes? (i.e. all the lines without "HW")

Why can't you just extend and use the existing wolfCrypt error codes in error-crypt.h and error.c?

@gojimmypi
Copy link
Contributor Author

Why can't you just extend and use the existing wolfCrypt error codes in error-crypt.h and error.c?

Yes, I will (already did), good suggestion about errors.c. I'll append to the end and adjust WC_LAST_E and MIN_CODE_E.

What I am referring to is the other changes:

Missing from sp_int.h: #define MP_ERROR (-1) and #define MP_WOULDBLOCK (-4) and #define MP_SIZE SP_INT_DIGITS

Missing from tfm.h: #define MP_SIZE (FP_MAX_SIZE/DIGIT_BIT) /* for compatibility with SP_INT */

But I suppose since they are not being used... perhaps not important. I just thought it would be good to be consistent.

gojimmypi added a commit to gojimmypi/wolfssl that referenced this pull request Jul 10, 2023
@gojimmypi
Copy link
Contributor Author

gojimmypi commented Jul 11, 2023

On the topic of error codes: although I was able to successfully add new values to error.c that works with wolfcrypt test, this modification fails with the benchmark due to a conflict with a separate set of error codes referenced in ssl.c.

Specifically the wolfssl/openssl/ssl.h does this:

#define PEM_R_BAD_PASSWORD_READ         (-MIN_CODE_E + 3)

As MIN_CODE_E has a new value of -303, making it positive and adding 3 ends causing the OpenSSL values to conflict with the wolfssl/error-ssl.h values.

and we end up with duplicate case value error:

C:/workspace/wolfssl-gojimmypi/src/ssl.c: In function 'wolfSSL_ERR_GET_LIB':
C:/workspace/wolfssl-gojimmypi/src/ssl.c:21868:5: error: duplicate case value
21868 |     case PEM_R_BAD_PASSWORD_READ:
      |     ^~~~
C:/workspace/wolfssl-gojimmypi/src/ssl.c:21863:5: note: previously used here
21863 |     case -SSL_R_HTTP_REQUEST:
      |     ^~~~

I'd rather not fudge the new hardware codes with misleading and unintuitive approximate matches to existing codes. But it looks like there's no practical room for new error codes in error-crypt.h.

What do you prefer that I do here?

*edit: perhaps this change in /src/ssl.c, adding 1000 to the adjustments? Still seems a bit hacky.

/* Avoid wolfSSL error code range */
#define PEM_R_NO_START_LINE             (-MIN_CODE_E + 1001)
#define PEM_R_PROBLEMS_GETTING_PASSWORD (-MIN_CODE_E + 1002)
#define PEM_R_BAD_PASSWORD_READ         (-MIN_CODE_E + 1003)
#define PEM_R_BAD_DECRYPT               (-MIN_CODE_E + 1004)
#define ASN1_R_HEADER_TOO_LONG          (-MIN_CODE_E + 1005)

/* Unused error. Defined for backward compatibility. */
#define MP_RANGE MP_NOT_INF
/* hardware error, consider falling back to SW */
#define MP_HW_ERROR (-6)
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use the existing WC_HW_E

Copy link
Contributor

Choose a reason for hiding this comment

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

Debug logs with WOLFSSL_MSG could be used to narrow down location of hardware error and distinguish between math hardware acceleration and others if wanted.

/* hardware error, consider falling back to SW */
#define MP_HW_ERROR (-6)
/* hardware busy; wait or fall back to SW */
#define MP_HW_BUSY (-7)
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use the existing WC_HW_WAIT_E

/* hardware busy; wait or fall back to SW */
#define MP_HW_BUSY (-7)
/* signal to caller to fall back to SW (e.g unsupported, etc) */
#define MP_HW_FALLBACK (-8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can NOT_COMPILED_IN or BAD_STATE_E be used instead?

@gojimmypi
Copy link
Contributor Author

Closing this lingering draft PR, as I believe this issue has been resolved in other PRs.

@gojimmypi gojimmypi closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants