imx: hab: Add helper functions for scripted HAB auth

PATCH Superseded Bryan O'Donoghue 3 participants 15 messages Mar 9, 2018 – Mar 21, 2018

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape

lore

Share & tools

lore

Bryan O'Donoghue

author

2018-03-09 17:35 UTC #0 lore raw

cover imx: hab: Add helper functions for scripted HAB auth

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(-)

Signature · 1 line

2.7.4

Bryan O'Donoghue

author

2018-03-09 17:35 UTC #1 lore raw

PATCH 1/4 imx: hab: Add routine to set HAB IVT address

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 <[email protected]>

Cc Utkarsh Gupta <[email protected]>

Cc Breno Lima <[email protected]>

Cc Fabio Estevam <[email protected]>

---
 arch/arm/mach-imx/hab.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

arch/arm/mach-imx/hab.c +33 -0

index c730c8f..0c18b2e 100644
@@ -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 */

Signature · 1 line

2.7.4

Breno Matheus Lima

2018-03-15 16:37 UTC #2 lore raw

PATCH 1/4 imx: hab: Add routine to set HAB IVT address

Hi Bryan,

2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <[email protected]>:

Quoted 6 lines

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.

Quoted 7 lines

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 <[email protected]>
Cc Utkarsh Gupta <[email protected]>
Cc Breno Lima <[email protected]>
Cc Fabio Estevam <[email protected]>

Tested-by <[email protected]>

Thanks,
Breno Lima

Lothar Waßmann

2018-03-16 08:17 UTC #3 lore raw

PATCH 1/4 imx: hab: Add routine to set HAB IVT address

In reply to Bryan O'Donoghue

Hi,

Quoted 44 lines · 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 <[email protected]>
Cc Utkarsh Gupta <[email protected]>
Cc Breno Lima <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 arch/arm/mach-imx/hab.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
arch/arm/mach-imx/hab.c +18 -0
index c730c8f..0c18b2e 100644
@@ -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);

arch/arm/mach-imx/hab.c static int do_hab_failsafe(cmd_tbl_t *cmdtp, int flag, int argc, · line 371

Quoted 10 lines

arch/arm/mach-imx/hab.c +7 -0

+	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

Bryan O'Donoghue

author

2018-03-17 10:55 UTC #4 lore raw

PATCH 1/4 imx: hab: Add routine to set HAB IVT address

Quoted 1 line · Lothar WaÃŸmann

'setexpr hab_ivt_addr ${loadaddr} + 0x400' could not do as well?

That might work too. I'll check.

Bryan O'Donoghue

author

2018-03-09 17:35 UTC #5 lore raw

PATCH 2/4 imx: hab: Encase majority of header in __ASSEMBLY__ declaration

In reply to 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 <[email protected]>

Cc Utkarsh Gupta <[email protected]>

Cc Breno Lima <[email protected]>

Cc Fabio Estevam <[email protected]>

---
 arch/arm/include/asm/mach-imx/hab.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

arch/arm/include/asm/mach-imx/hab.h +5 -3

index ce9a44d..1bebdbe 100644
@@ -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

Signature · 1 line

2.7.4

Breno Matheus Lima

2018-03-15 16:38 UTC #6 lore raw

PATCH 2/4 imx: hab: Encase majority of header in __ASSEMBLY__ declaration

Hi Bryan,

2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <[email protected]>:

Quoted 9 lines

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 <[email protected]>
Cc Utkarsh Gupta <[email protected]>
Cc Breno Lima <[email protected]>
Cc Fabio Estevam <[email protected]>

Tested-by <[email protected]>

Thanks,
Breno Lima

Bryan O'Donoghue

author

2018-03-09 17:35 UTC #7 lore raw

PATCH 3/4 imx: hab: Specify IVT padding size

In reply to 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 <[email protected]>

Cc Utkarsh Gupta <[email protected]>

Cc Breno Lima <[email protected]>

Cc Fabio Estevam <[email protected]>

---
 arch/arm/include/asm/mach-imx/hab.h | 1 +
 1 file changed, 1 insertion(+)

arch/arm/include/asm/mach-imx/hab.h +1 -0

index 1bebdbe..f903e3e 100644
@@ -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

Signature · 1 line

2.7.4

Breno Matheus Lima

2018-03-15 16:54 UTC #8 lore raw

PATCH 3/4 imx: hab: Specify IVT padding size

Hi Bryan,

2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <[email protected]>:

Quoted 4 lines

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

Bryan O'Donoghue

author

2018-03-17 11:06 UTC #9 lore raw

PATCH 3/4 imx: hab: Specify IVT padding size

Quoted 8 lines · Breno Matheus Lima

Hi Bryan,
2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <[email protected]>:
Quoted 4 lines
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.

Quoted 9 lines

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.

Breno Matheus Lima

2018-03-19 17:53 UTC #10 lore raw

PATCH 3/4 imx: hab: Specify IVT padding size

Hi Bryan,

2018-03-17 8:06 GMT-03:00 Bryan O'Donoghue <[email protected]>:

Quoted 24 lines · Breno Matheus Lima

Quoted 11 lines
Hi Bryan,
2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <[email protected]>:
Quoted 6 lines
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.
Quoted 9 lines
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

Quoted 5 lines

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

Bryan O'Donoghue

author

2018-03-21 04:47 UTC #11 lore raw

PATCH 3/4 imx: hab: Specify IVT padding size

Quoted 40 lines · Breno Matheus Lima

Hi Bryan,
2018-03-17 8:06 GMT-03:00 Bryan O'Donoghue <[email protected]>:
Quoted 24 lines · Breno Matheus Lima
Quoted 11 lines
Hi Bryan,
2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <[email protected]>:
Quoted 6 lines
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.
Quoted 9 lines
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
Quoted 5 lines
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;

Quoted 6 lines

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 :(

Bryan O'Donoghue

author

2018-03-09 17:35 UTC #12 lore raw

PATCH 4/4 imx: hab: Provide hab_auth_img_or_fail command

In reply to 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 <[email protected]>

Cc Utkarsh Gupta <[email protected]>

Cc Breno Lima <[email protected]>

Cc Fabio Estevam <[email protected]>

---
 arch/arm/mach-imx/hab.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

arch/arm/mach-imx/hab.c +26 -0

index 0c18b2e..61ccdeb 100644
@@ -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 */

Signature · 1 line

2.7.4

Breno Matheus Lima

2018-03-15 17:15 UTC #13 lore raw

PATCH 4/4 imx: hab: Provide hab_auth_img_or_fail command

Hi Bryan,

2018-03-09 14:35 GMT-03:00 Bryan O'Donoghue <[email protected]>:

Quoted 24 lines

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 <[email protected]>
Cc Utkarsh Gupta <[email protected]>
Cc Breno Lima <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 arch/arm/mach-imx/hab.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
arch/arm/mach-imx/hab.c +0 -0
index 0c18b2e..61ccdeb 100644

arch/arm/mach-imx/hab.c static int do_hab_get_ivt_addr(cmd_tbl_t *cmdtp, int flag, int argc, · line 373

Quoted 8 lines

arch/arm/mach-imx/hab.c +5 -0

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

Bryan O'Donoghue

author

2018-03-17 11:06 UTC #14 lore raw

PATCH 4/4 imx: hab: Provide hab_auth_img_or_fail command

Quoted 2 lines · Breno Matheus Lima

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