spl: allow board_spl_fit_post_load() to fail

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