You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixes for the verified Fenrir findings on wolfHAL, plus the follow-ups from a Skoll review of those fixes. Each of the 195 findings was checked against the code and the
reference manuals in the repo: 96 are fixed here, 33 are closed as false positives or by design, 1 is won't fix (13102, Sharp SCS timing), and the rest are deferred, mostly
test-coverage gaps.
Commits are grouped by driver area so each can be reviewed and bisected on its own.
Changes
Crypto
WBA/U5 AES split from WB (1254cc7): the newer AES IP has a different register layout. WBA/U5 now have their own driver: CCF through ISR/ICR, a KEYVALID wait,
two-step ECB/CBC decrypt key preparation, and KEIF-safe key zeroing.
N6 CRYP (1254cc7): polls IFNF before each AAD block; key zeroing is KERF-safe.
PKA (853473b): on timeout, aborts the operation before wiping operands, so the wipe isn't ignored and no stale flags are left behind.
CCM/GCM (3797ed9, all of WB, WBA/U5, N6):
AAD of 0xFF00 bytes or more uses the 0xFFFE + 4-byte length encoding from SP 800-38C.
A payload that doesn't fit in B0 is rejected.
Streaming CCM checks that Start's tag length and payload length match Process and Finalize.
GCM/CCM reject any Process call after a partial block.
Dispatch (3797ed9): the crypto dispatchers return EINVAL for a NULL device, matching the other dispatch layers. WBA HASH Oneshot checks its input before touching
the core.
Flash
Unlock and errors (6a56a5d):
Unlock skips the key sequence if the flash is already unlocked, and fails if LOCK stays set.
FLASH_SR errors are checked after program/erase.
WBA polls WDW before BSY; the L1 SetLatency polls are bounded.
Write path (0b0037b): WB alignment checks fixed. The source buffer is read bytewise via whal_LoadLe32, so an unaligned buffer no longer faults on the Cortex-M0+.
Flash is programmed through volatile pointers.
Bounds overflow (8c12172): every on-chip driver's region check is rewritten so addr + dataSz can't wrap.
External storage (e6acddb):
SPI-NOR rejects 3-byte ranges that would cross 16 MB, and Lock/Unlock preserve the other SR1 bits.
SD-over-SPI reports stop-sequence errors and releases CS on failure.
Ethernet (603b9d7, H5 and N6)
RX drops frames that have errors, span multiple buffers, or are too long for the RX buffer.
dsb is issued before the DMA tail-pointer writes.
Start validates speed and duplex.
LAN8742A routes MDIO through the configured MAC and powers down in Deinit.
UART / DMA
DMA-UART (a2accc5):
Busy is checked before the zero-length shortcut, so a blocking call can't abort another transfer.
New Deinit that stops both channels (WB, WBA).
The WBA TX bounce byte is now per instance.
Also in a2accc5: F4/L1 Send waits for TC, and GPDMA REQSEL is 8 bits wide for U5/N6.
GPDMA Stop (8c12172): aborts an active channel with suspend, idle wait and reset (RM0493 17.4.4), since software can't clear EN.
Direct-mapping flags (72d3ba8): forwarded in the WBA SPI, N6 WWDG and WBA UART-DMA stubs.
I2C / GPIO (506a155, 8c12172)
WB-family I2C derives TIMINGR from the requested frequency; the result is unchanged at 100 kHz, 400 kHz and 1 MHz. It also waits for TC before STOP or a repeated START.
L1 I2C:
Rejects NULL message buffers.
Clears ACK/POS on error.
Waits for a pending STOP before StartCom, and software-resets only a bus that stays busy.
GPIO (WB, WB0) rejects pin indexes past the configured table.
Timers / watchdog / RNG
SysTick (086d506, 8c12172): RELOAD is programmed with cyclesPerTick - 1 and range-checked, and CVR is cleared.
Also in 086d506: WWDG rejects counter and window values below 0x40, and LPTIM PWM rejects 0%/100% duty and periods below 2.
RNG (244b921):
Generate fails on the latched SEIS flag; WB runs the RM0434 recovery.
WBA/U5/N6 program RNG_HTCR with the Configuration C value.
WB0 checks FAULT.
Docs: return codes, parameter contracts and stale comments are corrected across the touched headers.
Tests
New:Test_Flash_OutOfBounds (generic, all on-chip flash) and a WB write-while-locked test.
Updated: the GPIO out-of-range index check and the LPTIM PWM limits.
Testing
Board
Result
STM32WB55 Nucleo
53/53 (also run with DMA=1 and a BMI270 on I2C)
STM32WBA55 Nucleo
47/47
STM32U5A5 Nucleo
47/47
STM32H563 Nucleo
15/15
STM32N657 Nucleo
54/54
STM32C031 Nucleo
12/12
STM32F091 Nucleo
12/12
STM32F302 Nucleo
12/12
STM32L152 Nucleo
13/13
STM32F411 Blackpill
7/7
STM32WB05 Nucleo
10/10
PIC32CZ Curiosity Ultra
11/11
Fenrir findings fixed (96)
Commit
Findings
1254cc7 Split WBA AES from WB, fix CRYP AAD flow control
11386
853473b Abort the PKA on timeout before wiping operands
8428, 11373, 11382, 13106
6a56a5d Guard flash unlock, check status errors, test write-while-locked
- Clear EN, clear flags and re-enable on WaitForProcEnd timeout so
ZeroOperand is not ignored and no stale PROCENDF/RAMERRF remains
- Fix ZeroOperand comment and result-size docs ("exactly", not "at
least")
…le-locked
- Skip the key sequence when already unlocked and return EHARDWARE if
LOCK stays set (WB, WBA, U5, H5, F4, F0/F3, C0)
- Check FLASH_SR errors after program/erase on WB, F0/F3, C0
- Clear PER on the WB erase error path
- Poll WDW before BSY on WBA; bound the L1 SetLatency polls
- Document Lock/Unlock as device-wide
- Add a WB test that writing while locked fails and leaves flash unchanged
…ite alignment checks and source/destination access
- WB: require 8-byte addr/dataSz for writes only (was 16-byte, applied
to erase, and never checked dataSz)
- Read the source buffer bytewise via new whal_LoadLe32 so unaligned
buffers no longer fault (C0 Cortex-M0+) (WB, C0, WBA, H5, U5, F4)
- Program flash through volatile pointers (WB, C0, WBA, H5, F4)
…codes, add out-of-bounds test
- PIC32CZ: reject Read/Write/Erase outside the flash region, return
early on zero-length erase, report controller errors as EHARDWARE,
read the source bytewise via whal_LoadLe32
- Document PIC32CZ Lock as applying no protection
- Add a generic on-chip test that out-of-region Read/Write/Erase return
EINVAL; add flash region macros for PIC32CZ and F411
…, fix PHY MDIO device
- Drop RX frames that have errors, span multiple buffers, or report a
length larger than the RX buffer (H5, N6)
- Add dsb before DMA tail-pointer kicks on H5 and the N6 RX error path
- Validate speed and duplex in Start (H5, N6)
- LAN8742A: route MDIO through the configured MAC instead of NULL;
power the PHY down in Deinit
- Fix PHY Init, N6 Send, and N6/H5 IP description docs
…x DMA-UART zero-length and teardown, F4 TC wait, GPDMA REQSEL width
- Widen GPDMA REQSEL to 8 bits so U5/N6 request IDs >= 64 are not
truncated
- DMA-UART: check busy before the zero-length shortcut so blocking calls
never wait on or abort another transfer (WB, WBA)
- DMA-UART: add Deinit that stops both DMA channels and resets transfer
state (WB, WBA); keep the WBA 1-byte TX bounce per instance
- F4/L1 UART Send waits for TC after the last byte
- Fix F0 UART Send and DMA callback docs
… requested freq, fix L1 I2C recovery, bound GPIO index
- I2C (WB family): derive SCL low/high from the requested frequency
instead of only the speed mode; wait for TC before STOP or a repeated
START
- I2C (L1): reject NULL message buffers, clear ACK/POS and STOP on
errors, software-reset a bus left busy before StartCom
- GPIO (WB, WB0): reject pin indexes past the configured pin table; add
an out-of-range index check to the GPIO test
- Fix WB SPI frame-size, GPIO AFR and whal_Reg_Update docs
…load and counter, WWDG lower bounds, LPTIM PWM limits
- SysTick: program RELOAD with cyclesPerTick - 1 and range-check it;
clear CVR in Init and implement Reset
- WWDG (F0/F3, WB/WBA/U5/N6): reject counter or window below 0x40, which
would reset the MCU immediately
- LPTIM PWM: reject period < 2 and 0% / 100% duty, which the LPTIM
cannot produce (ARR must exceed CMP); update the platform test
- Fix NVIC NULL-priority and IWDG start-order docs
…atched RNG errors, program WBA HTCR, check WB0 FAULT
- Fail Generate on the latched SEIS flag, not just SECS (WB, WBA/U5/N6,
H5); WB runs the RM0434 seed-error recovery, the others clear SEIS
as required with auto-reset enabled
- WBA/U5/N6: program RNG_HTCR with the Configuration C value during
CONDRST
- WB0: return EHARDWARE when RNG_SR.FAULT reports a bad noise sequence
- Fix WB, WBA and WB0 RNG return-value docs
… in WBA SPI, N6 WWDG and WBA UART-DMA
- WBA SPI and N6 WWDG stubs forward their DIRECT_API_MAPPING flags to
the reused driver, like their sibling stubs
- WBA UART-DMA maps its functions to the generic whal_Uart_* names and
hides the vtable under WHAL_CFG_STM32WBA_UART_DMA_DIRECT_API_MAPPING
…ng AAD and length checks, streaming chunk and binding checks, dispatch and doc fixes
- CCM: encode AAD lengths >= 0xFF00 as 0xFFFE + 4 bytes; reject payloads
whose length does not fit in B0 (WB, WBA/U5, N6)
- Streaming CCM: bind Start's tag length and payload length to Process
and Finalize; GCM/CCM: reject Process after a partial block
- Crypto dispatchers return EINVAL for a NULL device
- WBA HASH Oneshot validates input before touching the core
- Document CTR multiple-of-16 lengths, exact digest sizes, HMAC key
lifetime and per-algorithm operations
…flash bounds, L1 I2C STOP wait, GPDMA abort, doc sync
- Flash: rewrite region checks so addr + dataSz cannot wrap (all
on-chip drivers)
- L1 I2C StartCom waits for BUSY to clear and only software-resets a
bus that stays busy past the timeout
- GPDMA Stop aborts an active channel with suspend, idle wait and
reset, since software cannot clear EN (WBA, U5, N6); WBA UART-DMA
Deinit returns Stop errors before resetting transfer state
- SysTick Init rejects cyclesPerTick below 2
- Document the streaming CTR/GCM/CCM rules in the WB, WBA and N6 AES
headers, and the full WB/C0 flash Write/Erase return codes
…send, test bounds wraparound
- F4/L1 UART Send returns success for zero bytes instead of polling TC
- Flash out-of-bounds test adds Read/Write/Erase sizes that make
addr + dataSz wrap, so the overflow-safe bounds check is covered
Poll IFNF before writing the first AAD header block
src/crypto/stm32n6_cryp.c:1320
This poll only protects the second and later AAD blocks; the first header block is written at line 1313 without checking IFNF. If the input FIFO is not ready immediately after entering the header phase, that write can overrun it, so poll before every block as the PR description requires.
This issue also appears on line 1506 of the same file.
Reject frequencies above 1 MHz before calculating TIMINGR
src/i2c/stm32wb_i2c.c:181
Frequencies above 1 MHz enter this branch and are now used to scale the Fast-mode Plus minimum timings downward, although this driver documents support only up to 1 MHz. Reject comCfg->freq > 1000000 in StartCom before calculating TIMINGR; otherwise callers can program out-of-spec bus timings.
Stop RX even when stopping TX fails
src/uart/stm32wba_uart_dma.c:296
Returning immediately when stopping TX fails leaves the RX channel running, despite this Deinit contract promising to stop both channels. Attempt both stops, preserve the first error, and only then return it.
Add non-symmetric byte-order coverage for little-endian loads
wolfHAL/endian.h:48
The existing endian unit suite exercises every big-endian load/store boundary, but the new little-endian loader has no corresponding test. Add at least a non-symmetric byte pattern test so byte-order regressions in this flash-critical helper are caught.
…dian loads
- WB-family I2C StartCom returns EINVAL for freq above 1 MHz instead of
scaling Fast-mode Plus timings below the spec minimums
- Add whal_LoadLe32 cases to the core endian suite
Previously missed (4)
In code that hasn't changed since last review
Poll IFNF before writing the first AAD header block src/crypto/stm32n6_cryp.c:1320
This poll only protects the second and later AAD blocks; the first header block is written at line 1313 without checking IFNF. If the input FIFO is not ready immediately after entering the header phase, that write can overrun it, so poll before every block as the PR description requires.
This issue also appears on line 1506 of the same file.
Reject frequencies above 1 MHz before calculating TIMINGR src/i2c/stm32wb_i2c.c:181
Frequencies above 1 MHz enter this branch and are now used to scale the Fast-mode Plus minimum timings downward, although this driver documents support only up to 1 MHz. Reject comCfg->freq > 1000000 in StartCom before calculating TIMINGR; otherwise callers can program out-of-spec bus timings.
Stop RX even when stopping TX fails src/uart/stm32wba_uart_dma.c:296
Returning immediately when stopping TX fails leaves the RX channel running, despite this Deinit contract promising to stop both channels. Attempt both stops, preserve the first error, and only then return it.
Add non-symmetric byte-order coverage for little-endian loads wolfHAL/endian.h:48
The existing endian unit suite exercises every big-endian load/store boundary, but the new little-endian loader has no corresponding test. Add at least a non-symmetric byte pattern test so byte-order regressions in this flash-critical helper are caught.
🧠 Review effort: Balanced
I addressed "Reject frequencies above 1 MHz before calculating TIMINGR" and "Add non-symmetric byte-order coverage for little-endian loads"
The other two issues are not real.
- Add cr, htcr and nscr to the WBA and H5 RNG cfg; Init writes them during CONDRST
- Set per-board values from AN4230 Table 3 (WBA55, U5A5, N657, H563)
- Drop the hardcoded configuration C (WBA/U5/N6) and H563 values from the drivers
- Add Test_AesEcb_KnownAnswer128 (NIST SP 800-38A F.1.1/F.1.2)
- Add Test_AesCbc_KnownAnswer128 (NIST SP 800-38A F.2.1/F.2.2)
- Each test encrypts and checks the result against the NIST ciphertext, then decrypts back to the plaintext
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
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.
aidangarske
left a comment
Summary
Fixes for the verified Fenrir findings on wolfHAL, plus the follow-ups from a Skoll review of those fixes. Each of the 195 findings was checked against the code and the
reference manuals in the repo: 96 are fixed here, 33 are closed as false positives or by design, 1 is won't fix (13102, Sharp SCS timing), and the rest are deferred, mostly
test-coverage gaps.
Commits are grouped by driver area so each can be reviewed and bisected on its own.
Changes
Crypto
1254cc7): the newer AES IP has a different register layout. WBA/U5 now have their own driver: CCF through ISR/ICR, a KEYVALID wait,two-step ECB/CBC decrypt key preparation, and KEIF-safe key zeroing.
1254cc7): polls IFNF before each AAD block; key zeroing is KERF-safe.853473b): on timeout, aborts the operation before wiping operands, so the wipe isn't ignored and no stale flags are left behind.3797ed9, all of WB, WBA/U5, N6):0xFFFE+ 4-byte length encoding from SP 800-38C.3797ed9): the crypto dispatchers returnEINVALfor a NULL device, matching the other dispatch layers. WBA HASH Oneshot checks its input before touchingthe core.
Flash
6a56a5d):0b0037b): WB alignment checks fixed. The source buffer is read bytewise viawhal_LoadLe32, so an unaligned buffer no longer faults on the Cortex-M0+.Flash is programmed through volatile pointers.
1a1aafb): region bounds checks added, zero-length erase returns early, controller errors returnEHARDWARE.8c12172): every on-chip driver's region check is rewritten soaddr + dataSzcan't wrap.e6acddb):Ethernet (
603b9d7, H5 and N6)dsbis issued before the DMA tail-pointer writes.UART / DMA
a2accc5):a2accc5: F4/L1 Send waits for TC, and GPDMA REQSEL is 8 bits wide for U5/N6.8c12172): aborts an active channel with suspend, idle wait and reset (RM0493 17.4.4), since software can't clear EN.72d3ba8): forwarded in the WBA SPI, N6 WWDG and WBA UART-DMA stubs.I2C / GPIO (
506a155,8c12172)Timers / watchdog / RNG
086d506,8c12172): RELOAD is programmed withcyclesPerTick - 1and range-checked, and CVR is cleared.086d506: WWDG rejects counter and window values below 0x40, and LPTIM PWM rejects 0%/100% duty and periods below 2.244b921):RNG_HTCRwith the Configuration C value.Docs: return codes, parameter contracts and stale comments are corrected across the touched headers.
Tests
Test_Flash_OutOfBounds(generic, all on-chip flash) and a WB write-while-locked test.Testing
DMA=1and a BMI270 on I2C)Fenrir findings fixed (96)
1254cc7Split WBA AES from WB, fix CRYP AAD flow control853473bAbort the PKA on timeout before wiping operands6a56a5dGuard flash unlock, check status errors, test write-while-locked0b0037bFix write alignment checks and source/destination access1a1aafbBound PIC32CZ flash ops, fix error codes, add out-of-bounds test603b9d7Bound Ethernet RX, add DMA barriers, fix PHY MDIO devicea2accc5Fix DMA-UART zero-length and teardown, F4 TC wait, GPDMA REQSEL width506a155Scale I2C timing to requested freq, fix L1 I2C recovery, bound GPIO index086d506Fix SysTick reload and counter, WWDG lower bounds, LPTIM PWM limits244b921Check latched RNG errors, program WBA HTCR, check WB0 FAULTe6acddbBound SPI-NOR 3-byte ranges, preserve SR1 on lock, report SD stop errors72d3ba8Forward direct-mapping flags in WBA SPI, N6 WWDG and WBA UART-DMA3797ed9CCM long AAD and length checks, streaming chunk and binding checks, dispatch and doc fixesAll 96 IDs: 6077, 6079, 6084, 6086, 6087, 6088, 6089, 6090, 6091, 6093, 6187, 6195, 6455, 6636, 6637, 6638, 6919, 6924, 8428, 8436, 8439, 8440, 8441, 8442, 8446, 8450,
8451, 8452, 8454, 8455, 8456, 8457, 8458, 8462, 8463, 8465, 8466, 9351, 9352, 9353, 9354, 9355, 9356, 9357, 9358, 9359, 9360, 9361, 9362, 9363, 9364, 10178, 10179, 10181,
10183, 10184, 10190, 10191, 10192, 10193, 10195, 10196, 10203, 10205, 10207, 11364, 11365, 11367, 11368, 11369, 11370, 11371, 11372, 11373, 11374, 11375, 11382, 11383,
11384, 11386, 12344, 12356, 12357, 12366, 13101, 13103, 13106, 13107, 13123, 13124, 13746, 13747, 13748, 13780, 13781, 13784