diff mbox series

spl: allow board_spl_fit_post_load() to fail

Message ID 20200509183404.GA6495@nox.fritz.box
State Superseded
Headers show
Series spl: allow board_spl_fit_post_load() to fail | expand

Commit Message

Patrick Wildt May 9, 2020, 6:34 p.m. UTC
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 mbox series

Patch

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