Fix/fenrir9929 by ageprocpp · Pull Request #11653 · wolfSSL/wolfssl

ageprocpp · 2026-10-06T03:32:27Z

Description

Fixes F-9929

wc_Sha256Copy(wc_Sha256* src, wc_Sha256* dst), which copies wc_Sha256 instances, lacked error handling when src and dst memories have overlapping.
It is unclear if this function should return BAD_FUNC_ARG on all such opportunity, but implemented that behavior.
The same changes to wc_Sha224Copy.

Testing

Did not add any test because the specification does NOT guarantee this function returns BAD_FUNC_ARG and just stipulate that this is an undefined behavior.
(If guaranteeing this returns BAD_FUNC_ARG, we'd have to change all other ad-hoc implementations for a number of environments)

Checklist

added tests (not needed)
updated/added doxygen
updated appropriate READMEs (not needed)
Updated manual and documentation (not needed)

Discussion

It is unclear if this function should return BAD_FUNC_ARG when src == dst because it is somehow reasonable expectation for the function to work fine by doing nothing. I would like have reviewers' comments in this respect.
It should be noted that src is not const pointer and in some environment the content of src can be changed by this function.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-10-06T05:40:37Z

retest this please

github-actions · 2026-10-06T05:43:41Z

MemBrowse Memory Report

gcc-arm-cortex-m0plus

FLASH: .text +1,168 B (+1.7%, 70,435 B / 262,144 B, total: 27% used)

gcc-arm-cortex-m3

FLASH: .text +1,368 B (+1.1%, 130,745 B / 262,144 B, total: 50% used)
RAM: .bss +12 B, .data +4 B (+2.0%, 836 B / 65,536 B, total: 1% used)

gcc-arm-cortex-m4

FLASH: .rodata.CSWTCH.1 +8 B, .rodata.str1.1 +83 B, .rodata.wolfSSL_ERR_reason_error_string.str1.1 +142 B, .text +1,472 B (+0.8%, 209,948 B / 262,144 B, total: 80% used)
RAM: .bss +12 B, .data +4 B (+1.9%, 856 B / 65,536 B, total: 1% used)

gcc-arm-cortex-m4-baremetal

FLASH: .text +1,152 B (+1.6%, 72,951 B / 262,144 B, total: 28% used)

gcc-arm-cortex-m4-crypto-only

FLASH: .rodata.CSWTCH.1 +8 B, .rodata.str1.1 +83 B, .text +1,152 B (+0.7%, 182,463 B / 262,144 B, total: 70% used)

gcc-arm-cortex-m4-dtls13

FLASH: .rodata +20 B, .text +1,536 B (+0.8%, 194,264 B / 1,048,576 B, total: 19% used)
RAM: .bss +12 B, .data +4 B (+1.9%, 872 B / 262,144 B, total: 0% used)

gcc-arm-cortex-m4-min-ecc

FLASH: .text +1,088 B (+1.7%, 67,609 B / 262,144 B, total: 26% used)

gcc-arm-cortex-m4-openssl-compat

FLASH: .rodata +208 B, .text +2,624 B (+0.4%, 796,012 B / 1,048,576 B, total: 76% used)
RAM: .bss +16 B (+0.0%, 133,820 B / 262,144 B, total: 51% used)

gcc-arm-cortex-m4-pkcs7

FLASH: .rodata.CSWTCH.1 +8 B, .rodata.str1.1 +83 B, .text +1,344 B (+0.7%, 223,552 B / 262,144 B, total: 85% used)

gcc-arm-cortex-m4-pq

FLASH: .rodata +252 B, .text +2,368 B (+0.8%, 311,716 B / 1,048,576 B, total: 30% used)
RAM: .bss +16 B, .data +4 B (+0.1%, 16,680 B / 262,144 B, total: 6% used)

gcc-arm-cortex-m4-rsa-only

FLASH: .rodata +256 B, .text +1,664 B (+0.6%, 343,064 B / 1,048,576 B, total: 33% used)
RAM: .bss +16 B, .data +4 B (+0.1%, 16,676 B / 262,144 B, total: 6% used)

gcc-arm-cortex-m4-sp-math

FLASH: .text +1,088 B (+1.7%, 67,609 B / 262,144 B, total: 26% used)

gcc-arm-cortex-m4-tls12

FLASH: .text +1,408 B (+1.1%, 131,537 B / 262,144 B, total: 50% used)
RAM: .bss +12 B, .data +4 B (+2.0%, 836 B / 65,536 B, total: 1% used)

gcc-arm-cortex-m4-tls13

FLASH: .rodata.CSWTCH.1 +8 B, .rodata.str1.1 +83 B, .rodata.wolfSSL_ERR_reason_error_string.str1.1 +142 B, .text +1,536 B (+0.7%, 249,230 B / 262,144 B, total: 95% used)
RAM: .bss +16 B, .data +4 B (+0.1%, 16,628 B / 65,536 B, total: 25% used)

gcc-arm-cortex-m7

FLASH: .rodata.CSWTCH.1 +8 B, .rodata.str1.1 +83 B, .rodata.wolfSSL_ERR_reason_error_string.str1.1 +142 B, .text +1,472 B (+0.8%, 209,948 B / 262,144 B, total: 80% used)
RAM: .bss +12 B, .data +4 B (+1.9%, 856 B / 65,536 B, total: 1% used)

gcc-arm-cortex-m7-pq

FLASH: .rodata +252 B, .text +2,304 B (+0.8%, 312,612 B / 1,048,576 B, total: 30% used)
RAM: .bss +16 B, .data +4 B (+0.1%, 16,680 B / 262,144 B, total: 6% used)

gcc-arm-cortex-m7-tls13

FLASH: .rodata.CSWTCH.1 +8 B, .rodata.str1.1 +83 B, .rodata.wolfSSL_ERR_reason_error_string.str1.1 +142 B, .text +1,536 B (+0.7%, 249,294 B / 262,144 B, total: 95% used)
RAM: .bss +16 B, .data +4 B (+0.1%, 16,628 B / 65,536 B, total: 25% used)

linuxkm-pie

Data: __patchable_function_entries +264 B (+0.9%, 29,048 B)

linuxkm-standard

Data: __patchable_function_entries +288 B (+0.6%, 52,040 B)

stm32-sim-stm32h753

FLASH: .data +4 B, .text +10,604 B (+5.4%, 205,616 B / 2,097,152 B, total: 10% used)

dgarske · 2026-10-06T15:33:20Z

Okay to test. Intern for wolfSSL

ageprocpp · 2026-10-06T17:45:31Z

Will rebase after #11665 is merged

Copilot

🟢 Approval recommended

The implementation is sound; only minor comment grammar corrections remain.

1 open finding

Correct ungrammatical phrase “had overlapping” · New

🧠 Review effort: Balanced

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #11653

Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
Not reviewed (no scan target covers them): doc/dox_comments/header_files/sha256.h
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

philljj

Thank you for the PR. The code looks good, but I think it is overkill for the issue:

wc_Sha256Copy checks each pointer for NULL but does not reject src == dst.

I think a check for src != dst would be sufficient, and you could add a test that expects BAD_FUNC_ARG in wolfcrypt/test/test.c when calling with src == dst.

ageprocpp · 2026-10-22T06:28:58Z

@philljj
Thank you for your review. I agree with the idea that the simplified patch is better.

On the other hand, if we try to add a test that expects BAD_FUNC_ARG, it can only be applied except for many ad-hoc implementations and the test will seem a bit awkward like this:

    /* Only the common implementation guarantees rejection of self-copy. */
#if !defined(WOLFSSL_TI_HASH) && !defined(WOLFSSL_RENESAS_RX64_HASH) && \
    (!defined(WOLFSSL_RENESAS_RSIP) || \
     defined(NO_WOLFSSL_RENESAS_FSPSM_HASH)) && \
    (!defined(WOLFSSL_KCAPI_HASH) || defined(WOLFSSL_NO_KCAPI_SHA224)) && \
    (!defined(WOLFSSL_HAVE_PSA) || defined(WOLFSSL_PSA_NO_HASH)) && \
    !defined(MAX3266X_SHA)
    ret = wc_Sha224Copy(sha, sha);
    if (ret != BAD_FUNC_ARG)
        ERROR_OUT(WC_TEST_RET_ENC_NC, exit);
#endif

I'd like to confirm if it looks good to you.

philljj · 2026-10-26T16:30:03Z

The awkward define gates around a wc_Sha224Copy test is OK with me, because it documents that the implementation is indeed awkward around different ports.

I would rather the awkward behavior difference be documented clearly in test, rather than left implicit.

ageprocpp · 2026-10-26T17:55:21Z

Reflected your review.

Copilot AI balanced review requested due to automatic review settings October 6, 2026 03:32

Copilot AI reviewed Oct 6, 2026

ageprocpp force-pushed the fix/fenrir9929 branch from 00356ca to 7f6cb06 Compare October 6, 2026 05:27

ageprocpp requested a balanced review from Copilot October 6, 2026 05:28

Copilot AI reviewed Oct 6, 2026

ageprocpp marked this pull request as ready for review October 6, 2026 05:40

dgarske assigned ageprocpp Oct 6, 2026

ageprocpp force-pushed the fix/fenrir9929 branch from 7f6cb06 to 8451160 Compare October 6, 2026 17:33

ageprocpp assigned wolfSSL-Bot Oct 6, 2026

ageprocpp added 2 commits October 8, 2026 22:34

Reject overlapping src and dst in wc_Sha224/256Copy

515b2f7

Modified docs to clarify the exceptional behavior

f4bc2f4

ageprocpp force-pushed the fix/fenrir9929 branch from 8451160 to f4bc2f4 Compare October 8, 2026 13:34

kojiws unassigned ageprocpp Oct 13, 2026

kojiws requested a balanced review from Copilot October 13, 2026 04:22

Copilot started reviewing on behalf of kojiws October 13, 2026 04:22

View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Comment thread wolfcrypt/src/sha256.c Outdated

ageprocpp force-pushed the fix/fenrir9929 branch from 0f4b56c to f4bc2f4 Compare October 13, 2026 06:36

ageprocpp requested a review from wolfSSL-Fenrir-bot October 20, 2026 08:09

wolfSSL-Fenrir-bot reviewed Oct 20, 2026

View reviewed changes

philljj self-assigned this Oct 21, 2026

philljj requested changes Oct 21, 2026

View reviewed changes

Comment thread wolfcrypt/src/sha256.c Outdated

philljj assigned ageprocpp and unassigned wolfSSL-Bot Oct 21, 2026

Simplify SHA context copy validation and test self-copy

6b79737

Wrap SHA self-copy test error codes with WC_NO_ERR_TRACE

c47c5f2

Conversation

ageprocpp commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist

Discussion

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 6, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MemBrowse Memory Report

Uh oh!

dgarske commented Oct 6, 2026

Uh oh!

ageprocpp commented Oct 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #11653

Uh oh!

philljj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ageprocpp commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philljj commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ageprocpp commented Oct 26, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ageprocpp commented Oct 6, 2026 •

edited

Loading

github-actions Bot commented Oct 6, 2026 •

edited

Loading

ageprocpp commented Oct 22, 2026 •

edited

Loading

philljj commented Oct 26, 2026 •

edited

Loading