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

fiptool: Add --align-reserve command line option #916

Closed
wants to merge 1 commit into from

Conversation

afaerber
Copy link
Contributor

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]

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]>
@ssg-bot
Copy link

ssg-bot commented Apr 24, 2017

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");
Copy link

@ghost ghost Apr 24, 2017

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().

Copy link
Contributor Author

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.

Copy link

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.

@ghost
Copy link

ghost commented Apr 24, 2017

BTW, what is the mechanism by which you prepend those headers? Do you post-process the FIP using another tool?

@afaerber
Copy link
Contributor Author

afaerber commented Apr 24, 2017

Yes, see the commit message. The tools are proprietary aml_encrypt_gxb (there's also aml_encrypt_gxl, aml_encrypt_gxtvbb and aml_encrypt_txl versions not yet investigated in detail) and my Open Source replacement amlbootsig in the mentioned meson-tools.

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 padlen in my code).

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 fiptool as a realistic option - in case that's what you were getting at.

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.

@ghost
Copy link

ghost commented Apr 24, 2017

I see. I was just wondering whether the headers you are referring to could be additional images.

@ghost
Copy link

ghost commented Apr 25, 2017

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.

@masahir0y
Copy link
Contributor

[1] The option name "--pad" would be clearer? For example, dtc seems to do like this.
[2] If you want to make the units clearer in the help message, you can mention '--align ' instead of '--align '. dtc seems to do like this.

The following is a snippet of 'dtc --help'.

-S, --space
Make the blob at least long (extra space)
-p, --pad
Add padding to the blob of long (extra space)
-a, --align
Make the blob align to the (extra space)

@masahir0y
Copy link
Contributor

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 --align <bytes> instead of --align <value>. dtc seems to do like this.

The following is a snippet of 'dtc --help'.

-S, --space <arg>
    Make the blob at least <bytes> long (extra space)
-p, --pad <arg>
    Add padding to the blob of <bytes> long (extra space)
-a, --align <arg>
    Make the blob align to the <bytes> (extra space)

@ghost
Copy link

ghost commented Apr 27, 2017

Indeed, that would be clearer.

@danh-arm
Copy link
Contributor

Hi @afaerber. Do you have any response to the comment here to this PR?

@afaerber
Copy link
Contributor Author

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.
Given time, I will look into making minor tweaks to the help texts and update this pull.

@danh-arm
Copy link
Contributor

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.

@ghost
Copy link

ghost commented May 25, 2017

This PR has been superseded by PR "fiptool: Add --pad command line option".

@danh-arm
Copy link
Contributor

Superseded by #952

@danh-arm danh-arm closed this May 30, 2017
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