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

Keystore DMA functionality #85

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

bigbrett
Copy link
Contributor

Adds DMA capability for keystore operations that need to transmit keys (cache and export). Done in anticipation of PQC algorithms, which have larger key sizes.

A bit of a noisy diff, as I accidentally clang-formatted some code that I otherwise wouldn't have touched, but will be annoying to undo. I think it is still an improvement. You can turn off whitespace diff when looking at this PR to prevent that from being a distraction.

@bigbrett bigbrett requested a review from billphipps November 14, 2024 21:28
@bigbrett bigbrett self-assigned this Nov 15, 2024
Copy link
Contributor

@billphipps billphipps left a comment

Choose a reason for hiding this comment

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

A few minor nits for error handling but excellent!

return WH_ERROR_BADARGS;
}

packet = (whPacket*)wh_CommClient_GetDataPtr(c->comm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
packet = (whPacket*)wh_CommClient_GetDataPtr(c->comm);
packet = (whPacket*)wh_CommClient_GetDataPtr(c->comm);
if (packet == NULL) {
return WH_ERROR_BADARGS;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I don't think it is possible for this to return NULL right now, and we don't have this check anywhere else. Might be superfluous?

Comment on lines +1216 to +1221
if (labelSz > WH_NVM_LABEL_LEN) {
memcpy(packet->keyCacheDma32Req.label, label, WH_NVM_LABEL_LEN);
}
else {
memcpy(packet->keyCacheDma32Req.label, label, labelSz);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (labelSz > WH_NVM_LABEL_LEN) {
memcpy(packet->keyCacheDma32Req.label, label, WH_NVM_LABEL_LEN);
}
else {
memcpy(packet->keyCacheDma32Req.label, label, labelSz);
}
if (labelSz > WH_NVM_LABEL_LEN) {
labelSz = WH_NVM_LABEL_LEN;
}
memcpy(packet->keyCacheDma32Req.label, label, labelSz);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in #86

return WH_ERROR_BADARGS;
}

packet = (whPacket*)wh_CommClient_GetDataPtr(c->comm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
packet = (whPacket*)wh_CommClient_GetDataPtr(c->comm);
packet = (whPacket*)wh_CommClient_GetDataPtr(c->comm);
if (packet == NULL) {
return WH_ERROR_BADARGS;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I don't think it is possible for this to return NULL right now, and we don't have this check anywhere else. Might be superfluous?

@@ -58,6 +58,10 @@ enum WH_KEY_ENUM {
WH_KEY_EXPORT,
WH_KEY_COMMIT,
WH_KEY_ERASE,
WH_KEY_CACHE_DMA32,
Copy link
Contributor

Choose a reason for hiding this comment

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

Grumble. These should have been spelled like WH_MESSAGE_KEY_xxx. We should prolly fix all of them soon. No change now.

int wh_Client_KeyExportDma32(whClientContext* c, uint16_t keyId,
uint32_t keyAddr, uint16_t keySz, uint8_t* label,
uint16_t labelSz, uint16_t* outSz);
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#else
#endif /* WH_DMA_IS_32BIT */
#ifdef WH_DMA_IS_64BIT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

postponing fix until we rip/replace all the DMA stuff like we talked about. This will still be present in #86.

int wh_Client_KeyExportDma64(whClientContext* c, uint16_t keyId,
uint64_t keyAddr, uint16_t keySz, uint8_t* label,
uint16_t labelSz, uint16_t* outSz);
#endif /* WH_DMA_IS_32BIT */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#endif /* WH_DMA_IS_32BIT */
#endif /* WH_DMA_IS_64BIT */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

postponing fix until we rip/replace all the DMA stuff like we talked about. This will still be present in #86.

Comment on lines +1531 to +1533
#else
return wh_Client_KeyCacheDma64Response(c, keyId);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#else
return wh_Client_KeyCacheDma64Response(c, keyId);
#endif
#elseif defined(WH_DMA_IS_64BIT)
return wh_Client_KeyCacheDma64Response(c, keyId);
#else
#error("Either WH_DMA_IS_32BIT or WH_DMA_IS_64BIT must be defined.")
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

postponing fix until we rip/replace all the DMA stuff like we talked about. This will still be present in #86.

}

/* Copy metadata */
XMEMCPY(slotMeta, meta, sizeof(whNvmMetadata));
Copy link
Contributor

Choose a reason for hiding this comment

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

consider memcpy/memset instead of the wc wrapper version in case nocrypto is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in #86

@billphipps billphipps merged commit 6515aa5 into wolfSSL:main Nov 20, 2024
2 checks passed
@bigbrett bigbrett mentioned this pull request Nov 21, 2024
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