Re: [PATCH 4/4] commands: hab: extend by field_return fuse burn

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

On 25-12-18, Fabian Pflug wrote:
> Extend hab command with an additional parameter to burn the field return
> fuse, but only if it is unlocked via the kconfig option.
> Without the kconfig option, the extra argument makes no sense, as it
> would not be possible to access the FIELD_RETURN fuse.
> 
> Signed-off-by: Fabian Pflug <f.pflug@pengutronix.de>
> ---
>  arch/arm/mach-imx/Kconfig |  6 +++++-
>  commands/hab.c            | 20 +++++++++++++++++---
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index 5f50d1a823..5fea0bbbca 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -926,13 +926,17 @@ config HABV4_CSF_UNLOCK_UID
>            feature. This value must match the per device UNIQUE_ID fuses.
>  
>  	  The below example shows the expected format. The UNIQUE_ID is
> -	  queried by Linux via:
> +	  printed during boot by barebox:
> +	    i.MX___ unique ID: 7766554433221100
> +	  or it can be queried by Linux via:
>              - cat /sys/devices/soc0/serial_number
>  	      7766554433221100
>  
>  	  So this value have to be set:
>  	    - 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77
>  
> +	  Afterwards, the `hab -p -r` command can be used to burn the fuse.

This Kconfig update should be done in a separate patch or at least
mentioned within the commit message.

> +
>  config HABV4_IMG_CRT_PEM
>  	string "Path to IMG certificate"
>  	default "../crts/IMG1_1_sha256_4096_65537_v3_usr_crt.pem"
> diff --git a/commands/hab.c b/commands/hab.c
> index 8ae943a4c8..435c20f4d3 100644
> --- a/commands/hab.c
> +++ b/commands/hab.c
> @@ -16,9 +16,9 @@ static int do_hab(int argc, char *argv[])
>  	char *srkhashfile = NULL, *srkhash = NULL;
>  	unsigned flags = 0;
>  	u8 srk[SRK_HASH_SIZE];
> -	int lockdown = 0, info = 0;
> +	int lockdown = 0, info = 0, field_return = 0;
>  
> -	while ((opt = getopt(argc, argv, "s:fpx:li")) > 0) {
> +	while ((opt = getopt(argc, argv, "s:fpx:lir")) > 0) {
>  		switch (opt) {
>  		case 's':
>  			srkhashfile = optarg;
> @@ -38,12 +38,16 @@ static int do_hab(int argc, char *argv[])
>  		case 'i':
>  			info = 1;
>  			break;
> +		case 'r':
> +			field_return = 1;
> +			break;
>  		default:
>  			return COMMAND_ERROR_USAGE;
>  		}
>  	}
>  
> -	if (!info && !lockdown && !srkhashfile && !srkhash) {
> +	if (!info && !lockdown && !srkhashfile && !srkhash &&
> +	    !(IS_ENABLED(CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN) && field_return)) {
		^
I would drop the CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN check here and
instead just check the: !field_return option...

>  		printf("Nothing to do\n");
>  		return COMMAND_ERROR_USAGE;
>  	}
> @@ -94,6 +98,13 @@ static int do_hab(int argc, char *argv[])
>  		printf("Device successfully locked down\n");
>  	}
>  
> +	if (IS_ENABLED(CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN) && field_return) {

same here..

	if (field_return) {

> +		ret = imx_hab_field_return(flags & IMX_SRK_HASH_WRITE_PERMANENT);
> +		if (ret)
> +			return ret;

and instead check the return value here. If -EINVAL is returned, the
sticky bit is still locked because either the CSF SOC_UID is wrong or
the CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN wasn't enabled at all.
Therefore:

		if (ret) {
			if (ret == -EINVAL && IS_ENABLED(CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN))
				printf("Field-return burn failed, check HABV4_CSF_UNLOCK_UID!\n");
			else if (ret == -EINVAL && !IS_ENABLED(CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN))
				printf("Field-return burn failed because CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN=n\n");
			else
				printf("Field-return burn failed\n");

			return ret;
		}

> +		printf("Field return fuse successfully burnt\n");
> +	}
> +
>  	return 0;
>  }
>  
> @@ -105,6 +116,9 @@ BAREBOX_CMD_HELP_OPT ("-x <sha256>",  "Burn Super Root Key hash from hex string"
>  BAREBOX_CMD_HELP_OPT ("-i",  "Print HAB info")
>  BAREBOX_CMD_HELP_OPT ("-f",  "Force. Write even when a key is already written")
>  BAREBOX_CMD_HELP_OPT ("-l",  "Lockdown device. Dangerous! After executing only signed images can be booted")
> +#ifdef CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN

IMHO we shouldn't add conditional compilation here.

Regards,
  Marco


> +BAREBOX_CMD_HELP_OPT ("-r",  "Field Return. Dangerous! After executing signed images are disabled forever.")
> +#endif // CONFIG_HABV4_CSF_UNLOCK_FIELD_RETURN
>  BAREBOX_CMD_HELP_OPT ("-p",  "Permanent. Really burn fuses. Be careful!")
>  BAREBOX_CMD_HELP_END
>  
> 
> -- 
> 2.47.3
> 
> 

-- 
#gernperDu 
#CallMeByMyFirstName

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |