diff mbox

[U-Boot] common: fit: Allow U-Boot images to be booted

Message ID 20160718120542.18598-1-mario.six@gdsys.cc
State Changes Requested
Delegated to: Wolfgang Denk
Headers show

Commit Message

Mario Six July 18, 2016, 12:05 p.m. UTC
In certain circumstances it comes in handy to be able to boot into a second
U-Boot. But as of now it is not possible to boot a U-Boot binary that is inside
a FIT image, which is problematic for projects that e.g. need to guarantee a
unbroken chain of trust from SOC all the way into the OS, since the FIT signing
mechanism cannot be used.

This patch adds the capability to load such FIT images.

An example its snippet (utilizing signature verification) might look like the
following:

images {
	kernel@1 {
		description = "2nd stage U-Boot image";
		data = /incbin/("u-boot-dtb.img.gz");
		type = "kernel";
		arch = "arm";
		os = "u-boot";
		compression = "gzip";
		load = <0x8FFFC0>;
		entry = <0x900000>;
		signature@1 {
			algo = "sha256,rsa4096";
			key-name-hint = "key";
		};
	};
};

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 common/image-fit.c | 1 +
 1 file changed, 1 insertion(+)

--
2.9.0

Comments

Tom Rini July 18, 2016, 3:59 p.m. UTC | #1
On Mon, Jul 18, 2016 at 02:05:42PM +0200, Mario Six wrote:

> In certain circumstances it comes in handy to be able to boot into a second
> U-Boot. But as of now it is not possible to boot a U-Boot binary that is inside
> a FIT image, which is problematic for projects that e.g. need to guarantee a
> unbroken chain of trust from SOC all the way into the OS, since the FIT signing
> mechanism cannot be used.
> 
> This patch adds the capability to load such FIT images.
> 
> An example its snippet (utilizing signature verification) might look like the
> following:
> 
> images {
> 	kernel@1 {
> 		description = "2nd stage U-Boot image";
> 		data = /incbin/("u-boot-dtb.img.gz");
> 		type = "kernel";
> 		arch = "arm";
> 		os = "u-boot";
> 		compression = "gzip";
> 		load = <0x8FFFC0>;
> 		entry = <0x900000>;
> 		signature@1 {
> 			algo = "sha256,rsa4096";
> 			key-name-hint = "key";
> 		};
> 	};
> };
> 
> Signed-off-by: Mario Six <mario.six@gdsys.cc>

Reviewed-by: Tom Rini <trini@konsulko.com>
Michal Simek July 19, 2016, 5:46 a.m. UTC | #2
On 18.7.2016 14:05, Mario Six wrote:
> In certain circumstances it comes in handy to be able to boot into a second
> U-Boot. But as of now it is not possible to boot a U-Boot binary that is inside
> a FIT image, which is problematic for projects that e.g. need to guarantee a
> unbroken chain of trust from SOC all the way into the OS, since the FIT signing
> mechanism cannot be used.
> 
> This patch adds the capability to load such FIT images.
> 
> An example its snippet (utilizing signature verification) might look like the
> following:
> 
> images {
> 	kernel@1 {
> 		description = "2nd stage U-Boot image";
> 		data = /incbin/("u-boot-dtb.img.gz");
> 		type = "kernel";

Isn't this type weird for u-boot itself?

The rest is good.

Thanks,
Michal
Mario Six July 19, 2016, 6:45 a.m. UTC | #3
On Tue, Jul 19, 2016 at 7:46 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 18.7.2016 14:05, Mario Six wrote:
>> In certain circumstances it comes in handy to be able to boot into a second
>> U-Boot. But as of now it is not possible to boot a U-Boot binary that is inside
>> a FIT image, which is problematic for projects that e.g. need to guarantee a
>> unbroken chain of trust from SOC all the way into the OS, since the FIT signing
>> mechanism cannot be used.
>>
>> This patch adds the capability to load such FIT images.
>>
>> An example its snippet (utilizing signature verification) might look like the
>> following:
>>
>> images {
>>       kernel@1 {
>>               description = "2nd stage U-Boot image";
>>               data = /incbin/("u-boot-dtb.img.gz");
>>               type = "kernel";
>
> Isn't this type weird for u-boot itself?
>
> The rest is good.
>
> Thanks,
> Michal

It is, but the problem is that adding a new type, like "ubootimage," or
something like that, would not be as easy as with the platform-specific image
types (like, e.g. "rkimage"), which only appear in mkimage, because we'd have
to account for this new type in common/bootm.c (essentially, add a lot of
additional "|| images.os.type == IH_TYPE_UBOOT").

I didn't think that it was worth the hassle, and it would have the additional
minor problem that it would effectively be a synonym of "kernel," so you could
declare a regular kernel as having type "ubootimage," and U-Boot would happily
boot it; to prevent that we'd have to do more checks, like, e.g. check the os
value against the type and only boot if the combination makes sense or some
such.

It would be nicer to have a dedicated type, but I think it's too much effort
for such a fringe use case; should it become popular, though, I'll happily add
a new type :-)

Best regards,

Mario
Michal Simek July 19, 2016, 6:47 a.m. UTC | #4
On 19.7.2016 08:45, Mario Six wrote:
> On Tue, Jul 19, 2016 at 7:46 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 18.7.2016 14:05, Mario Six wrote:
>>> In certain circumstances it comes in handy to be able to boot into a second
>>> U-Boot. But as of now it is not possible to boot a U-Boot binary that is inside
>>> a FIT image, which is problematic for projects that e.g. need to guarantee a
>>> unbroken chain of trust from SOC all the way into the OS, since the FIT signing
>>> mechanism cannot be used.
>>>
>>> This patch adds the capability to load such FIT images.
>>>
>>> An example its snippet (utilizing signature verification) might look like the
>>> following:
>>>
>>> images {
>>>       kernel@1 {
>>>               description = "2nd stage U-Boot image";
>>>               data = /incbin/("u-boot-dtb.img.gz");
>>>               type = "kernel";
>>
>> Isn't this type weird for u-boot itself?
>>
>> The rest is good.
>>
>> Thanks,
>> Michal
> 
> It is, but the problem is that adding a new type, like "ubootimage," or
> something like that, would not be as easy as with the platform-specific image
> types (like, e.g. "rkimage"), which only appear in mkimage, because we'd have
> to account for this new type in common/bootm.c (essentially, add a lot of
> additional "|| images.os.type == IH_TYPE_UBOOT").
> 
> I didn't think that it was worth the hassle, and it would have the additional
> minor problem that it would effectively be a synonym of "kernel," so you could
> declare a regular kernel as having type "ubootimage," and U-Boot would happily
> boot it; to prevent that we'd have to do more checks, like, e.g. check the os
> value against the type and only boot if the combination makes sense or some
> such.
> 
> It would be nicer to have a dedicated type, but I think it's too much effort
> for such a fringe use case; should it become popular, though, I'll happily add
> a new type :-)

Is the type property even required for this case?

Thanks,
Michal
Mario Six July 19, 2016, 7:04 a.m. UTC | #5
On Tue, Jul 19, 2016 at 8:47 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 19.7.2016 08:45, Mario Six wrote:
>> On Tue, Jul 19, 2016 at 7:46 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>>> On 18.7.2016 14:05, Mario Six wrote:
>>>> In certain circumstances it comes in handy to be able to boot into a second
>>>> U-Boot. But as of now it is not possible to boot a U-Boot binary that is inside
>>>> a FIT image, which is problematic for projects that e.g. need to guarantee a
>>>> unbroken chain of trust from SOC all the way into the OS, since the FIT signing
>>>> mechanism cannot be used.
>>>>
>>>> This patch adds the capability to load such FIT images.
>>>>
>>>> An example its snippet (utilizing signature verification) might look like the
>>>> following:
>>>>
>>>> images {
>>>>       kernel@1 {
>>>>               description = "2nd stage U-Boot image";
>>>>               data = /incbin/("u-boot-dtb.img.gz");
>>>>               type = "kernel";
>>>
>>> Isn't this type weird for u-boot itself?
>>>
>>> The rest is good.
>>>
>>> Thanks,
>>> Michal
>>
>> It is, but the problem is that adding a new type, like "ubootimage," or
>> something like that, would not be as easy as with the platform-specific image
>> types (like, e.g. "rkimage"), which only appear in mkimage, because we'd have
>> to account for this new type in common/bootm.c (essentially, add a lot of
>> additional "|| images.os.type == IH_TYPE_UBOOT").
>>
>> I didn't think that it was worth the hassle, and it would have the additional
>> minor problem that it would effectively be a synonym of "kernel," so you could
>> declare a regular kernel as having type "ubootimage," and U-Boot would happily
>> boot it; to prevent that we'd have to do more checks, like, e.g. check the os
>> value against the type and only boot if the combination makes sense or some
>> such.
>>
>> It would be nicer to have a dedicated type, but I think it's too much effort
>> for such a fringe use case; should it become popular, though, I'll happily add
>> a new type :-)
>
> Is the type property even required for this case?
>
> Thanks,
> Michal

I just tried to boot one without the type, and apparently it is required:

## Loading kernel from FIT Image at 02000000 ...
   Using 'config@1' configuration
   Verifying Hash Integrity ... OK
   Trying 'kernel@1' kernel subimage
     Description:  2nd stage U-Boot image
     Type:         Unknown Image
     Compression:  gzip compressed
     Data Start:   0x020000d4
     Data Size:    206485 Bytes = 201.6 KiB
     Sign algo:    sha256,rsa4096:ccdc
     Sign value:   ...
   Verifying Hash Integrity ... sha256,rsa4096:ccdc+ OK
No U-Boot ARM Kernel Image Image
ERROR: can't get kernel image!

That makes sense, since common/bootm.c checks the image type in several places.

Best regards,

Mario
Wolfgang Denk July 19, 2016, 7:14 a.m. UTC | #6
Dear Mario,

In message <CAN1kZorTfQcVoOz5O5J7-R-T5yTDbrCqDBHgC4gHaMX1sUh3YQ@mail.gmail.com> you wrote:
>
> It is, but the problem is that adding a new type, like "ubootimage," or
> something like that, would not be as easy as with the platform-specific image
> types (like, e.g. "rkimage"), which only appear in mkimage, because we'd have
> to account for this new type in common/bootm.c (essentially, add a lot of
> additional "|| images.os.type == IH_TYPE_UBOOT").

There is no need for a new type.  IH_TYPE_FIRMWARE should be used for
this; see also comments around [1]

[1] http://git.denx.de/?p=u-boot.git;a=blob;f=include/image.h#l228

Best regards,

Wolfgang Denk
Mario Six July 19, 2016, 7:41 a.m. UTC | #7
Hi Wolfgang,

On Tue, Jul 19, 2016 at 9:14 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Mario,
>
> In message <CAN1kZorTfQcVoOz5O5J7-R-T5yTDbrCqDBHgC4gHaMX1sUh3YQ@mail.gmail.com> you wrote:
>>
>> It is, but the problem is that adding a new type, like "ubootimage," or
>> something like that, would not be as easy as with the platform-specific image
>> types (like, e.g. "rkimage"), which only appear in mkimage, because we'd have
>> to account for this new type in common/bootm.c (essentially, add a lot of
>> additional "|| images.os.type == IH_TYPE_UBOOT").
>
> There is no need for a new type.  IH_TYPE_FIRMWARE should be used for
> this; see also comments around [1]
>
> [1] http://git.denx.de/?p=u-boot.git;a=blob;f=include/image.h#l228
>
> Best regards,
>
> Wolfgang Denk
>

Ah, OK, I thought that IH_TYPE_FIRMWARE should exclusively be used for firmware
updates, like the comment said, not for actually *running* the firmware. In
that case the work is more manageable (there's still some changes needed,
because common/image-fit.c refuses to boot images with firmware type, too).

I'll look into preparing a v2.

Thanks for the help!

Best regards,

Mario
diff mbox

Patch

diff --git a/common/image-fit.c b/common/image-fit.c
index 6f920da..0c7b400 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1690,6 +1690,7 @@  int fit_image_load(bootm_headers_t *images, ulong addr,

 	os_ok = image_type == IH_TYPE_FLATDT || IH_TYPE_FPGA ||
 		fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
+		fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
 		fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);

 	/*