On Sat, May 09, 2020 at 08:09:39PM +0200, Heinrich Schuchardt wrote:

On 5/9/20 7:45 PM, Patrick Wildt wrote:

...

On Sat, May 09, 2020 at 06:38:33PM +0200, Heinrich Schuchardt wrote:

...

On 5/9/20 6:13 PM, Patrick Wildt wrote:

...

On i.MX platforms board_spl_fit_post_load() can check the loaded SPL image for authenticity using its HAB engine. U-Boot's SPL mechanism allows booting images from other sources as well, but in the current setup the SPL would just hang if it encounters an image that does not pass scrutiny. Allowing the function to return an error, allows the SPL to try booting from another source as a fallback instead of ending up as a brick.

Signed-off-by: Patrick Wildt patrick@blueri.se

Could an intruder abuse this by destroying a signed image and providing an unsigned image on a source under his control?

Best regards

Heinrich

Sure, let's think about it here. Maybe you have some more thoughts to add.

First of all, the SPL goes through all the boot devices, and if there's none to find with an image, it will hang. It will hang like it does before the diff. The only difference is that it tries additional sources before hanging. Thus the attacker, unless he can exploit it in his first trial, or is able to force a reset, must have some access to reset the machine to have it boot and try again. This seems like he must have some kind of local or remote phyiscal access.

Let's assume the image is on the network or on another remote medium. Then I guess the attacker will just try to attack that medium, and the alternate boot sources won't make a difference.

I guess that means we should focus on local sources. I think we can also ignore a removable SD card, since he can just put in another one and try again.

So maybe let's think about SPI flash and eMMC, soldered on, not directly accessible. If he has physical access, I guess he could open up the box and desolder a few pins, or add some voltage here and there to try and disrupt the bootup. But, then maybe it's easier to just desolder the whole SPI/eMMC and add his own.

But what if he doesn't have that access? If he's remote? Ok, he will probably have to exploit the daemon (webserver or whatever) to gain some code execution. Then he'll try and become root, so he can access the disks. Then I figure he'll try and overwrite or remove the image. With this, on the next reboot it will (hopefully) fail to boot, unless he already has an exploit, then my patch won't make a difference.

I figure the real issue could be that when the attacker has physical access, manages to remove/replace the image with a fallback to load from a device like an SD card, that it's now easier for him to try and find an exploit.

This last scenario is what I was thinking of.

So if HAB is used it may be ok to search all devices for signed images but non-signed images should be rejected. Is that given with your patch?

Best regards

Heinrich

I think you have a very good point there. No, with that diff I think there's an issue, but I have added a second diff here. I'll explain.

i.MX's jump_to_image_no_args() has a check:

if (spl_image->flags & SPL_FIT_FOUND) { image_entry(); } else { [...] if (!imx_hab_authenticate_image(spl_image->load_addr,

That means that it will jump right away if the flags say SPL_FIT_FOUND. Unfortunately my diff sets that flag before returning an error, and that means if the fallback tried to boot a non-FIT, it will skip the HAB check and jump right away. I have also not (yet) seen code that resets the flag.

I figure we should just set the flag after doing the post load. Since it's a "fit_post_load"-function, we can assume that that function does not depend on the SPL_FIT_FOUND flag to do its work. I mean, it's only called because it parses a FIT in the first place.

Thanks for making me think about this again!

Best regards, Patrick

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index fd3fa04600..b8f6fcb4df 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -311,7 +311,7 @@ ulong board_spl_fit_size_align(ulong size) return size; }

-void board_spl_fit_post_load(ulong load_addr, size_t length) +int board_spl_fit_post_load(ulong load_addr, size_t length) { u32 offset = length - CONFIG_CSF_SIZE;

@@ -319,8 +319,10 @@ void board_spl_fit_post_load(ulong load_addr, size_t length) offset + IVT_SIZE + CSF_PAD_SIZE, offset)) { puts("spl: ERROR: image authentication unsuccessful\n"); - hang(); + return -1; } + + return 0; } #endif

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index c51e4beb1c..7e9aff9240 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -24,8 +24,9 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_SYS_BOOTM_LEN (64 << 20) #endif

-__weak void board_spl_fit_post_load(ulong load_addr, size_t length) +__weak int board_spl_fit_post_load(ulong load_addr, size_t length) { + return 0; }

__weak ulong board_spl_fit_size_align(ulong size) @@ -675,11 +676,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (spl_image->entry_point == FDT_ERROR || spl_image->entry_point == 0) spl_image->entry_point = spl_image->load_addr;

- spl_image->flags |= SPL_FIT_FOUND; - #ifdef CONFIG_IMX_HAB - board_spl_fit_post_load((ulong)fit, size); + ret = board_spl_fit_post_load((ulong)fit, size); + if (ret) + return ret; #endif

+ spl_image->flags |= SPL_FIT_FOUND; return 0; } diff --git a/include/spl.h b/include/spl.h index 6bf9fd8beb..93d5a5a1f3 100644 --- a/include/spl.h +++ b/include/spl.h @@ -560,7 +560,7 @@ int board_return_to_bootrom(struct spl_image_info *spl_image, * board_spl_fit_post_load - allow process images after loading finished * */ -void board_spl_fit_post_load(ulong load_addr, size_t length); +int board_spl_fit_post_load(ulong load_addr, size_t length);

/** * board_spl_fit_size_align - specific size align before processing payload