[U-Boot] [PATCH 0/4] imx: hab: Add helper functions for scripted HAB auth

[U-Boot] [PATCH 0/4] imx: hab: Add helper functions for scripted HAB auth

[Bryan O'Donoghue]

Greetings.

This set adds some helper functions as a pre-cursor to an upcoming set of
changes to a BSP adding scripted HAB authentication.

Calculating a HAB IVT address based on a base address and a +/- offset is a
trivial but, useful function for HAB. It means you can have a load address
for a HAB image inside of your environment and specify the IVT offset
relative to that address. All you need to do then is to call the function
to obtain the correct IVT address to pass into hab_auth_img.

Two relatively minor changes then - one encasing the hab.h in ifndef
__ASSEMBLY__ which is required if you want to include hab.h in a board.h.

Specifying the IVT padding size is again properly done as a define as
opposed to a magic number in code.

The final patch then is wrappering up two common use-cases in the upcoming
BSP
- hab_auth_image ? continue-to-boot : drop-to-bootrom USB mode.

In other words if you fail to authenticate an image on the secure-boot path
the appropriate next step is typically to drop into USB recovery mode.

In USB recovery mode you need to provide a signed image on a secure-boot
(closed in the parlance) board. So hab_auth_img_or_fail() encapsulates that
behaviour in one place - again allowing for scripting to reuse instead of
replicate functionality over and over again.

These helper functions could all be buried in the board-port but, they are
made available here in the hopes they will be of use to others.

Bryan O'Donoghue (4):
  imx: hab: Add routine to set HAB IVT address
  imx: hab: Encase majority of header in __ASSEMBLY__ declaration
  imx: hab: Specify IVT padding size
  imx: hab: Provide hab_auth_img_or_fail command

 arch/arm/include/asm/mach-imx/hab.h |  9 ++++--
 arch/arm/mach-imx/hab.c             | 59 +++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 3 deletions(-)

-- 
2.7.4

[U-Boot] [PATCH 1/4] imx: hab: Add routine to set HAB IVT address

[Bryan O'Donoghue]

This patch takes a given address applies a plus or minus offset to locate
the putative address of an IVT given a non-IVT link location.

It then sets hab_ivt_address to allow for further logic/scripting based on
the calculated address.

This routine is useful when scripting hab_auth_img calls from boot.scr.
Subsequent patches will illustrate its utility in a board-port.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
Cc: Breno Lima <breno.lima@nxp.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/mach-imx/hab.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
index c730c8f..0c18b2e 100644
--- a/arch/arm/mach-imx/hab.c
+++ b/arch/arm/mach-imx/hab.c
@@ -341,6 +341,31 @@ static int do_hab_failsafe(cmd_tbl_t *cmdtp, int flag, int argc,
 	return 0;
 }
 
+/*
+ * This routine takes a given address and applies a plus or minus offset to that
+ * address.
+ */
+static int do_hab_get_ivt_addr(cmd_tbl_t *cmdtp, int flag, int argc,
+			       char * const argv[])
+{
+	ulong	addr;
+	long	ivt_offset;
+
+	if (argc < 3)
+		return CMD_RET_USAGE;
+
+	if (!imx_hab_is_enabled())
+		return CMD_RET_FAILURE;
+
+	addr = simple_strtoul(argv[1], NULL, 16);
+	ivt_offset = simple_strtol(argv[2], NULL, 16);
+	addr += ivt_offset;
+
+	env_set_hex("hab_ivt_addr", addr);
+
+	return CMD_RET_SUCCESS;
+}
+
 U_BOOT_CMD(
 		hab_status, CONFIG_SYS_MAXARGS, 1, do_hab_status,
 		"display HAB status",
@@ -362,6 +387,14 @@ U_BOOT_CMD(
 		""
 	  );
 
+U_BOOT_CMD(
+		hab_get_ivt_addr, 3, 0, do_hab_get_ivt_addr,
+		"determine IVT header location and store in $hab_ivt_addr",
+		"addr ivt_offset\n"
+		"addr - image hex address\n"
+		"ivt_offset - hex offset of IVT in the image"
+	  );
+
 #endif /* !defined(CONFIG_SPL_BUILD) */
 
 /* Get CSF Header length */
-- 
2.7.4

[U-Boot] [PATCH 1/4] imx: hab: Add routine to set HAB IVT address

[Breno Matheus Lima]

Hi Bryan,

2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
> This patch takes a given address applies a plus or minus offset to locate
> the putative address of an IVT given a non-IVT link location.
>
> It then sets hab_ivt_address to allow for further logic/scripting based on
> the calculated address.
>
> This routine is useful when scripting hab_auth_img calls from boot.scr.
> Subsequent patches will illustrate its utility in a board-port.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> Cc: Breno Lima <breno.lima@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>

Tested-by: <breno.lima@nxp.com>

Thanks,
Breno Lima

[U-Boot] [PATCH 1/4] imx: hab: Add routine to set HAB IVT address

[Lothar Waßmann]

Hi,

On Fri,  9 Mar 2018 17:35:46 +0000 Bryan O'Donoghue wrote:
> This patch takes a given address applies a plus or minus offset to locate
> the putative address of an IVT given a non-IVT link location.
> 
> It then sets hab_ivt_address to allow for further logic/scripting based on
> the calculated address.
> 
> This routine is useful when scripting hab_auth_img calls from boot.scr.
> Subsequent patches will illustrate its utility in a board-port.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> Cc: Breno Lima <breno.lima@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>  arch/arm/mach-imx/hab.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
> index c730c8f..0c18b2e 100644
> --- a/arch/arm/mach-imx/hab.c
> +++ b/arch/arm/mach-imx/hab.c
> @@ -341,6 +341,31 @@ static int do_hab_failsafe(cmd_tbl_t *cmdtp, int flag, int argc,
>  	return 0;
>  }
>  
> +/*
> + * This routine takes a given address and applies a plus or minus offset to that
> + * address.
> + */
> +static int do_hab_get_ivt_addr(cmd_tbl_t *cmdtp, int flag, int argc,
> +			       char * const argv[])
> +{
> +	ulong	addr;
> +	long	ivt_offset;
> +
> +	if (argc < 3)
> +		return CMD_RET_USAGE;
> +
> +	if (!imx_hab_is_enabled())
> +		return CMD_RET_FAILURE;
> +
> +	addr = simple_strtoul(argv[1], NULL, 16);
> +	ivt_offset = simple_strtol(argv[2], NULL, 16);
> +	addr += ivt_offset;
> +
> +	env_set_hex("hab_ivt_addr", addr);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  U_BOOT_CMD(
>  		hab_status, CONFIG_SYS_MAXARGS, 1, do_hab_status,
>  		"display HAB status",
>
What does this function offer, that a
'setexpr hab_ivt_addr ${loadaddr} + 0x400' could not do as well?


Lothar Waßmann

[U-Boot] [PATCH 1/4] imx: hab: Add routine to set HAB IVT address

[Bryan O'Donoghue]

On 16/03/18 08:17, Lothar Waßmann wrote:
> 'setexpr hab_ivt_addr ${loadaddr} + 0x400' could not do as well?

That might work too. I'll check.

[U-Boot] [PATCH 2/4] imx: hab: Encase majority of header in __ASSEMBLY__ declaration

[Bryan O'Donoghue]

Subsequent patches will want to include hab.h but in doing so include it on
an assembly compile path causing a range of compile errors. Fix the errors
pre-emptively by encasing the majority of the declarations in hab.h inside
an ifdef __ASSEMBLY__ block.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
Cc: Breno Lima <breno.lima@nxp.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/include/asm/mach-imx/hab.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/mach-imx/hab.h b/arch/arm/include/asm/mach-imx/hab.h
index ce9a44d..1bebdbe 100644
--- a/arch/arm/include/asm/mach-imx/hab.h
+++ b/arch/arm/include/asm/mach-imx/hab.h
@@ -8,6 +8,7 @@
 #ifndef __SECURE_MX6Q_H__
 #define __SECURE_MX6Q_H__
 
+#ifndef __ASSEMBLY__
 #include <linux/types.h>
 #include <linux/compiler.h>
 
@@ -196,13 +197,14 @@ typedef void hapi_clock_init_t(void);
 #define HAB_CMD_SET          0xB1  /* Set command tag */
 #define HAB_PAR_MID          0x01  /* MID parameter value */
 
-#define IVT_SIZE			0x20
-#define CSF_PAD_SIZE			0x2000
-
 /* ----------- end of HAB API updates ------------*/
 
 int imx_hab_authenticate_image(uint32_t ddr_start, uint32_t image_size,
 			       uint32_t ivt_offset);
 bool imx_hab_is_enabled(void);
+#endif /* __ASSEMBLY__ */
+
+#define IVT_SIZE			0x20
+#define CSF_PAD_SIZE			0x2000
 
 #endif
-- 
2.7.4

[U-Boot] [PATCH 2/4] imx: hab: Encase majority of header in __ASSEMBLY__ declaration

[Breno Matheus Lima]

Hi Bryan,

2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
> Subsequent patches will want to include hab.h but in doing so include it on
> an assembly compile path causing a range of compile errors. Fix the errors
> pre-emptively by encasing the majority of the declarations in hab.h inside
> an ifdef __ASSEMBLY__ block.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> Cc: Breno Lima <breno.lima@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>

Tested-by: <breno.lima@nxp.com>

Thanks,
Breno Lima

[U-Boot] [PATCH 3/4] imx: hab: Specify IVT padding size

[Bryan O'Donoghue]

This patch adds IVT_PAD_SIZE at 0xC00. The IVT header is padded to this
size. Defining the size explicitly makes it possible to use the define to
locate the start/end of an IVT header without using magic numbers in code.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
Cc: Breno Lima <breno.lima@nxp.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/include/asm/mach-imx/hab.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/include/asm/mach-imx/hab.h b/arch/arm/include/asm/mach-imx/hab.h
index 1bebdbe..f903e3e 100644
--- a/arch/arm/include/asm/mach-imx/hab.h
+++ b/arch/arm/include/asm/mach-imx/hab.h
@@ -205,6 +205,7 @@ bool imx_hab_is_enabled(void);
 #endif /* __ASSEMBLY__ */
 
 #define IVT_SIZE			0x20
+#define IVT_PAD_SIZE			0xC00
 #define CSF_PAD_SIZE			0x2000
 
 #endif
-- 
2.7.4

[U-Boot] [PATCH 3/4] imx: hab: Specify IVT padding size

[Breno Matheus Lima]

Hi Bryan,

2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
> This patch adds IVT_PAD_SIZE at 0xC00. The IVT header is padded to this
> size. Defining the size explicitly makes it possible to use the define to
> locate the start/end of an IVT header without using magic numbers in code.
>

As far as I know the 0xC00 pad size is only mandatory in U-Boot/SPL
images, for instance in some U-Boot images the first 0xC00 should
include IVT + Boot data + DCD table + padding to 0xC00.
Are you using the IVT_PAD_SIZE in your current code? Or this macro
will be used in a next series?

Thanks,
Breno Lima

[U-Boot] [PATCH 3/4] imx: hab: Specify IVT padding size

[Bryan O'Donoghue]

On 15/03/18 16:54, Breno Matheus Lima wrote:
> Hi Bryan,
> 
> 2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
>> This patch adds IVT_PAD_SIZE at 0xC00. The IVT header is padded to this
>> size. Defining the size explicitly makes it possible to use the define to
>> locate the start/end of an IVT header without using magic numbers in code.
>>
> 
> As far as I know the 0xC00 pad size is only mandatory in U-Boot/SPL
> images, for instance in some U-Boot images the first 0xC00 should
> include IVT + Boot data + DCD table + padding to 0xC00.
> Are you using the IVT_PAD_SIZE in your current code? Or this macro
> will be used in a next series?
> 
> Thanks,
> Breno Lima
> 

All of my images - kernel, u-boot, optee, boot.scr are signed in the 
same first-stage format with this padding present - for simplicity.

Maybe this better named "BOOTROM_IVT_PAD_SIZE" since only the bootrom 
requires it.

[U-Boot] [PATCH 3/4] imx: hab: Specify IVT padding size

[Breno Matheus Lima]

Hi Bryan,

2018-03-17 8:06 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
>
>
> On 15/03/18 16:54, Breno Matheus Lima wrote:
>>
>> Hi Bryan,
>>
>> 2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
>>>
>>> This patch adds IVT_PAD_SIZE at 0xC00. The IVT header is padded to this
>>> size. Defining the size explicitly makes it possible to use the define to
>>> locate the start/end of an IVT header without using magic numbers in
>>> code.
>>>
>>
>> As far as I know the 0xC00 pad size is only mandatory in U-Boot/SPL
>> images, for instance in some U-Boot images the first 0xC00 should
>> include IVT + Boot data + DCD table + padding to 0xC00.
>> Are you using the IVT_PAD_SIZE in your current code? Or this macro
>> will be used in a next series?
>>
>> Thanks,
>> Breno Lima
>>
>
> All of my images - kernel, u-boot, optee, boot.scr are signed in the same
> first-stage format with this padding present - for simplicity.
>
> Maybe this better named "BOOTROM_IVT_PAD_SIZE" since only the bootrom
> requires it.

As far as I know the IVT has a fixed size of 0x20, and the set of IVT
+ Boot Data + DCD cannot exceed 0xC00.

This value is calculated by the following operation in function
imximage_generate() at tools/imximage.c:
alloc_len = imximage_init_loadsize - imximage_ivt_offset;

Maybe we can rename to something like UBOOT_IMX_HDR_SIZE? We also have
to ensure this definition is being used by U-Boot code.

Thanks,
Breno Lima

[U-Boot] [PATCH 3/4] imx: hab: Specify IVT padding size

[Bryan O'Donoghue]

On 20/03/18 01:53, Breno Matheus Lima wrote:
> Hi Bryan,
> 
> 2018-03-17 8:06 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
>>
>>
>> On 15/03/18 16:54, Breno Matheus Lima wrote:
>>>
>>> Hi Bryan,
>>>
>>> 2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
>>>>
>>>> This patch adds IVT_PAD_SIZE at 0xC00. The IVT header is padded to this
>>>> size. Defining the size explicitly makes it possible to use the define to
>>>> locate the start/end of an IVT header without using magic numbers in
>>>> code.
>>>>
>>>
>>> As far as I know the 0xC00 pad size is only mandatory in U-Boot/SPL
>>> images, for instance in some U-Boot images the first 0xC00 should
>>> include IVT + Boot data + DCD table + padding to 0xC00.
>>> Are you using the IVT_PAD_SIZE in your current code? Or this macro
>>> will be used in a next series?
>>>
>>> Thanks,
>>> Breno Lima
>>>
>>
>> All of my images - kernel, u-boot, optee, boot.scr are signed in the same
>> first-stage format with this padding present - for simplicity.
>>
>> Maybe this better named "BOOTROM_IVT_PAD_SIZE" since only the bootrom
>> requires it.
> 
> As far as I know the IVT has a fixed size of 0x20, and the set of IVT
> + Boot Data + DCD cannot exceed 0xC00.
> 
> This value is calculated by the following operation in function
> imximage_generate() at tools/imximage.c:
> alloc_len = imximage_init_loadsize - imximage_ivt_offset;
> 
> Maybe we can rename to something like UBOOT_IMX_HDR_SIZE? We also have
> to ensure this definition is being used by U-Boot code.
> 
> Thanks,
> Breno Lima
> 

So.

I've stupidly called this "PAD_SIZE" and we've gone off on a padding 
size discussion.

This define is supposed to represent the IVT _offset_ in that initial 
BootROM image.

Note to self: the hint is in the variable "setenv ivt_offset=some_number"

Anyway Breno - we could add padding size checks to images but, _this_ 
define is related to IVT offset so... my bad.

I'll redo this patch with a better name :(

[U-Boot] [PATCH 4/4] imx: hab: Provide hab_auth_img_or_fail command

[Bryan O'Donoghue]

This patch adds hab_auth_img_or_fail() a command line function that
encapsulates a common usage of authenticate and failover, namely if
authenticate image fails, then drop to BootROM USB recovery mode.

For secure-boot systems, this type of locked down behavior is important to
ensure no unsigned images can be run.

It's possible to script this logic but, when done over and over again the
environment starts get very complex and repetitive, reducing that script
repetition down to a command line function makes sense.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
Cc: Breno Lima <breno.lima@nxp.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/mach-imx/hab.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
index 0c18b2e..61ccdeb 100644
--- a/arch/arm/mach-imx/hab.c
+++ b/arch/arm/mach-imx/hab.c
@@ -366,6 +366,22 @@ static int do_hab_get_ivt_addr(cmd_tbl_t *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+static int do_authenticate_image_or_failover(cmd_tbl_t *cmdtp, int flag,
+					     int argc, char * const argv[])
+{
+	if (!imx_hab_is_enabled())
+		goto done;
+
+	if (do_authenticate_image(NULL, flag, argc, argv) != CMD_RET_SUCCESS) {
+		fprintf(stderr, "authentication fail -> %s %s %s %s\n",
+			argv[0], argv[1], argv[2], argv[3]);
+		do_hab_failsafe(0, 0, 1, NULL);
+	};
+
+done:
+	return CMD_RET_SUCCESS;
+}
+
 U_BOOT_CMD(
 		hab_status, CONFIG_SYS_MAXARGS, 1, do_hab_status,
 		"display HAB status",
@@ -395,6 +411,16 @@ U_BOOT_CMD(
 		"ivt_offset - hex offset of IVT in the image"
 	  );
 
+U_BOOT_CMD(
+		hab_auth_img_or_fail, 4, 0,
+		do_authenticate_image_or_failover,
+		"authenticate image via HAB on failure drop to USB BootROM mode",
+		"addr length ivt_offset\n"
+		"addr - image hex address\n"
+		"length - image hex length\n"
+		"ivt_offset - hex offset of IVT in the image"
+	  );
+
 #endif /* !defined(CONFIG_SPL_BUILD) */
 
 /* Get CSF Header length */
-- 
2.7.4

[U-Boot] [PATCH 4/4] imx: hab: Provide hab_auth_img_or_fail command

[Breno Matheus Lima]

Hi Bryan,

2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
> This patch adds hab_auth_img_or_fail() a command line function that
> encapsulates a common usage of authenticate and failover, namely if
> authenticate image fails, then drop to BootROM USB recovery mode.
>
> For secure-boot systems, this type of locked down behavior is important to
> ensure no unsigned images can be run.
>
> It's possible to script this logic but, when done over and over again the
> environment starts get very complex and repetitive, reducing that script
> repetition down to a command line function makes sense.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> Cc: Breno Lima <breno.lima@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>  arch/arm/mach-imx/hab.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
> index 0c18b2e..61ccdeb 100644
> --- a/arch/arm/mach-imx/hab.c
> +++ b/arch/arm/mach-imx/hab.c
> @@ -366,6 +366,22 @@ static int do_hab_get_ivt_addr(cmd_tbl_t *cmdtp, int flag, int argc,
>         return CMD_RET_SUCCESS;
>  }
>
> +static int do_authenticate_image_or_failover(cmd_tbl_t *cmdtp, int flag,
> +                                            int argc, char * const argv[])
> +{
> +       if (!imx_hab_is_enabled())
> +               goto done;

It would be nice to return CMD_RET_USAGE on this case, or maybe print
something like "Secure boot disabled". If I run in a non HAB enabled
board I get the following output:

=> hab_auth_img_or_fail <addr> <length> <ivt_offset>
=>

We may also need to add the following here:

if (argc < 4)
       return CMD_RET_USAGE;

If I run this command without any parameter the code is wrongly
executed, and the system goes to USB recovery mode.

Thanks,
Breno Lima

[U-Boot] [PATCH 4/4] imx: hab: Provide hab_auth_img_or_fail command

[Bryan O'Donoghue]

On 15/03/18 17:15, Breno Matheus Lima wrote:
> If I run this command without any parameter the code is wrongly
> executed, and the system goes to USB recovery mode.

Oops.

I'll fix that so.

---
bod