-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fiptool: Add --align-reserve command line option #916
Conversation
Allow each image to reserve some space before alignment. Use case is --align 0x4000 --align-reserve 64 to make sure sufficient room is available to prepend (or append) vendor-specific headers. Helps resolve afaerber/meson-tools#3. Signed-off-by: Andreas Färber <[email protected]>
Can one of the admins verify this patch? |
@@ -753,6 +773,7 @@ static void create_usage(void) | |||
printf("\n"); | |||
printf("Options:\n"); | |||
printf(" --align <value>\t\tEach image is aligned to <value> (default: 1).\n"); | |||
printf(" --align-reserve <value>\t\tEach image is padded by <value> (default: 0).\n"); |
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.
Use single \t here and in update_usage() and remove_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.
As you wish.
Can you think of any ways to improve the wording while at it? I clearly copied the --align example, but it might be clearer to append "bytes" to both? I.e., to distinguish it from which byte value is used to pad up to the alignment size, which I assume is always 0x00.
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.
We should probably mention the units indeed.
BTW, what is the mechanism by which you prepend those headers? Do you post-process the FIP using another tool? |
Yes, see the commit message. The tools are proprietary Affected are the development boards Hardkernel Odroid-C2 and presumably FriendlyElec NanoPi K2 (both Amlogic S905 aka Meson GXB), Khadas VIM (S905X aka GXL), as well as several TV boxes based on these and related Amlogic SoCs. I still have one ugly UUID addition that I am holding back. It seems that at least one image's offset and size are hardcoded in Odroid-C2's BL2, so I first want to investigate whether we can work around it by choosing other official UUIDs in the expected sort order. One other change in the postprocessing was an alignment of the image size to 16 bytes (see Since I am barely just finding out by black-box-testing what the header fields may mean, I do not see integrating those headers into Independently, it seems there are some licensing inconsistencies that will make upstreaming BLx driver code as BSD-3-clause difficult. Amlogic's U-Boot tarballs and Hardkernel's u-boot_firmware repository contain some code I didn't investigate further yet in "acs", "bl21" and "scp_task" dirs - Hardkernel has licensed the repository as GPL-3.0. |
I see. I was just wondering whether the headers you are referring to could be additional images. |
So thinking about it I have two concerns. First of all, those headers cannot be authenticated because they are essentially "phantom" blobs. To authenticate them they would need to be presented as an image with their own UUID and corresponding hash in the content certificate. My second concern is a consequence of the fact that these headers cannot be authenticated. The platform code will need to parse these headers and do appropriate checks. Failing to do these checks properly can result in security issues depending on the content of those headers. For example, assuming there is a load address field in those headers, the attacker could modify the field and point it anywhere. To be clear, I am not against merging this patch. I just want to understand more about the consequences of this patch in ARM TF. |
[1] The option name "--pad" would be clearer? For example, dtc seems to do like this. The following is a snippet of 'dtc --help'. -S, --space |
Bah, all the angle brackets disappeared. I wanted to mean like follows. I hope it is OK this time... [2] If you want to make the units clearer in the help message, you can mention The following is a snippet of 'dtc --help'.
|
Indeed, that would be clearer. |
Hi @danh-arm, as you know, I am not associated with Amlogic and do not understand why I should defend their boot ROM or BLx code. Thus, discussions about whether or not some hypothetical authentication feature we're not using works or works not in that theoretical scenario is utterly useless. |
Hi @afaerber. We're not asking you to defend Amlogic's boot code; we just wondered if you had more information on how it works, mainly just to understand the bigger picture. However, you're right that any security concerns in that firmware don't really affect this PR or the wider ARM TF Trusted Board Boot feature. So we'll merge this once you've done those minor tweaks. |
This PR has been superseded by PR "fiptool: Add --pad command line option". |
Superseded by #952 |
Allow each image to reserve some space before alignment.
Use case is --align 0x4000 --align-reserve 64 to make sure sufficient
room is available to prepend (or append) vendor-specific headers.
Helps resolve afaerber/meson-tools#3.
Signed-off-by: Andreas Färber [email protected]