Skip to content

cryptocb: sha512_family: try specific digest length hashtype first#9271

Merged
JacobBarthelmeh merged 2 commits intowolfSSL:masterfrom
rizlik:cryptocb_sha512_family_fix
Oct 6, 2025
Merged

cryptocb: sha512_family: try specific digest length hashtype first#9271
JacobBarthelmeh merged 2 commits intowolfSSL:masterfrom
rizlik:cryptocb_sha512_family_fix

Conversation

@rizlik
Copy link
Copy Markdown
Contributor

@rizlik rizlik commented Oct 6, 2025

If the cryptocb provider supports specific SHA512/224 and SHA512/256 hashtype, this commit allows to:

  1. avoid a copy
  2. do not touch the output buffer outside of the cryptocb handler

2 might be important for cryptocb provider that needs special handling of memory buffer (DMA, memory mapping).

@rizlik rizlik self-assigned this Oct 6, 2025
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🛟 Devin Lifeguard found 2 likely issues in this PR

  • do-not-change-external-apis snippet: Determine if headers bearing WOLFSSL_LOCAL declarations are part of the public install set; if the function is externally visible, create a new function (e.g., wc_CryptoCb_Sha512HashEx(..., size_t digestSz)) and have the original wrapper call it with digestSz set to WC_SHA512_DIGEST_SIZE.
  • limit-stack-usage snippet: Check the size of wc_CryptoInfo; if the combined size of wc_CryptoInfo + localHash + other locals exceeds 100 bytes, apply WOLFSSL_SMALL_STACK by allocating localHash dynamically (e.g., via XMALLOC) or reusing caller-supplied buffers.

@rizlik
please take a look at the above issues which Devin flagged. Devin will not fix these issues automatically.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the SHA512 family cryptocb implementation to prioritize specific hash types (SHA512/224 and SHA512/256) over the generic SHA512 handler, enabling better memory management and avoiding unnecessary buffer copies.

  • Updated the wc_CryptoCb_Sha512Hash function signature to include a digestSz parameter
  • Modified the cryptocb logic to attempt specific SHA512 family hash types before falling back to generic SHA512
  • Eliminated unnecessary local buffer allocation and memory copy operations when the cryptocb provider supports the specific hash type

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
wolfssl/wolfcrypt/cryptocb.h Added digestSz parameter to wc_CryptoCb_Sha512Hash function declaration
wolfcrypt/src/sha512.c Updated function calls and removed local buffer/copy logic in Sha512_Family_Final
wolfcrypt/src/port/arm/armv8-sha512.c Updated function calls with new parameter signature
wolfcrypt/src/cryptocb.c Implemented new logic to try specific hash types first and handle buffer management

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread wolfcrypt/src/port/arm/armv8-sha512.c Outdated
If the cryptocb provider supports specific SHA512/224 and SHA512/256
hashtype, this commit allows to:

1. avoid a copy
2. do not touch the output buffer outside of the cryptocb handler

2 might be important for cryptocb provider that needs special handling
of memory buffer (DMA, memory mapping).
@rizlik rizlik force-pushed the cryptocb_sha512_family_fix branch from e5a4a55 to 9cbc3f9 Compare October 6, 2025 09:43
@rizlik
Copy link
Copy Markdown
Contributor Author

rizlik commented Oct 6, 2025

🛟 Devin Lifeguard found 2 likely issues in this PR

* `do-not-change-external-apis` [snippet](https://github.com/wolfSSL/wolfssl/blob/e5a4a5582aee0b983439fb72aef737ae59c9933e/wolfssl/wolfcrypt/cryptocb.h#L678): Determine if headers bearing WOLFSSL_LOCAL declarations are part of the public install set; if the function is externally visible, create a new function (e.g., wc_CryptoCb_Sha512HashEx(..., size_t digestSz)) and have the original wrapper call it with digestSz set to WC_SHA512_DIGEST_SIZE.

* `limit-stack-usage` [snippet](https://github.com/wolfSSL/wolfssl/blob/e5a4a5582aee0b983439fb72aef737ae59c9933e/wolfcrypt/src/cryptocb.c#L1743): Check the size of wc_CryptoInfo; if the combined size of wc_CryptoInfo + localHash + other locals exceeds 100 bytes, apply WOLFSSL_SMALL_STACK by allocating localHash dynamically (e.g., via XMALLOC) or reusing caller-supplied buffers.

@rizlik please take a look at the above issues which Devin flagged. Devin will not fix these issues automatically.

  1. Function is static
  2. Local buffer was just moved inside the cryptocb handler, stack usage is the same of the base version.

@rizlik
Copy link
Copy Markdown
Contributor Author

rizlik commented Oct 6, 2025

retest this please

@rizlik rizlik assigned JacobBarthelmeh and unassigned rizlik Oct 6, 2025
Copy link
Copy Markdown
Contributor

@JacobBarthelmeh JacobBarthelmeh left a comment

Choose a reason for hiding this comment

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

It took awhile to make sure I understood the behavior with the if statements, but I think I got it now and it's a nice change.

If I understand it right, the desired behavior is:

  1. If WC_SHA512_256_DIGEST_SIZE (or other non WC_HASH_TYPE_SHA512) is handled by dev->cb then localHash is not used and the digest is passed directly.
  2. If WC_SHA512_256_DIGEST_SIZE (or other non WC_HASH_TYPE_SHA512) is not handled by dev->cb then a localHash is used and the result is copied back over. Using the full digest buffer size for a SHA512 operation and letting the caller truncate the resulting digest buffer if wanted. This was similar to the behavior before this change.
  3. If WC_HASH_TYPE_SHA512 is used the resulting digest is no longer copied over from a local buffer in all cases, and the digest buffer is passed directly instead.

@JacobBarthelmeh JacobBarthelmeh merged commit 68eb8b7 into wolfSSL:master Oct 6, 2025
259 of 260 checks passed
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.

3 participants