Remove obsolete python tools, fix tools finding and other minor issues - #909
Merged
Merged
Conversation
case 4 masked to 32 bits then fell through into case 8, whose only statement is break. Harmless today, but indistinguishable from a genuine missing break; add the explicit break.
fread() returns size_t, so the (io_sz < 0) guard could never trigger, and a short read silently folded stale buffer bytes into the image digest. Compare the read count against the requested size in all three loops (SHA-256, SHA-384, SHA3-384), matching the existing io_sz != expected pattern used elsewhere in this file.
argv[argc] is NULL, so a missing trailing argument (e.g. 'sign --ecc256 img.bin key.der') yielded a NULL fw_version that crashed strtoul, and --id/--encrypt/--delta/--policy as the last token dereferenced NULL directly. Guard the four value-taking options like --cert-chain already does, and validate the positional argument count against the selected signing mode before indexing.
The snprintf building output_diff_file passed sizeof(CMD.output_image_file); both are char[PATH_MAX] today, so the guarantee was accidental. Use sizeof of the destination.
The loop wrote the 5-byte 'pBOOT' trailer SWAP_SIZE times, emitting 5x SWAP_SIZE bytes past the 4KB swap region (the file grew 16KB past the mmap'd device) and leaving the swap filled with 'pBOOT' instead of the erased 0xFF state real flash has. Use the 0xFF pad loop the non-WOLF path already uses.
ERASE_TIMEOUT was defined but never used; the final erase ACK used the plain short WAIT_CYCLES budget, so a slow remote flash made ext_flash_erase() fail even when the erase succeeded. Factor wait_ack_cycles() out of wait_ack() and use WAIT_CYCLES * ERASE_TIMEOUT for the erase ACK, like uart_rx_timeout() does with READ_TIMEOUT. Add unit tests for the success and timeout paths.
sign.py and keygen.py are superseded by the C tools/keytools/sign and keygen (built by the same Makefile, no python/wolfcrypt-py dependency). Rewrite unit-sign-delta-tlv.py to sign a real delta image with the C sign tool instead of the python bmdiff/sign.py pipeline, and update README.md and include/delta.h references.
The 64-bit partial write indexed a fixed base with i>>2, so an unaligned start crossing a double-word boundary programmed the wrong, misaligned DW pair. Adopt the stm32g4/stm32c0 form: recompute unit_addr = (address+i) & ~0x07 and use dst[0]/dst[1].
powerfail as the last argv made strtol(NULL) run - undefined behaviour and a segfault. Print a usage error and exit instead.
Rounding the start address down to a sector/page without extending len shifted the erase window back and left the tail unerased. Capture end = address+len before aligning and loop on address<end.
spi_write left spi_rx_ready at 0 on RX/TX bus error, so the unconditional spin in spi_read hung the bootloader. Force a defined 0xFF byte and set the ready flag on the error path.
n_parts is unsigned, so the caller's < 0 guard was dead code and an empty MBR reported success with zero partitions. Fail explicitly.
A mismatch at index 0 evaluated to 0, which spi_flash_probe reads as success. Use the fixed -1 like src/spi_flash.c does.
Groups are numbered 1..6; group 0 wrapped (group-1)*4 into a 0xFFFFFFFC shift count - undefined behaviour. Tighten the guard to group_number < 1.
The do/while fell out with status==0 when the card stayed busy until timeout, indistinguishable from supported, so the SWITCH command went out for a function the card never cleared. Set status=-1 when timeout reaches 0.
A 0 return (no base clock / ICS never stabilized) proceeded with an unconfigured card clock. Fail sdhci_init, emmc_card_full_init and the UHS-I 50MHz step when the clock is not programmed.
The CTR counter block built on the stack is a full copy of the secret disk-encryption nonce plus counter; both in-tree sibling IV helpers already scrub their stack copy, this was the only one that did not.
The small-stack crypto pool hands out workspace for hash blocks and signature verification state; the slot was released back to the pool still holding that data. Scrub it in XFREE before marking the slot free (wc_ForceZero, same helper the rest of the crypto path uses).
wolfBoot_verify_signature_ecc() imports the raw signature into mp_int r/s and passes them to wc_ecc_verify_hash_ex() without clearing them; mp_clear() scrubs the digit memory before the stack frame retires.
The key/key2 structs in main() hold the decoded private key material for the rest of the process lifetime, and free_key() only releases what its dispatch knows about. Scrub both objects unconditionally in cleanup so no key residue survives, for any algorithm.
wolfBoot_tpm2_deinit() unloads the session and SRK handles and cleans up the device, but the file-scope wolftpm_session (HMAC/parameter- encryption session key) and wolftpm_srk (SRK authValue) stayed in .bss SRAM for the booted OS to read. ForceZero both after Cleanup.
wolfBoot_seal_auth()/wolfBoot_unseal_auth() copy the caller authValue into a stack WOLFTPM2_KEYBLOB and the NV helpers copy it into a stack WOLFTPM2_NV; none were cleared before return, inconsistent with the existing TPM2_ForceZero(&unsealOut) in the same file. ForceZero each stack object at the single exit.
wolfBoot_seal_blob() and wolfBoot_unseal_blob() unload the policy_session TPM handle on cleanup but leave the stack session object (SRK-derived session key material) in the frame; ForceZero it in the common cleanup of both.
The tests #include the real src/tpm.c with a mocked wolfTPM surface: add the missing TPM2_ForceZero mock (already present in three sibling tests) and bump unit-tpm-blob's call-count assertion from 1 to 2 now that wolfBoot_unseal_blob() also scrubs policy_session in its exit path.
The forcezero_calls bump to 2 dropped the sizeof(Unseal_Out) assertion, leaving the unsealOut wipe unguarded. Track the first scrub length and assert it alongside the last one.
wolfSSL-Fenrir-bot
previously requested changes
Sep 23, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #909
Scan targets checked: wolfboot-src, wolfboot-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
The success test delivered the erase-completion ACK on the first poll, so it passed identically with the pre-PR short budget. Add a per-byte delay to the uart_rx mock and a test where the final ACK arrives after WAIT_CYCLES + 1 empty polls: the short budget times out (verified), the extended one returns 0.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #909
Scan targets checked: wolfboot-src, wolfboot-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
wolfSSL-Fenrir-bot
dismissed
their stale review
September 23, 2026 07:57
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
dgarske
approved these changes
Sep 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
07301f9 F-12943: zeroize the policy session in seal/unseal blob cleanup
525fd76 F-7395: zeroize the stack auth blobs in the TPM seal/unseal and NV paths
e1c7fb1 F-7393: zeroize the TPM session/SRK globals at deinit
8cec76e F-14155: scrub the decoded key objects in sign.c cleanup
60803ad F-14156: mp_clear the ECDSA r/s scalars after verify
217dbe8 F-11039: zeroize XFREE'd slots in the static malloc pool
6b50581 F-13660: zeroize the nonce-derived IV copy in disk_crypto_set_iv
02f15ef F-7062: sdhci: check sdhci_set_clock results at all three call sites
ab3b6f6 F-7386: sdcard_send_switch_function: fail when busy budget runs out
9b73957 F-7387: sdcard_send_switch_function: reject group 0
0f266e6 F-7973: qspi test_ext_flash: return -1, not -i, on mismatch
c94453b F-7972: disk_open_mbr: return -1 when no usable partition
0000505 F-7385: nrf54l spi: unblock spi_read after a DMA bus error
b092968 F-7384: hal_flash_erase: erase to the end of the requested range
bb99d7d F-7061: sim: reject powerfail without an address argument
7b4a2ec F-6758: hal_flash_write: derive DW base from address+i in else branch
769c388 Remove the obsolete python keytools, convert remaining users to C
2f9a8ea F-7064: give the erase-completion ACK its long timeout
53f1d4e F-7060: fill the swap area with erased bytes, not the update flags
16ab1e0 F-7057: use the size of the destination buffer
96f507e F-7058: bound-check positional and option arguments
bfda601 F-7059: detect short reads in the image hashing loops
76db63f F-7063: add missing break in arg2num case 4