Update version checks during DFU update #253
+255
−19
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -214,9 +214,14 @@ const struct opentitan_image_version* libhoth_ot_staged_romext( | |
| bool libhoth_ot_boot_slot_eq( | ||
| const struct opentitan_get_version_resp* resp, | ||
| const struct opentitan_image_version* desired_romext, | ||
| const struct opentitan_image_version* desired_app, | ||
| const bool expect_desired_rom_ext) { | ||
| bool result = libhoth_ot_version_eq(libhoth_ot_boot_app(resp), desired_app); | ||
| if (expect_desired_rom_ext) { | ||
| result &= | ||
| libhoth_ot_version_eq(libhoth_ot_boot_romext(resp), desired_romext); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| bool libhoth_ot_staged_slot_eq( | ||
|
|
@@ -231,7 +236,69 @@ bool libhoth_ot_staged_slot_eq( | |
| bool libhoth_update_complete( | ||
| const struct opentitan_get_version_resp* resp, | ||
| const struct opentitan_image_version* desired_romext, | ||
| const struct opentitan_image_version* desired_app, | ||
| const bool expect_desired_rom_ext) { | ||
| // In case of ROM_EXT update not occurring due to valid reasons (see | ||
| // `expect_active_rom_ext_change_with_update`), the staging side will still be | ||
| // running the ROM_EXT from the update package | ||
| return libhoth_ot_boot_slot_eq(resp, desired_romext, desired_app, | ||
| expect_desired_rom_ext) && | ||
| libhoth_ot_staged_slot_eq(resp, desired_romext, desired_app); | ||
| } | ||
|
|
||
| bool expect_active_rom_ext_change_with_update( | ||
| const struct opentitan_image_version* desired_rom_ext_version, | ||
| const struct opentitan_get_version_resp* current_versions_resp) { | ||
| if ((desired_rom_ext_version == NULL) || (current_versions_resp == NULL)) { | ||
| return true; | ||
| } | ||
| const struct opentitan_image_version* current_rom_ext_version = | ||
| libhoth_ot_boot_romext(current_versions_resp); | ||
| // For booting ROM_EXT, ROM will give priority to slot with greater security | ||
| // version first, and then greater major version, and then greater minor | ||
| // version | ||
| // ([ref](https://github.com/lowRISC/opentitan/blob/acb2179c7f4b59d4ee2118fb24cfb4f97d920dc9/sw/device/silicon_creator/rom/boot_policy.c#L18-L41)) | ||
| if (current_rom_ext_version->security_version > | ||
|
Collaborator
There was a problem hiding this comment. This might be easier to understand with a
Collaborator
There was a problem hiding this comment. I guess that would be libhoth_ot_version_cmp(), which libhoth_ot_version_eq() can delegate to...
Collaborator
There was a problem hiding this comment. interestingly, libhoth_ot_version_eq() doesn't currently look at security version...
Collaborator
Author
There was a problem hiding this comment. I don't think the security version check here matters for the application firmware. The boot policy check for application firmware seems to only consult Same concerns apply for application major and minor firmware. My understanding is that which application firmware version to boot depends primarily on That's why I limited the logic to ROM_EXT only (
I am not sure if putting this logic in a generic |
||
| desired_rom_ext_version->security_version) { | ||
| fprintf(stderr, | ||
| "INFO: Running ROM_EXT security version %u, Desired ROM_EXT " | ||
| "security version %u\n", | ||
| current_rom_ext_version->security_version, | ||
| desired_rom_ext_version->security_version); | ||
| return false; | ||
| } else if ((current_rom_ext_version->security_version == | ||
| desired_rom_ext_version->security_version) && | ||
| (current_rom_ext_version->major > | ||
| desired_rom_ext_version->major)) { | ||
| fprintf(stderr, | ||
| "INFO: Running ROM_EXT has major version %u, Desired ROM_EXT major " | ||
| "version %u\n", | ||
| current_rom_ext_version->major, desired_rom_ext_version->major); | ||
| return false; | ||
| } else if ((current_rom_ext_version->security_version == | ||
| desired_rom_ext_version->security_version) && | ||
| (current_rom_ext_version->major == | ||
| desired_rom_ext_version->major) && | ||
| (current_rom_ext_version->minor > | ||
| desired_rom_ext_version->minor)) { | ||
| fprintf(stderr, | ||
| "INFO: Running ROM_EXT has minor version %u, Desired ROM_EXT minor " | ||
| "version %u\n", | ||
| current_rom_ext_version->minor, desired_rom_ext_version->minor); | ||
| return false; | ||
| } | ||
|
|
||
| // There is no need to check minimum ROM_EXT security version. ROM checks the | ||
| // security version of currently running ROM_EXT | ||
| // [here](https://github.com/lowRISC/opentitan/blob/acb2179c7f4b59d4ee2118fb24cfb4f97d920dc9/sw/device/silicon_creator/rom/boot_policy.c#L55-L60). | ||
| // Failure during this check will cause ROM to try the other ROM_EXT with the | ||
| // same check applied to it | ||
| // [here](https://github.com/lowRISC/opentitan/blob/acb2179c7f4b59d4ee2118fb24cfb4f97d920dc9/sw/device/silicon_creator/rom/rom.c#L761-L785). | ||
| // So it is expected that the running ROM_EXT has security version greater | ||
| // than or equal to the minimum ROM_EXT security version. In such a case, the | ||
| // conditions above will already handle firmware update to image with ROM_EXT | ||
| // version less than the minimum ROM_EXT security version (in the ROM_EXT | ||
| // security version check between desired and currently running ROM_EXT) | ||
|
|
||
| return true; | ||
| } | ||
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we want to complete omit check the ROM_EXT version in this case? Would it make more sense to instead check that the ROM_EXT version hasn't changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could it change? the version that booted previously is pinned in flash and the ROM will refuse to boot the old one we just flashed...
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used both at end of DFU update procedure, and during post-update procedure to check whether the running firmware matches what is expected (from the file). I can check that ROM_EXT version hasn't changes at end of the update procedure, but not for the other scenario (since I cannot query the version before the update in that case)