Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wolfBoot_update_trigger sector calculation #530

Closed

Conversation

MulattoKid
Copy link

wolfBoot_update_trigger is called to initiate an update on the next boot. It does this by modifying the metadata in the last sector (selecting one of the two if NVM_FLASH_WRITEONCE is defined).

There's a check to handle the case where the PART_UPDATE_ENDFLAGS isn't aligned with the WOLFBOOT_SECTOR_SIZE. However, this check was wrong, as the modulo of these two macros is 0 when aligned, so the check should be != 0.

See ticket #19133 for more information.

wolfBoot_update_trigger is called to initiate an update on the next
boot. It does this by modifying the metadata in the last sector
(selecting one of the two if NVM_FLASH_WRITEONCE is defined).

There's a check to handle the case where the PART_UPDATE_ENDFLAGS isn't
aligned with the WOLFBOOT_SECTOR_SIZE. However, this check was wrong, as
the modulo of these two macros is 0 when aligned, so the check should be
!= 0.
@MulattoKid
Copy link
Author

MulattoKid commented Dec 23, 2024

I've tested this, and performing an update now works. Without this change the second and third to last sectors are chosen to work with, instead of the last and second to last sectors, resulting in an invalid SHA integrity check when our image spans the entire available region (excluding the two sectors reserved by wolfBoot at the end of the BOOT and UPDATE partitions).

I'm really curious to understand if I'm thinking correctly about this, and there's been a bug in this check for a long time, or if I'm misunderstanding something. AFAIK wolfBoot reserves the last two sectors (each of size WOLFBOOT_SECTOR_SIZE) of each partition, but from testing it overwrites more (as explained in the ticket).

Edit: I see there's a unit test failing, so there's something strange going on here...

@dgarske
Copy link
Contributor

dgarske commented Dec 23, 2024

Unit test says:

83%: Checks: 6, Failures: 1, Errors: 0
unit-common.c:53:F:verify_integrity:test_verify_integrity:0: Assertion 'address + len <= (33 * 1024)' failed: address + len == 94631014433515, (33 * 1024) == 33792
make: *** [Makefile:37: run] Error 1

If the fix is valid it's possible a unit test may need updated. @jpbland1 please review.

@jpbland1 jpbland1 self-assigned this Dec 23, 2024
@dgarske
Copy link
Contributor

dgarske commented Dec 23, 2024

Closing in favor of #531

@dgarske dgarske closed this Dec 23, 2024
@MulattoKid
Copy link
Author

Thanks for looking into it!

@MulattoKid
Copy link
Author

Just noting that #531 seems to fix the issue

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.

4 participants