Address two fenrir reports - #859
Open
danielinux wants to merge 4 commits into
Open
Conversation
octospi_wait_ready() pre-zeros sr and ignored the return value of octospi_cmd(). A failed status-register transfer therefore left sr == 0 and the do/while loop exited as if the flash were no longer BUSY, so a page program or sector erase that could not be confirmed completed was reported as successful by nor_flash_write()/nor_flash_erase() and, with it, by hal_flash_write/erase and ext_flash_write/erase. Make octospi_wait_ready() return -1 when the status read transfer fails, and abort both NOR mutation loops with -1 in that case, matching the existing error handling for the program/erase command transfers themselves. Note: the wait loop remains unbounded if the flash genuinely stays BUSY (stuck operation) — a separate concern from the false-success path fixed here; a bounded timeout would need a timing reference this HAL does not have and no sibling HAL currently uses one. Verified: make TARGET=stm32n6 (full build, wolfboot.bin + signed test-app image), tools/unit-tests suite green. Hardware fault injection required to reproduce, so no host unit test.
FlashProgram() and FlashSectorErase() return a Fapi_Status_t (FAPI_STATUS_FSM_ERROR, FAPI_STATUS_INCORRECT_DATABUFFER_LENGTH, ...) that both functions discarded. The subsequent FlashCheckFsmForReady() loop only waits for the flash controller FSM to go idle again and does not report whether the program or erase actually succeeded, so a failed operation (e.g. locked/faulty sector) was reported as success to the callers in src/update_flash.c and src/libwolfboot.c that key their control flow on the return value. Check both return values and return -1 on failure, matching the error-handling convention of the sibling HALs (mcxa, mcxn, same51, lpc55s69, psoc6, renesas-rx). A silently corrupted image would still be caught later by wolfBoot_verify_integrity/authenticity, so severity remains low; this makes the HAL contract honest. Verified: hal/cc26x2.c has no in-repo build target (TI SDK board), so the file was object-compiled with arm-none-eabi-gcc against FAPI/driverlib prototypes matching the TI public API (-Wall -Wextra, clean). tools/unit-tests suite green.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses two static-analysis (“fenrir”) findings by improving error propagation in NOR flash operations across two HAL implementations (STM32N6 OctoSPI and TI CC26x2).
Changes:
- STM32N6: Make
octospi_wait_ready()return a status and propagate status-register read failures up to NOR write/erase loops. - CC26x2: Check and handle return status from TI Flash API calls in
hal_flash_write()andhal_flash_erase().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
hal/stm32n6.c |
Propagates OctoSPI status-read failures to avoid falsely treating a failed SR read as “ready”. |
hal/cc26x2.c |
Validates TI Flash API return codes for program/erase operations and returns failure on error. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A failed WREN leaves the write-enable latch clear, so the following page program or sector erase is silently ignored by the device: the command itself completes at the controller, the flash never goes BUSY, and octospi_wait_ready() reports idle on its first poll. Same false-success shape the previous commit closed for the status read.
unit-flash-write-cc26x2 includes hal/cc26x2.c directly with stubbed TI SDK headers and injects Fapi status codes, pinning the new -1 returns from hal_flash_write()/hal_flash_erase(). All four error-injection cases fail against the pre-fix code. This is also the only build coverage hal/cc26x2.c has; it immediately flagged the const-discarding FlashProgram() call, now cast.
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.
69712e6 F-6869: check FAPI status in cc26x2 hal_flash_write/erase
0140466 F-7983: propagate OctoSPI status-read failures in octospi_wait_ready()