spl: allow board_spl_fit_post_load() to fail
Patrick Wildt
patrick at blueri.se
Sat May 9 20:34:04 CEST 2020
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 at 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
More information about the U-Boot
mailing list