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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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?

> +
>  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 |