-
Notifications
You must be signed in to change notification settings - Fork 841
bootutil: Add manifest-based loader for Direct XIP #2511
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
base: main
Are you sure you want to change the base?
bootutil: Add manifest-based loader for Direct XIP #2511
Conversation
6ebfcaf to
aaf2a5a
Compare
aaf2a5a to
d8e62ff
Compare
d8e62ff to
3688d72
Compare
JarmouniA
left a comment
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.
Can we have some documentation for this feature? Thanks!
3688d72 to
8eca34c
Compare
| static inline const uint8_t *bootutil_get_image_hash(const struct mcuboot_manifest *manifest, | ||
| uint32_t image_index) | ||
| { | ||
| if (!bootutil_verify_manifest(manifest)) { | ||
| return NULL; | ||
| } | ||
|
|
||
| if (image_index >= BOOT_IMAGE_NUMBER) { | ||
| return NULL; | ||
| } | ||
|
|
||
| if (image_index < MCUBOOT_MANIFEST_IMAGE_NUMBER) { | ||
| return manifest->image_hash[image_index]; | ||
| } else if (image_index > MCUBOOT_MANIFEST_IMAGE_NUMBER) { | ||
| return manifest->image_hash[image_index - 1]; | ||
| } | ||
|
|
||
| return NULL; | ||
| } |
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.
I am not sure this should be able to return NULL in any case.
This should panic on any parameter that is not expected.
Since the function is always called before boot_fih_memequal I think that both functions should be merged and the bootutil_get_image_hash should be changed to FIH return function that will be given enough parameter to compare a hash without pulling it out of the manifest object.
This should also reduce code size, since the boot_fih_memequal and will be combined here and no need for external null checks.
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.
I agree that there is no need to expose the manifest hash, however since this function is likely to be shared between the application and bootloader, I will remove the FIH part from the logic, so it does not rely on additional external modules.
|
|
||
| #if defined(MCUBOOT_MANIFEST_UPDATES) | ||
| struct mcuboot_manifest manifest[BOOT_NUM_SLOTS]; | ||
| bool manifest_valid[BOOT_NUM_SLOTS]; |
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.
Can we move that into the mcuboot_manifest? We can have different structs for TLV and internal usage.
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.
That'd need a struct within a struct, as the struct mcuboot_manifest is directly loaded from the TLV and the manifest_valid is not part of the incoming data, but an internal flag, indicating that the structure was loaded.
If you find the "valid" sate flag to be non-universal, I may move everything inside (including manifest_valid as well as matching_manifest), but I am hesitant to do so as those are not variables that holds information from the manifest, but a state variables for a loader logic and are not needed outside of the bootloader scope.
boot/bootutil/src/image_validate.c
Outdated
| if (image_index == MCUBOOT_MANIFEST_IMAGE_NUMBER) { | ||
| if (!state->manifest_valid[slot]) { | ||
| /* Manifest TLV must be processed before any of the image's hash TLV. */ | ||
| BOOT_LOG_INF("bootutil_img_validate: image rejected, manifest not found before image %d hash", |
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.
Isn't that at least warning? It ends up in rc = -1, so basically fails the function.
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.
I'm OK with increasing the log level. Generally speaking - this function is not too verbose (most errors logged as debug information), but I do not fully understand the reason behind it (code size?).
8eca34c to
34d235b
Compare
This feature definitely deserves a few words in the docs. Just keep in mind that:
|
34d235b to
faed92a
Compare
f5b0fdc to
fbb5565
Compare
Add a possibility to attach a basic manifest with expected digests to an image. Alter the image verification logic, so only digests specified by the manifest are allowed on the device. Signed-off-by: Tomasz Chyrowicz <[email protected]>
Add a simple logic that allows to attach a manifest TLV to an image. Signed-off-by: Tomasz Chyrowicz <[email protected]>
Add a loader variant that is capable of booting images, based on a simple manifest. Signed-off-by: Tomasz Chyrowicz <[email protected]>
Add a short description about motivation behind manifest-based updates and the Direct XIP mode of operation if the manifests are enabled. Signed-off-by: Tomasz Chyrowicz <[email protected]>
fbb5565 to
937c80a
Compare
Alternatively, you could expand the PR's description. |
Add a loader variant that is capable of booting images, based on a simple manifest.
Ideas: