Re: [PATCH v2 7/9] i.MX8M: HABv4: add an option to allow key revocation

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Ahmad,

On 24-07-03, Ahmad Fatoum wrote:
> Hello Marco,
> 
> On 03.07.24 19:20, Marco Felsch wrote:
> > The HAB code needs an special [Unlock] instruction to keep the
> > SRK_REVOKE fuse bank unlocked. This is required if a key needs to be
> > revoked.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  arch/arm/mach-imx/Kconfig            | 8 ++++++++
> >  include/mach/imx/habv4-imx8-gencsf.h | 6 ++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > index 61258137736f..68f55971506b 100644
> > --- a/arch/arm/mach-imx/Kconfig
> > +++ b/arch/arm/mach-imx/Kconfig
> > @@ -835,6 +835,14 @@ config HABV4_QSPI
> >  	help
> >  	  Enable this option to build signed QSPI/FlexSPI images.
> >  
> > +config HABV4_CSF_UNLOCK_SRK_REVOKE
> > +	depends on HABV4
> > +	bool "Unlock SRK revocation"
> > +	help
> > +	  Enable this option to instruct the HAB code to not lock
> > +	  the SRK_REVOKE_LOCK sticky bit. This is required for key
> > +	  revocation. Don't enable this if you are unsure.
> 
> I think for added safety we should have an extra option that prompts
> for the key to be revoked and an initcall that is activated depending
> on it, e.g.:
> 
> config HABV4_CSF_SRK_REVOKE_INDEX
>         int "SRK to revoke"
>         range 0 3
>         default 0
>         depends on HABV4_CSF_SRK_REVOKE_UNLOCK
>         help
>           Which of the first three SRKs to revoke. The SRK indices are
>           1-based. Saying 0 here will just print the SRK Revocation
>           register without modification. SRK #4 is immutable.
>                                                                        
>           Proceed with caution, revoking a SRK is irreversible and
>           manual manipulation of this code can brick the board!
>                                                                        
> if HABV4_CSF_SRK_REVOKE_INDEX = HABV4_SRK_INDEX
> comment "Can't revoke same SRK used for signing"
> comment "Attempts to build a signed barebox image will fail"
> endif
> 
> and then some code that checks the same above condition during final
> assembly of the signed image.
> 
> What do you think?

That's an good idea to make it more user-friendly for most users :)
Regarding this patchset I do see it more as an addition since for my
project the revocation is checked on every startup and we do allow the
revocation of multiple SRK slots at the same time.

Regards,
  Marco

> >  config HAB_CERTS_ENV
> >  	depends on HAB
> >  	bool "Specify certificates in environment"
> > diff --git a/include/mach/imx/habv4-imx8-gencsf.h b/include/mach/imx/habv4-imx8-gencsf.h
> > index 5f92ceceab00..56d9ef2de92f 100644
> > --- a/include/mach/imx/habv4-imx8-gencsf.h
> > +++ b/include/mach/imx/habv4-imx8-gencsf.h
> > @@ -36,6 +36,12 @@ hab [Unlock]
> >  hab Engine = CAAM
> >  hab Features = RNG, MID
> >  
> > +#if defined(CONFIG_HABV4_CSF_UNLOCK_SRK_REVOKE)
> > +hab [Unlock]
> > +hab Engine = OCOTP
> > +hab Features = SRK REVOKE
> > +#endif
> > +
> >  hab [Install Key]
> >  /* verification key index in key store (0, 2...4) */
> >  hab Verification index = 0
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
>