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 CryptoCb features #6636

Merged
merged 4 commits into from
Jul 27, 2023
Merged

Add CryptoCb features #6636

merged 4 commits into from
Jul 27, 2023

Conversation

billphipps
Copy link
Contributor

@billphipps billphipps commented Jul 19, 2023

Description

Added additional cryptocb features:

  1. Test and Benchmark will now use WC_USE_DEVID if defined to set the default devId to allow out of tree build of test and benchmark objects
  2. Added support for cryptocb commands which are invoked during Register and UnRegister using ALGO_TYPE_NONE
  3. Register command callback allows hardware initialization/context to be set during register. Returning 0 indicates success and the dev->ctx will be updated to the value set in the provided info pointer. Returning CRYPTOCB_UNAVAILABLE or NOT_COMPILED_IN is silently handled as a success.
  4. Unregister command callback allows state and hardware cleanup to be triggered internally during the unregister.
  5. wolfCrypt_Cleanup now invokes UnRegister device on all devices, which will invoke the callback as well.
  6. Register/UnRegister command callback allow late hardware initialization and supports the Java JNI wrapper that is not able to easily pass in a hardware context pointer.

Testing

This was tested using the out-of-tree VaultIC 420 porting library. The current cryptocb test passed within test and the cleanup is being called automatically.

Checklist

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

@billphipps
Copy link
Contributor Author

I still owe a build option here.

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.

Really nice. Just a few minor suggestions.

wolfcrypt/benchmark/benchmark.c Outdated Show resolved Hide resolved
wolfcrypt/src/cryptocb.c Outdated Show resolved Hide resolved
wolfcrypt/src/cryptocb.c Outdated Show resolved Hide resolved
wolfcrypt/test/test.c Outdated Show resolved Hide resolved
@dgarske dgarske assigned billphipps and unassigned dgarske Jul 19, 2023
@billphipps billphipps marked this pull request as ready for review July 25, 2023 18:43
@dgarske dgarske self-assigned this Jul 26, 2023
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.

Looks great, just a couple small portability and coding standard cleanups. Then happy to merge it.

@@ -223,11 +252,28 @@ static WC_INLINE int wc_CryptoCb_TranslateErrorCode(int ret)
return ret;
}

/* Helper function to reset a device entry to invalid */
static inline void wc_CryptoCb_ClearDev(CryptoCb *dev)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider WC_INLINE vs inline. Portability.

@@ -255,6 +301,8 @@ void wc_CryptoCb_SetDeviceFindCb(CryptoDevCallbackFind cb)

int wc_CryptoCb_RegisterDevice(int devId, CryptoDevCallbackFunc cb, void* ctx)
{
int rc=0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use spaces around =. There are several places.

#ifdef WOLF_CRYPTO_CB_CMD
if (dev->cb != NULL) {
/* Invoke callback with unregister command.*/
wc_CryptoInfo info= {0};
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer an XMEMSET for portability. The = {0} is not perfectly portable.

#ifdef WOLF_CRYPTO_CB_CMD
if (cb != NULL) {
/* Invoke callback with register command */
wc_CryptoInfo info= {0};
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer an XMEMSET for portability. The = {0} is not perfectly portable.

@dgarske dgarske removed their assignment Jul 26, 2023
@billphipps billphipps requested a review from dgarske July 27, 2023 19:18
else if ((rc == CRYPTOCB_UNAVAILABLE) ||
(rc == NOT_COMPILED_IN)) {
/* Not implemented. Return success*/
rc = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for this. It enables backwards compatibility when WOLF_CRYPTO_CB_CMD is defined. I tested this with and without using wolfTPM's TLS crypto callback examples and all works!

@dgarske dgarske merged commit 10adca1 into wolfSSL:master Jul 27, 2023
73 checks passed
@billphipps billphipps deleted the add_vaultic420 branch August 29, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants