[PATCH] ARM: imx: hab: panic on authentication failure

[Patrick Wildt]

On Sun, May 31, 2020 at 06:51:14PM +0200, Marek Vasut wrote:
> On 5/31/20 5:53 PM, Patrick Wildt wrote:
> > On Sun, May 31, 2020 at 05:38:05PM +0200, Marek Vasut wrote:
> >> On 5/30/20 10:53 PM, Patrick Wildt wrote:
> >>> On Sat, May 30, 2020 at 10:29:19PM +0200, Marek Vasut wrote:
> >>>> On 5/30/20 10:14 PM, Patrick Wildt wrote:
> >>>>> On Sat, May 30, 2020 at 03:31:29PM -0300, Fabio Estevam wrote:
> >>>>>> Hi Marek,
> >>>>>>
> >>>>>> [Adding Breno]
> >>>>>>
> >>>>>> On Sat, May 30, 2020 at 3:29 PM Marek Vasut <marex at denx.de> wrote:
> >>>>>>>
> >>>>>>> Instead of hang()ing the system and thus disallowing any automated
> >>>>>>> recovery possibility from a HAB authentication failure, panic() .
> >>>>>>> The panic() function can be configured to hang() the system after
> >>>>>>> printing an error message, however the default is to reset the
> >>>>>>> system instead.
> >>>>>>>
> >>>>>>> This allows redundant boot to work correctly. In case the primary
> >>>>>>> or secondary image cannot be authenticated, the system reboots and
> >>>>>>> bootrom can try to start the other one.
> >>>>>>>
> >>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>>>>> Cc: Fabio Estevam <festevam at gmail.com>
> >>>>>>> Cc: NXP i.MX U-Boot Team <uboot-imx at nxp.com>
> >>>>>>> Cc: Peng Fan <peng.fan at nxp.com>
> >>>>>>> Cc: Stefano Babic <sbabic at denx.de>
> >>>>>>
> >>>>>> This is a better behavior indeed:
> >>>>>>
> >>>>>> Reviewed-by: Fabio Estevam <festevam at gmail.com>
> >>>>>
> >>>>> What about this?  Have you ignored this patch for a reason? :/
> >>>>>
> >>>>> https://marc.info/?l=u-boot&m=159069441005730&w=2
> >>>>
> >>>> Yes, and the reason is I was not even aware of your patch, sorry. The CC
> >>>> list in this mail should cover all the interested parties, so use it
> >>>> when sending V2, or use patman.
> >>>
> >>> I already had 11 people on CC, but apparently I missed you.
> >>>
> >>>> The patch looks fine, one nit is that you should return errno.h return
> >>>> value and another is that it changes the current behavior. Now that I
> >>>> look at this imx code, board_spl_fit_post_load() should not even be in
> >>>> arch/ , sigh, but that's for separate patch either way.
> >>>>
> >>>> So I think if you want to support this sort of fallback, you should make
> >>>> the board_spl_fit_post_load() be in board/ files, with default __weak
> >>>> implementation calling some arch_hab_authenticate...() which implements
> >>>> current content of board_spl_fit_post_load(), and let boards decide how
> >>>> to handle the fallback if it needs to be altered.
> >>>>
> >>>> Would that work ?
> >>>
> >>> I'm not sure.  In comparison to the people from NXP who are paid to
> >>> upstream their code and still don't do it correctly, I'm doing this
> >>> in my spare time and I'm not sure I want to bikeshed all day long.
> >>>
> >>> I can send a V3 that replaces the -1 with EINVAL, EACCESS, EPERM or
> >>> something the like.  If you want to clean up after NXP, feel free to.
> >>
> >> In fact, what is it that you're trying to achieve with this fallback ?
> >> What are you falling back to , another fallback fitImage ?
> > 
> > Exactly.
> > 
> > I have a device with a glued enclosure, with no external sources, apart
> > from a serial console.  If the SPL fails to load the U-Boot main image
> > from eMMC, the only way to fix it is to destroy the case, open up the
> > enclosure and short some lines.  That's not really nice, since we'd have
> > to get a new enclosure, new serial number label,... it's a hassle.
> 
> Look for SRC PERSIST_SECONDARY_BOOT in your datasheet then. This will
> let you use two copies of SPL, two copies of U-Boot, etc.

I'll have a look!

> > If the SPL was gone as well, the BootROM would fall back to other
> > sources.  But with only one half of U-Boot missing, it would just hang.
> > I'm sure that's also why you replace the hang() with a panic().
> > 
> > I thought that if the SPL is still fine, but only the U-Boot image was
> > gone, why not make use of the spl_boot_list to try and boot from another
> > source?  Like yModem.  For that I sent the following fix, also with many
> > people CCd:
> > 
> > https://marc.info/?l=u-boot&m=158893200030861&w=2
> > 
> > Now the board spl boot order can have eMMC as first, and yModem as
> > second.  If eMMC fails, it falls back to yModem.  If none work, it
> > though hang()s, doing
> > 
> > 	puts(SPL_TPL_PROMPT "failed to boot from all boot devices\n");
> > 	hang();
> > 
> > Maybe you want this as panic instead?
> > 
> > Anyway, I think this is nicer option for recovery, instead of simply
> > hanging or rebooting.
> 
> The problem is, this only works for fitImage and not for raw uImage. But
> in your case, that is probably OK.
> 
> So if you can just fix the errno return value to some -EINVAL (?) and
> send a V3, I think that would be good.

Ok, will do, thanks!