-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add UC config for disabling storage hash validation #92
base: master
Are you sure you want to change the base?
Conversation
@ccli8 Thank you! I'll take a look! |
b07b2e3
to
1ef3df2
Compare
@marcuschangarm I merge the suggestion and original #91 into this PR. |
@ccli8 good catch with disabling I've sent you an email with an alternative solution to disabling hash calculations, since that would have a negative impact on our security story. |
4bda366
to
8e27558
Compare
@marcuschangarm I merge the in-transmit hash validation alternative into this PR. It's fine per my test. |
8e27558
to
25fabfe
Compare
@marcuschangarm Added PSA implementation Currently, only non-secure firmware update is supported. To support secure or combined secure/non-secure firmware update, it is necessary to read header (and TLV) of active located in SPE, which is prohibited by TF-M. |
uint32_t total_size = 0; | ||
|
||
result = | ||
arm_uc_pal_psa_get_hash_from_header(arm_uc_flashiap_active_read, |
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 was expecting the hash to be read using the PSA call psa_fwu_query
and not directly from memory: https://github.com/ARMmbed/mbed-os/blob/master/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/include/psa/update.h#L178-L193
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.
Originally, I also expect psa_fwu_query()
can do the hash calculation, but situation isn't so smooth: TF-M's psa_fwu_query()
doesn't return hash of active firmware for security concern
https://github.com/OpenNuvoton/trusted-firmware-m/blob/149271c63a448b0d690b41ae62dfbf3f36abcf72/secure_fw/partitions/firmware_update/bootloader/mcuboot/tfm_mcuboot_fwu.c#L529-L533
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 see. That doesn't seem right, but it is what it is.
I would suggest using the firmware version (x.y.z) as the key instead of the hash then. That would make the solution generic without depending on where the image is stored and whether the client has read permissions. The tradeoff is that new firmware must increment the version number for the client to detect any errors during installation.
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.
Using PSA firmware version (x.y.z) (actually, it is MCUboot firmware version) to identify firmware is one approach, but it is not workable in practice. Currently, in Mbed, all M2354 and Musca hardcode MCUboot firmware version to 1.3.0, that is, TF-M version. It is not a requirement to advance MCUboot firmware version number each build.
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 doesn't sound right. You should be able to set the firmware version when signing the image: https://mcu-tools.github.io/mcuboot/imgtool.html#signing-images
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.
Yes, mcuboot firmware version is specified on signing. But currently, Mbed integrated build system doesn't open this. I suggest adding one Update Client configuration so that secure or combined firmware update can be supported for such build system supporting the expected firmware versioning.
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 sounds like a PR to Mbed OS would fix that problem? From the Update client's and PSA's point of view there are three options:
- Full image update, updating both Secure and Non-Secure partition.
- Secure image update.
- Non-secure image update.
Using the concatenated version for both secure and non-secure as the key to KCM would make the update work as intended as long as the version is correctly incremented.
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 raise one PR to Mbed so that image version (Major.Minor.Revision+Build) via psa_fwu_query()
is unique. For this PR, I update for above discussion, plus removing unnecessary code.
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.
Sounds good, I'll take another look and get back to you!
if (result.error != ERR_NONE) { | ||
|
||
/* calculate hash from active image */ | ||
result = arm_uc_pal_psa_calculate_hash(arm_uc_flashiap_active_read, |
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 added this to the MCUBoot version to handle the corner case where one wanted to do a delta update as the very first update. In that scenario, the KCM would be empty and the update client would not be able to verify that the delta image matches the image it is supposed to be applied to.
For the PSA implementation, I would expect the secure side, e.g., TFM, to do the delta update since reading from memory is not guaranteed with PSA.
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.
Not clear about delta update. It seems Update Client needs to read active firmware plus delta update patch to generate intended new firmware for update. For non-secure firmware update, it is possible because non-secure active firmware can read. But for secure or combined firmware update, read is prohibited as pointed out.
/* TODO: Currently, only non-secure firmware update is supported. To support | ||
* secure/combined firmware update, it is necessary to read | ||
* header (and TLV) of active located in SPE, which is prohibited by | ||
* TF-M. */ |
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.
It shouldn't be necessary to calculate the hash in this file when using psa_fwu_set_manifest
and psa_fwu_query
. Depending on how the system is configured, it should be possible to do a full system update, updating secure and non-secure at the same time. In the new FOTA client it would also be possible to use component update to update each side individually.
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.
As above, TF-M's psa_fwu_query()
doesn't return hash of active firmware. Besides, its psa_fwu_set_manifest()
doesn't support. So I need to parse mcuboot and calculate hash of active firmware manually. But the solution can apply only on non-secure firmware and cannot on secure or combined one.
1. Add configuration UPDATE_CLIENT_STORAGE_PSA for PSA PAL 2. Fix compile errors for PSA PAL having no ARM_UC_FEATURE_PAL_FLASHIAP support 3. Support in-transit hash validation for like PSA PAL having no read permission and unable to do read-back hash validation from storage 4. Add PSA PAL implementation
25fabfe
to
8ee7cc3
Compare
[x] I confirm this contribution is my own and I agree to license it with Apache 2.0.
[x] I confirm the moderators may change the PR before merging it in.
[x] I understand the release model prohibits detailed Git history and my contribution will be recorded to the list at the bottom of CONTRIBUTING.md.
Summary of changes
This PR tries to add one Update Client configuration option to disable the flow of storage hash validation for PAL using like PSA FWU API which doesn't support storage read of firmware candidate (no
psa_fwu_read()
).@marcuschangarm