[U-Boot] [PATCH 0/4] Improve HAB support for SPL targets

[U-Boot] [PATCH 0/4] Improve HAB support for SPL targets

[Breno Matheus Lima]

This patch set is improving the HAB support for targets that are using
IH_TYPE_FIRMWARE_IVT image type.

Patch 0001 is migrating CONFIG_CSF_SIZE to Kconfig so value can be
configurable and used by U-Boot tools.

Patch 0002 is fixing secure boot support for targets using
IH_TYPE_FIRMWARE_IVT image type.

Patch 0003 is verifying the SPL size to avoid a possible HAB failure event.

Patch 0004 is cleaning up the code as CONFIG_SECURE_BOOT is now enabled
through Kconfig.

Breno Lima (4):
  Kconfig: Migrate CONFIG_CSF_SIZE to Kconfig
  habv4: tools: Avoid hardcoded CSF size for SPL targets
  imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled
  imx: configs: Cleanup CONFIG_SECURE_BOOT comments

 arch/arm/mach-imx/Kconfig     | 7 +++++++
 common/image.c                | 8 +++++---
 configs/imx8mq_evk_defconfig  | 1 +
 include/configs/cl-som-imx7.h | 4 ----
 include/configs/imx8mq_evk.h  | 4 ----
 include/configs/mx6_common.h  | 4 ----
 include/configs/mx6sllevk.h   | 6 ------
 include/configs/mx6ullevk.h   | 6 ------
 include/configs/mx7_common.h  | 4 ----
 include/configs/mx7ulp_evk.h  | 9 ---------
 scripts/config_whitelist.txt  | 1 -
 tools/default_image.c         | 5 ++++-
 tools/spl_size_limit.c        | 3 +++
 13 files changed, 20 insertions(+), 42 deletions(-)

-- 
2.17.1

[U-Boot] [PATCH 1/4] Kconfig: Migrate CONFIG_CSF_SIZE to Kconfig

[Breno Matheus Lima]

Move CONFIG_CSF_SIZE to Kconfig and define default value as 0x4000.

mx8mqevk requires 0x2000 add this configuration in imx8mq_evk_defconfig
file.

Signed-off-by: Breno Lima <breno.lima@nxp.com>
---
 arch/arm/mach-imx/Kconfig     | 7 +++++++
 configs/imx8mq_evk_defconfig  | 1 +
 include/configs/cl-som-imx7.h | 1 -
 include/configs/imx8mq_evk.h  | 4 ----
 include/configs/mx6_common.h  | 4 ----
 include/configs/mx6sllevk.h   | 6 ------
 include/configs/mx6ullevk.h   | 6 ------
 include/configs/mx7_common.h  | 4 ----
 include/configs/mx7ulp_evk.h  | 6 ------
 scripts/config_whitelist.txt  | 1 -
 10 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index b6fd1595f0..3df96d570b 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -43,6 +43,13 @@ config SECURE_BOOT
 	  This option enables the support for secure boot (HAB).
 	  See doc/README.mxc_hab for more details.
 
+config CSF_SIZE
+	hex "Maximum size for Command Sequence File (CSF) binary"
+	default 0x4000
+	help
+	  Define the maximum size for Command Sequence File (CSF) binary
+	  this information is used to define the image boot data.
+
 config CMD_BMODE
 	bool "Support the 'bmode' command"
 	default y
diff --git a/configs/imx8mq_evk_defconfig b/configs/imx8mq_evk_defconfig
index 8417c3ba54..534f4b39f0 100644
--- a/configs/imx8mq_evk_defconfig
+++ b/configs/imx8mq_evk_defconfig
@@ -6,6 +6,7 @@ CONFIG_SYS_TEXT_BASE=0x40200000
 CONFIG_TARGET_IMX8MQ_EVK=y
 CONFIG_SPL_SERIAL_SUPPORT=y
 CONFIG_SPL=y
+CONFIG_CSF_SIZE=0x2000
 CONFIG_FIT=y
 CONFIG_FIT_EXTERNAL_OFFSET=0x3000
 CONFIG_SPL_LOAD_FIT=y
diff --git a/include/configs/cl-som-imx7.h b/include/configs/cl-som-imx7.h
index 4c93fc6cbe..73fbceec06 100644
--- a/include/configs/cl-som-imx7.h
+++ b/include/configs/cl-som-imx7.h
@@ -19,7 +19,6 @@
 
 /* Uncomment to enable secure boot support */
 /* #define CONFIG_SECURE_BOOT */
-#define CONFIG_CSF_SIZE			0x4000
 
 /* Network */
 #define CONFIG_FEC_MXC
diff --git a/include/configs/imx8mq_evk.h b/include/configs/imx8mq_evk.h
index 16e4136fa9..8ecdf9eb67 100644
--- a/include/configs/imx8mq_evk.h
+++ b/include/configs/imx8mq_evk.h
@@ -9,10 +9,6 @@
 #include <linux/sizes.h>
 #include <asm/arch/imx-regs.h>
 
-#ifdef CONFIG_SECURE_BOOT
-#define CONFIG_CSF_SIZE			0x2000 /* 8K region */
-#endif
-
 #define CONFIG_SPL_MAX_SIZE		(124 * 1024)
 #define CONFIG_SYS_MONITOR_LEN		(512 * 1024)
 #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR
diff --git a/include/configs/mx6_common.h b/include/configs/mx6_common.h
index 2b8ce9d71d..25e27da54b 100644
--- a/include/configs/mx6_common.h
+++ b/include/configs/mx6_common.h
@@ -57,12 +57,8 @@
 /* MMC */
 #define CONFIG_FSL_USDHC
 
-/* Secure boot (HAB) support */
-#ifdef CONFIG_SECURE_BOOT
-#define CONFIG_CSF_SIZE			0x4000
 #ifdef CONFIG_SPL_BUILD
 #define CONFIG_SPL_DRIVERS_MISC_SUPPORT
 #endif
-#endif
 
 #endif
diff --git a/include/configs/mx6sllevk.h b/include/configs/mx6sllevk.h
index fb8f44684b..b96e63198d 100644
--- a/include/configs/mx6sllevk.h
+++ b/include/configs/mx6sllevk.h
@@ -10,12 +10,6 @@
 
 #include "mx6_common.h"
 
-#ifdef CONFIG_SECURE_BOOT
-#ifndef CONFIG_CSF_SIZE
-#define CONFIG_CSF_SIZE 0x4000
-#endif
-#endif
-
 /* Size of malloc() pool */
 #define CONFIG_SYS_MALLOC_LEN		(16 * SZ_1M)
 
diff --git a/include/configs/mx6ullevk.h b/include/configs/mx6ullevk.h
index 1fc5c24dec..7416ccf3ae 100644
--- a/include/configs/mx6ullevk.h
+++ b/include/configs/mx6ullevk.h
@@ -13,12 +13,6 @@
 #include "mx6_common.h"
 #include <asm/mach-imx/gpio.h>
 
-#ifdef CONFIG_SECURE_BOOT
-#ifndef CONFIG_CSF_SIZE
-#define CONFIG_CSF_SIZE 0x4000
-#endif
-#endif
-
 #define PHYS_SDRAM_SIZE	SZ_512M
 
 /* Size of malloc() pool */
diff --git a/include/configs/mx7_common.h b/include/configs/mx7_common.h
index 4f822ef9a0..01a625b135 100644
--- a/include/configs/mx7_common.h
+++ b/include/configs/mx7_common.h
@@ -46,13 +46,9 @@
 
 #define CONFIG_ARMV7_PSCI_1_0
 
-/* Secure boot (HAB) support */
-#ifdef CONFIG_SECURE_BOOT
-#define CONFIG_CSF_SIZE			0x4000
 #ifdef CONFIG_SPL_BUILD
 #define CONFIG_SPL_DRIVERS_MISC_SUPPORT
 #endif
-#endif
 
 /*
  * If we have defined the OPTEE ram size and not OPTEE it means that we were
diff --git a/include/configs/mx7ulp_evk.h b/include/configs/mx7ulp_evk.h
index 2af5a4fe3e..c53270d3ad 100644
--- a/include/configs/mx7ulp_evk.h
+++ b/include/configs/mx7ulp_evk.h
@@ -14,12 +14,6 @@
 /*Uncomment it to use secure boot*/
 /*#define CONFIG_SECURE_BOOT*/
 
-#ifdef CONFIG_SECURE_BOOT
-#ifndef CONFIG_CSF_SIZE
-#define CONFIG_CSF_SIZE			0x4000
-#endif
-#endif
-
 #define CONFIG_BOARD_POSTCLK_INIT
 #define CONFIG_SYS_BOOTM_LEN		0x1000000
 
diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
index 2c9cfb450d..9684f1784a 100644
--- a/scripts/config_whitelist.txt
+++ b/scripts/config_whitelist.txt
@@ -276,7 +276,6 @@ CONFIG_CPU_VR41XX
 CONFIG_CQSPI_REF_CLK
 CONFIG_CS8900_BUS16
 CONFIG_CS8900_BUS32
-CONFIG_CSF_SIZE
 CONFIG_CTL_JTAG
 CONFIG_CTL_TBE
 CONFIG_CUSTOMER_BOARD_SUPPORT
-- 
2.17.1

[U-Boot] [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

[Breno Matheus Lima]

Currently it's not possible to authenticate the U-Boot proper of
mx6ul_14x14_evk_defconfig target:

Authenticate image from DDR location 0x877fffc0...
bad magic magic=0x0 length=0x00 version=0x3
bad length magic=0x0 length=0x00 version=0x3
bad version magic=0x0 length=0x00 version=0x3
spl: ERROR:  image authentication fail

Commit 0633e134784a ("imx: hab: Increase CSF_SIZE for i.MX6 and
i.MX7 devices") has increased CSF_SIZE to avoid a possible issue
when booting encrypted boot images.

Commit d21bd69b6e95 ("tools: mkimage: add firmware-ivt image type
for HAB verification") is hardcoding the CSF and IVT sizes, the
new CSF size is not being considered and u-boot-ivt.img fails to
boot.

Avoid hardcoded CSF and IVT size to fix this issue.

Signed-off-by: Breno Lima <breno.lima@nxp.com>
---
 common/image.c        | 8 +++++---
 tools/default_image.c | 5 ++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/common/image.c b/common/image.c
index 9f9538fac2..fc19dfdd9c 100644
--- a/common/image.c
+++ b/common/image.c
@@ -54,6 +54,8 @@ static const image_header_t *image_get_ramdisk(ulong rd_addr, uint8_t arch,
 #endif /* !USE_HOSTCC*/
 
 #include <u-boot/crc.h>
+#include <imximage.h>
+#include <generated/autoconf.h>
 
 #ifndef CONFIG_SYS_BARGSIZE
 #define CONFIG_SYS_BARGSIZE 512
@@ -369,9 +371,9 @@ void image_print_contents(const void *ptr)
 		}
 	} else if (image_check_type(hdr, IH_TYPE_FIRMWARE_IVT)) {
 		printf("HAB Blocks:   0x%08x   0x0000   0x%08x\n",
-				image_get_load(hdr) - image_get_header_size(),
-				image_get_size(hdr) + image_get_header_size()
-						- 0x1FE0);
+			image_get_load(hdr) - image_get_header_size(),
+			(int)(image_get_size(hdr) + image_get_header_size()
+			+ sizeof(flash_header_v2_t) - CONFIG_CSF_SIZE));
 	}
 }
 
diff --git a/tools/default_image.c b/tools/default_image.c
index 4b7d1ed4a1..7a26232779 100644
--- a/tools/default_image.c
+++ b/tools/default_image.c
@@ -19,6 +19,8 @@
 #include <image.h>
 #include <tee/optee.h>
 #include <u-boot/crc.h>
+#include <imximage.h>
+#include <generated/autoconf.h>
 
 static image_header_t header;
 
@@ -106,7 +108,8 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
 
 	if (params->type == IH_TYPE_FIRMWARE_IVT)
 		/* Add size of CSF minus IVT */
-		imagesize = sbuf->st_size - sizeof(image_header_t) + 0x1FE0;
+		imagesize = sbuf->st_size - sizeof(image_header_t)
+			    + CONFIG_CSF_SIZE - sizeof(flash_header_v2_t);
 	else
 		imagesize = sbuf->st_size - sizeof(image_header_t);
 
-- 
2.17.1

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Breno Matheus Lima]

In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
avoid a possible HAB failure event:

--------- HAB Event 1 -----------------
event data:
        0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
        0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
        0x00 0x01 0x10 0x00
STS = HAB_FAILURE (0x33)
RSN = HAB_INV_ADDRESS (0x22)
CTX = HAB_CTX_TARGET (0x33)
ENG = HAB_ENG_ANY (0x00)

As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
default") the i.MX6 SPL size limit is 68KB.

The ROM code is copying the image size defined in boot data to its
respective load address, in case we exceed the OCRAM free region a
HAB invalid address failure event is generated.

The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
limit based on this configuration.

Signed-off-by: Breno Lima <breno.lima@nxp.com>
---
 tools/spl_size_limit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/spl_size_limit.c b/tools/spl_size_limit.c
index 98ff491867..8902e30129 100644
--- a/tools/spl_size_limit.c
+++ b/tools/spl_size_limit.c
@@ -14,6 +14,9 @@ int main(int argc, char *argv[])
 
 #ifdef CONFIG_SPL_SIZE_LIMIT
 	spl_size_limit = CONFIG_SPL_SIZE_LIMIT;
+#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_CSF_SIZE)
+	spl_size_limit -= CONFIG_CSF_SIZE;
+#endif
 #ifdef CONFIG_SPL_SIZE_LIMIT_SUBTRACT_GD
 	spl_size_limit -= GENERATED_GBL_DATA_SIZE;
 #endif
-- 
2.17.1

[U-Boot] [PATCH 4/4] imx: configs: Cleanup CONFIG_SECURE_BOOT comments

[Breno Matheus Lima]

Since commit 6e1f4d2652e7 ("arm: imx-common: add SECURE_BOOT option
to Kconfig") the SECURE_BOOT option is selected through Kconfig.

Cleanup comments in code to align with this change.

Signed-off-by: Breno Lima <breno.lima@nxp.com>
---
 include/configs/cl-som-imx7.h | 3 ---
 include/configs/mx7ulp_evk.h  | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/include/configs/cl-som-imx7.h b/include/configs/cl-som-imx7.h
index 73fbceec06..3ca16c5201 100644
--- a/include/configs/cl-som-imx7.h
+++ b/include/configs/cl-som-imx7.h
@@ -17,9 +17,6 @@
 
 #define CONFIG_BOARD_LATE_INIT
 
-/* Uncomment to enable secure boot support */
-/* #define CONFIG_SECURE_BOOT */
-
 /* Network */
 #define CONFIG_FEC_MXC
 #define CONFIG_FEC_XCV_TYPE             RGMII
diff --git a/include/configs/mx7ulp_evk.h b/include/configs/mx7ulp_evk.h
index c53270d3ad..437b2758e8 100644
--- a/include/configs/mx7ulp_evk.h
+++ b/include/configs/mx7ulp_evk.h
@@ -11,9 +11,6 @@
 #include <linux/sizes.h>
 #include <asm/arch/imx-regs.h>
 
-/*Uncomment it to use secure boot*/
-/*#define CONFIG_SECURE_BOOT*/
-
 #define CONFIG_BOARD_POSTCLK_INIT
 #define CONFIG_SYS_BOOTM_LEN		0x1000000
 
-- 
2.17.1

[U-Boot] [PATCH 1/4] Kconfig: Migrate CONFIG_CSF_SIZE to Kconfig

[Fabio Estevam]

On Thu, Jul 18, 2019 at 9:34 AM Breno Matheus Lima <breno.lima@nxp.com> wrote:
>
> Move CONFIG_CSF_SIZE to Kconfig and define default value as 0x4000.
>
> mx8mqevk requires 0x2000 add this configuration in imx8mq_evk_defconfig
> file.
>
> Signed-off-by: Breno Lima <breno.lima@nxp.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

[U-Boot] [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

[Fabio Estevam]

On Thu, Jul 18, 2019 at 9:34 AM Breno Matheus Lima <breno.lima@nxp.com> wrote:
>
> Currently it's not possible to authenticate the U-Boot proper of
> mx6ul_14x14_evk_defconfig target:
>
> Authenticate image from DDR location 0x877fffc0...
> bad magic magic=0x0 length=0x00 version=0x3
> bad length magic=0x0 length=0x00 version=0x3
> bad version magic=0x0 length=0x00 version=0x3
> spl: ERROR:  image authentication fail
>
> Commit 0633e134784a ("imx: hab: Increase CSF_SIZE for i.MX6 and
> i.MX7 devices") has increased CSF_SIZE to avoid a possible issue
> when booting encrypted boot images.
>
> Commit d21bd69b6e95 ("tools: mkimage: add firmware-ivt image type
> for HAB verification") is hardcoding the CSF and IVT sizes, the
> new CSF size is not being considered and u-boot-ivt.img fails to
> boot.
>
> Avoid hardcoded CSF and IVT size to fix this issue.
>
> Signed-off-by: Breno Lima <breno.lima@nxp.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Fabio Estevam]

On Thu, Jul 18, 2019 at 9:34 AM Breno Matheus Lima <breno.lima@nxp.com> wrote:
>
> In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
> avoid a possible HAB failure event:
>
> --------- HAB Event 1 -----------------
> event data:
>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
>         0x00 0x01 0x10 0x00
> STS = HAB_FAILURE (0x33)
> RSN = HAB_INV_ADDRESS (0x22)
> CTX = HAB_CTX_TARGET (0x33)
> ENG = HAB_ENG_ANY (0x00)
>
> As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
> default") the i.MX6 SPL size limit is 68KB.
>
> The ROM code is copying the image size defined in boot data to its
> respective load address, in case we exceed the OCRAM free region a
> HAB invalid address failure event is generated.
>
> The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
> limit based on this configuration.
>
> Signed-off-by: Breno Lima <breno.lima@nxp.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

[U-Boot] [PATCH 4/4] imx: configs: Cleanup CONFIG_SECURE_BOOT comments

[Fabio Estevam]

On Thu, Jul 18, 2019 at 9:34 AM Breno Matheus Lima <breno.lima@nxp.com> wrote:
>
> Since commit 6e1f4d2652e7 ("arm: imx-common: add SECURE_BOOT option
> to Kconfig") the SECURE_BOOT option is selected through Kconfig.
>
> Cleanup comments in code to align with this change.
>
> Signed-off-by: Breno Lima <breno.lima@nxp.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

[U-Boot] [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

[Peng Fan]

Hi Breno,

> Subject: [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

I saw this patch in imx/master, not in Tom's tree. But this patch breaks
build for other archs, such as arc and etc.

Regards,
Peng.

> 
> Currently it's not possible to authenticate the U-Boot proper of
> mx6ul_14x14_evk_defconfig target:
> 
> Authenticate image from DDR location 0x877fffc0...
> bad magic magic=0x0 length=0x00 version=0x3 bad length magic=0x0
> length=0x00 version=0x3 bad version magic=0x0 length=0x00 version=0x3
> spl: ERROR:  image authentication fail
> 
> Commit 0633e134784a ("imx: hab: Increase CSF_SIZE for i.MX6 and
> i.MX7 devices") has increased CSF_SIZE to avoid a possible issue when
> booting encrypted boot images.
> 
> Commit d21bd69b6e95 ("tools: mkimage: add firmware-ivt image type for
> HAB verification") is hardcoding the CSF and IVT sizes, the new CSF size is not
> being considered and u-boot-ivt.img fails to boot.
> 
> Avoid hardcoded CSF and IVT size to fix this issue.
> 
> Signed-off-by: Breno Lima <breno.lima@nxp.com>
> ---
>  common/image.c        | 8 +++++---
>  tools/default_image.c | 5 ++++-
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/common/image.c b/common/image.c index
> 9f9538fac2..fc19dfdd9c 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -54,6 +54,8 @@ static const image_header_t *image_get_ramdisk(ulong
> rd_addr, uint8_t arch,  #endif /* !USE_HOSTCC*/
> 
>  #include <u-boot/crc.h>
> +#include <imximage.h>
> +#include <generated/autoconf.h>
> 
>  #ifndef CONFIG_SYS_BARGSIZE
>  #define CONFIG_SYS_BARGSIZE 512
> @@ -369,9 +371,9 @@ void image_print_contents(const void *ptr)
>  		}
>  	} else if (image_check_type(hdr, IH_TYPE_FIRMWARE_IVT)) {
>  		printf("HAB Blocks:   0x%08x   0x0000   0x%08x\n",
> -				image_get_load(hdr) - image_get_header_size(),
> -				image_get_size(hdr) + image_get_header_size()
> -						- 0x1FE0);
> +			image_get_load(hdr) - image_get_header_size(),
> +			(int)(image_get_size(hdr) + image_get_header_size()
> +			+ sizeof(flash_header_v2_t) - CONFIG_CSF_SIZE));
>  	}
>  }
> 
> diff --git a/tools/default_image.c b/tools/default_image.c index
> 4b7d1ed4a1..7a26232779 100644
> --- a/tools/default_image.c
> +++ b/tools/default_image.c
> @@ -19,6 +19,8 @@
>  #include <image.h>
>  #include <tee/optee.h>
>  #include <u-boot/crc.h>
> +#include <imximage.h>
> +#include <generated/autoconf.h>
> 
>  static image_header_t header;
> 
> @@ -106,7 +108,8 @@ static void image_set_header(void *ptr, struct stat
> *sbuf, int ifd,
> 
>  	if (params->type == IH_TYPE_FIRMWARE_IVT)
>  		/* Add size of CSF minus IVT */
> -		imagesize = sbuf->st_size - sizeof(image_header_t) + 0x1FE0;
> +		imagesize = sbuf->st_size - sizeof(image_header_t)
> +			    + CONFIG_CSF_SIZE - sizeof(flash_header_v2_t);
>  	else
>  		imagesize = sbuf->st_size - sizeof(image_header_t);
> 
> --
> 2.17.1

[U-Boot] [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

[Breno Matheus Lima]

Hi Peng,

Em qua, 11 de set de 2019 às 22:07, Peng Fan <peng.fan@nxp.com> escreveu:
>
> Hi Breno,
>
> > Subject: [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets
>
> I saw this patch in imx/master, not in Tom's tree. But this patch breaks
> build for other archs, such as arc and etc.
>

Thanks for reporting the issue, I will submit a fix.

Best regards,
Breno Lima

[U-Boot] [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

[Stefano Babic]

Hi Breno,

On 12/09/19 03:07, Peng Fan wrote:
> Hi Breno,
> 
>> Subject: [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets
> 
> I saw this patch in imx/master, not in Tom's tree. But this patch breaks
> build for other archs, such as arc and etc.
> 

Any news on this ? I dropped it from u-boot-imx due the breakage, but I
can easy pick it up again if it will be fixed.

Regards,
Stefano

> Regards,
> Peng.
> 
>>
>> Currently it's not possible to authenticate the U-Boot proper of
>> mx6ul_14x14_evk_defconfig target:
>>
>> Authenticate image from DDR location 0x877fffc0...
>> bad magic magic=0x0 length=0x00 version=0x3 bad length magic=0x0
>> length=0x00 version=0x3 bad version magic=0x0 length=0x00 version=0x3
>> spl: ERROR:  image authentication fail
>>
>> Commit 0633e134784a ("imx: hab: Increase CSF_SIZE for i.MX6 and
>> i.MX7 devices") has increased CSF_SIZE to avoid a possible issue when
>> booting encrypted boot images.
>>
>> Commit d21bd69b6e95 ("tools: mkimage: add firmware-ivt image type for
>> HAB verification") is hardcoding the CSF and IVT sizes, the new CSF size is not
>> being considered and u-boot-ivt.img fails to boot.
>>
>> Avoid hardcoded CSF and IVT size to fix this issue.
>>
>> Signed-off-by: Breno Lima <breno.lima@nxp.com>
>> ---
>>  common/image.c        | 8 +++++---
>>  tools/default_image.c | 5 ++++-
>>  2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/common/image.c b/common/image.c index
>> 9f9538fac2..fc19dfdd9c 100644
>> --- a/common/image.c
>> +++ b/common/image.c
>> @@ -54,6 +54,8 @@ static const image_header_t *image_get_ramdisk(ulong
>> rd_addr, uint8_t arch,  #endif /* !USE_HOSTCC*/
>>
>>  #include <u-boot/crc.h>
>> +#include <imximage.h>
>> +#include <generated/autoconf.h>
>>
>>  #ifndef CONFIG_SYS_BARGSIZE
>>  #define CONFIG_SYS_BARGSIZE 512
>> @@ -369,9 +371,9 @@ void image_print_contents(const void *ptr)
>>  		}
>>  	} else if (image_check_type(hdr, IH_TYPE_FIRMWARE_IVT)) {
>>  		printf("HAB Blocks:   0x%08x   0x0000   0x%08x\n",
>> -				image_get_load(hdr) - image_get_header_size(),
>> -				image_get_size(hdr) + image_get_header_size()
>> -						- 0x1FE0);
>> +			image_get_load(hdr) - image_get_header_size(),
>> +			(int)(image_get_size(hdr) + image_get_header_size()
>> +			+ sizeof(flash_header_v2_t) - CONFIG_CSF_SIZE));
>>  	}
>>  }
>>
>> diff --git a/tools/default_image.c b/tools/default_image.c index
>> 4b7d1ed4a1..7a26232779 100644
>> --- a/tools/default_image.c
>> +++ b/tools/default_image.c
>> @@ -19,6 +19,8 @@
>>  #include <image.h>
>>  #include <tee/optee.h>
>>  #include <u-boot/crc.h>
>> +#include <imximage.h>
>> +#include <generated/autoconf.h>
>>
>>  static image_header_t header;
>>
>> @@ -106,7 +108,8 @@ static void image_set_header(void *ptr, struct stat
>> *sbuf, int ifd,
>>
>>  	if (params->type == IH_TYPE_FIRMWARE_IVT)
>>  		/* Add size of CSF minus IVT */
>> -		imagesize = sbuf->st_size - sizeof(image_header_t) + 0x1FE0;
>> +		imagesize = sbuf->st_size - sizeof(image_header_t)
>> +			    + CONFIG_CSF_SIZE - sizeof(flash_header_v2_t);
>>  	else
>>  		imagesize = sbuf->st_size - sizeof(image_header_t);
>>
>> --
>> 2.17.1
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

[Breno Matheus Lima]

Hi Stefano,

Em seg, 16 de set de 2019 às 05:17, Stefano Babic <sbabic@denx.de> escreveu:
>
> Hi Breno,
>
> On 12/09/19 03:07, Peng Fan wrote:
> > Hi Breno,
> >
> >> Subject: [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets
> >
> > I saw this patch in imx/master, not in Tom's tree. But this patch breaks
> > build for other archs, such as arc and etc.
> >
>
> Any news on this ? I dropped it from u-boot-imx due the breakage, but I
> can easy pick it up again if it will be fixed.
>

Sorry the delay. I'm still trying to reproduce the issue, probably
something is missing in my buildman setup.

One workaround would be to enclose the code with CONFIG_SECURE_BOOT,
this code is only used by IH_TYPE_FIRMWARE_IVT which requires HAB to
be enabled. I can send a patch but I would like to confirm before.

Thanks,
Breno Lima

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Jagan Teki]

Hi Breno,

On Thu, Jul 18, 2019 at 6:06 PM Breno Matheus Lima <breno.lima@nxp.com> wrote:
>
> In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
> avoid a possible HAB failure event:
>
> --------- HAB Event 1 -----------------
> event data:
>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
>         0x00 0x01 0x10 0x00
> STS = HAB_FAILURE (0x33)
> RSN = HAB_INV_ADDRESS (0x22)
> CTX = HAB_CTX_TARGET (0x33)
> ENG = HAB_ENG_ANY (0x00)
>
> As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
> default") the i.MX6 SPL size limit is 68KB.
>
> The ROM code is copying the image size defined in boot data to its
> respective load address, in case we exceed the OCRAM free region a
> HAB invalid address failure event is generated.
>
> The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
> limit based on this configuration.
>
> Signed-off-by: Breno Lima <breno.lima@nxp.com>
> ---
>  tools/spl_size_limit.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/tools/spl_size_limit.c b/tools/spl_size_limit.c
> index 98ff491867..8902e30129 100644
> --- a/tools/spl_size_limit.c
> +++ b/tools/spl_size_limit.c
> @@ -14,6 +14,9 @@ int main(int argc, char *argv[])
>
>  #ifdef CONFIG_SPL_SIZE_LIMIT
>         spl_size_limit = CONFIG_SPL_SIZE_LIMIT;
> +#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_CSF_SIZE)
> +       spl_size_limit -= CONFIG_CSF_SIZE;
> +#endif

But, if the target enable HAB on SPL the size would be part of SPL
limit, isn't ?

Just now I have looked at this, since one of my board (imx6_mamoj)
fails to build.

Jagan.

[U-Boot] [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

[Stefano Babic]

On 17/09/19 05:26, Breno Matheus Lima wrote:
> Hi Stefano,
> 
> Em seg, 16 de set de 2019 às 05:17, Stefano Babic <sbabic@denx.de> escreveu:
>>
>> Hi Breno,
>>
>> On 12/09/19 03:07, Peng Fan wrote:
>>> Hi Breno,
>>>
>>>> Subject: [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets
>>>
>>> I saw this patch in imx/master, not in Tom's tree. But this patch breaks
>>> build for other archs, such as arc and etc.
>>>
>>
>> Any news on this ? I dropped it from u-boot-imx due the breakage, but I
>> can easy pick it up again if it will be fixed.
>>
> 
> Sorry the delay. I'm still trying to reproduce the issue, probably
> something is missing in my buildman setup.

It looks like that a header is missing, maybe this came together with
the moving to a common place for gzip & Co (patch for Simon). I think it
could be easy for you to reproduce if you set current u-boot-imx.

> 
> One workaround would be to enclose the code with CONFIG_SECURE_BOOT,
> this code is only used by IH_TYPE_FIRMWARE_IVT which requires HAB to
> be enabled. I can send a patch but I would like to confirm before.

Nevertheless, HAB code should be put in just if needed. I agree on the
patch independently from the issue.

Regards,
Stefano

> 
> Thanks,
> Breno Lima
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Stefano Babic]

Hi Jagan, Breno,

On 17/09/19 09:13, Jagan Teki wrote:
> Hi Breno,
> 
> On Thu, Jul 18, 2019 at 6:06 PM Breno Matheus Lima <breno.lima@nxp.com> wrote:
>>
>> In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
>> avoid a possible HAB failure event:
>>
>> --------- HAB Event 1 -----------------
>> event data:
>>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
>>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
>>         0x00 0x01 0x10 0x00
>> STS = HAB_FAILURE (0x33)
>> RSN = HAB_INV_ADDRESS (0x22)
>> CTX = HAB_CTX_TARGET (0x33)
>> ENG = HAB_ENG_ANY (0x00)
>>
>> As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
>> default") the i.MX6 SPL size limit is 68KB.
>>
>> The ROM code is copying the image size defined in boot data to its
>> respective load address, in case we exceed the OCRAM free region a
>> HAB invalid address failure event is generated.
>>
>> The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
>> limit based on this configuration.
>>
>> Signed-off-by: Breno Lima <breno.lima@nxp.com>
>> ---
>>  tools/spl_size_limit.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/tools/spl_size_limit.c b/tools/spl_size_limit.c
>> index 98ff491867..8902e30129 100644
>> --- a/tools/spl_size_limit.c
>> +++ b/tools/spl_size_limit.c
>> @@ -14,6 +14,9 @@ int main(int argc, char *argv[])
>>
>>  #ifdef CONFIG_SPL_SIZE_LIMIT
>>         spl_size_limit = CONFIG_SPL_SIZE_LIMIT;
>> +#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_CSF_SIZE)
>> +       spl_size_limit -= CONFIG_CSF_SIZE;
>> +#endif
> 
> But, if the target enable HAB on SPL the size would be part of SPL
> limit, isn't ?

Indeed - it is not clear to me, too, if it is correct, even if CSF is
added later by the NXP signing tools. The patch reduces significantly
the available space for SPL, I just wondering why just mamoj is
affected. Jagan, does it work without this patch applied ?

Regards,
Stefano

> 
> Just now I have looked at this, since one of my board (imx6_mamoj)
> fails to build.
> 
> Jagan.
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Breno Matheus Lima]

HI Stefano and Jagan,

Em qua, 18 de set de 2019 às 04:59, Stefano Babic <sbabic@denx.de> escreveu:
>
> Hi Jagan, Breno,
>
> On 17/09/19 09:13, Jagan Teki wrote:
> > Hi Breno,
> >
> > On Thu, Jul 18, 2019 at 6:06 PM Breno Matheus Lima <breno.lima@nxp.com> wrote:
> >>
> >> In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
> >> avoid a possible HAB failure event:
> >>
> >> --------- HAB Event 1 -----------------
> >> event data:
> >>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
> >>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
> >>         0x00 0x01 0x10 0x00
> >> STS = HAB_FAILURE (0x33)
> >> RSN = HAB_INV_ADDRESS (0x22)
> >> CTX = HAB_CTX_TARGET (0x33)
> >> ENG = HAB_ENG_ANY (0x00)
> >>
> >> As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
> >> default") the i.MX6 SPL size limit is 68KB.
> >>
> >> The ROM code is copying the image size defined in boot data to its
> >> respective load address, in case we exceed the OCRAM free region a
> >> HAB invalid address failure event is generated.
> >>
> >> The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
> >> limit based on this configuration.
> >>
> >> Signed-off-by: Breno Lima <breno.lima@nxp.com>
> >> ---
> >>  tools/spl_size_limit.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/tools/spl_size_limit.c b/tools/spl_size_limit.c
> >> index 98ff491867..8902e30129 100644
> >> --- a/tools/spl_size_limit.c
> >> +++ b/tools/spl_size_limit.c
> >> @@ -14,6 +14,9 @@ int main(int argc, char *argv[])
> >>
> >>  #ifdef CONFIG_SPL_SIZE_LIMIT
> >>         spl_size_limit = CONFIG_SPL_SIZE_LIMIT;
> >> +#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_CSF_SIZE)
> >> +       spl_size_limit -= CONFIG_CSF_SIZE;
> >> +#endif
> >
> > But, if the target enable HAB on SPL the size would be part of SPL
> > limit, isn't ?
>
> Indeed - it is not clear to me, too, if it is correct, even if CSF is
> added later by the NXP signing tools. The patch reduces significantly
> the available space for SPL, I just wondering why just mamoj is
> affected. Jagan, does it work without this patch applied ?
>

When enabling CONFIG_SECURE_BOOT we increase the image length in boot
data by the size defined in CONFIG_CSF_SIZE. The HAB code will parse
the boot data structure and copy the image length defined (SPL image
plus CSF appended) to its respective load address.

HAB code is checking if the image length defined can fit in OCRAM free
region, and logs the following HAB event in case not:

--------- HAB Event 1 -----------------
event data:
        0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
        0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
        0x00 0x01 0x10 0x00
STS = HAB_FAILURE (0x33)
RSN = HAB_INV_ADDRESS (0x22)
CTX = HAB_CTX_TARGET (0x33)
ENG = HAB_ENG_ANY (0x00)

HAB closed targets would then fail to boot, so for that reason we
added CONFIG_CSF_SIZE into consideration.

We can reduce the default CONFIG_CSF_SIZE but it depends on the user
specific HAB setup. I did a quick test with RSA 4K keys and couldn't
achieve 0x2000 length.

Do you think we should decrease default CONFIG_CSF_SIZE? Perhaps
0x2000 plus the maximum dek blob size (0x60) would be enough for most
uses cases, users requiring more space can modify their
CONFIG_CSF_SIZE.

Thanks,
Breno Lima

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Jagan Teki]

Hi Stefano,

On Wed, Sep 18, 2019 at 1:29 PM Stefano Babic <sbabic@denx.de> wrote:
>
> Hi Jagan, Breno,
>
> On 17/09/19 09:13, Jagan Teki wrote:
> > Hi Breno,
> >
> > On Thu, Jul 18, 2019 at 6:06 PM Breno Matheus Lima <breno.lima@nxp.com> wrote:
> >>
> >> In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
> >> avoid a possible HAB failure event:
> >>
> >> --------- HAB Event 1 -----------------
> >> event data:
> >>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
> >>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
> >>         0x00 0x01 0x10 0x00
> >> STS = HAB_FAILURE (0x33)
> >> RSN = HAB_INV_ADDRESS (0x22)
> >> CTX = HAB_CTX_TARGET (0x33)
> >> ENG = HAB_ENG_ANY (0x00)
> >>
> >> As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
> >> default") the i.MX6 SPL size limit is 68KB.
> >>
> >> The ROM code is copying the image size defined in boot data to its
> >> respective load address, in case we exceed the OCRAM free region a
> >> HAB invalid address failure event is generated.
> >>
> >> The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
> >> limit based on this configuration.
> >>
> >> Signed-off-by: Breno Lima <breno.lima@nxp.com>
> >> ---
> >>  tools/spl_size_limit.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/tools/spl_size_limit.c b/tools/spl_size_limit.c
> >> index 98ff491867..8902e30129 100644
> >> --- a/tools/spl_size_limit.c
> >> +++ b/tools/spl_size_limit.c
> >> @@ -14,6 +14,9 @@ int main(int argc, char *argv[])
> >>
> >>  #ifdef CONFIG_SPL_SIZE_LIMIT
> >>         spl_size_limit = CONFIG_SPL_SIZE_LIMIT;
> >> +#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_CSF_SIZE)
> >> +       spl_size_limit -= CONFIG_CSF_SIZE;
> >> +#endif
> >
> > But, if the target enable HAB on SPL the size would be part of SPL
> > limit, isn't ?
>
> Indeed - it is not clear to me, too, if it is correct, even if CSF is
> added later by the NXP signing tools. The patch reduces significantly
> the available space for SPL, I just wondering why just mamoj is
> affected. Jagan, does it work without this patch applied ?

mamoj is affected since the board enables SPL_DM, SPL_OF_CONTROL. Yes,
the build look fine without this patch.

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Stefano Babic]

Hi Breno,

On 19/09/19 03:31, Breno Matheus Lima wrote:
> HI Stefano and Jagan,
> 
> Em qua, 18 de set de 2019 às 04:59, Stefano Babic <sbabic@denx.de> escreveu:
>>
>> Hi Jagan, Breno,
>>
>> On 17/09/19 09:13, Jagan Teki wrote:
>>> Hi Breno,
>>>
>>> On Thu, Jul 18, 2019 at 6:06 PM Breno Matheus Lima <breno.lima@nxp.com> wrote:
>>>>
>>>> In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
>>>> avoid a possible HAB failure event:
>>>>
>>>> --------- HAB Event 1 -----------------
>>>> event data:
>>>>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
>>>>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
>>>>         0x00 0x01 0x10 0x00
>>>> STS = HAB_FAILURE (0x33)
>>>> RSN = HAB_INV_ADDRESS (0x22)
>>>> CTX = HAB_CTX_TARGET (0x33)
>>>> ENG = HAB_ENG_ANY (0x00)
>>>>
>>>> As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
>>>> default") the i.MX6 SPL size limit is 68KB.
>>>>
>>>> The ROM code is copying the image size defined in boot data to its
>>>> respective load address, in case we exceed the OCRAM free region a
>>>> HAB invalid address failure event is generated.
>>>>
>>>> The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
>>>> limit based on this configuration.
>>>>
>>>> Signed-off-by: Breno Lima <breno.lima@nxp.com>
>>>> ---
>>>>  tools/spl_size_limit.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/tools/spl_size_limit.c b/tools/spl_size_limit.c
>>>> index 98ff491867..8902e30129 100644
>>>> --- a/tools/spl_size_limit.c
>>>> +++ b/tools/spl_size_limit.c
>>>> @@ -14,6 +14,9 @@ int main(int argc, char *argv[])
>>>>
>>>>  #ifdef CONFIG_SPL_SIZE_LIMIT
>>>>         spl_size_limit = CONFIG_SPL_SIZE_LIMIT;
>>>> +#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_CSF_SIZE)
>>>> +       spl_size_limit -= CONFIG_CSF_SIZE;
>>>> +#endif
>>>
>>> But, if the target enable HAB on SPL the size would be part of SPL
>>> limit, isn't ?
>>
>> Indeed - it is not clear to me, too, if it is correct, even if CSF is
>> added later by the NXP signing tools. The patch reduces significantly
>> the available space for SPL, I just wondering why just mamoj is
>> affected. Jagan, does it work without this patch applied ?
>>
> 
> When enabling CONFIG_SECURE_BOOT we increase the image length in boot
> data by the size defined in CONFIG_CSF_SIZE. The HAB code will parse
> the boot data structure and copy the image length defined (SPL image
> plus CSF appended) to its respective load address.
> 
> HAB code is checking if the image length defined can fit in OCRAM free
> region, and logs the following HAB event in case not:
> 
> --------- HAB Event 1 -----------------
> event data:
>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
>         0x00 0x01 0x10 0x00
> STS = HAB_FAILURE (0x33)
> RSN = HAB_INV_ADDRESS (0x22)
> CTX = HAB_CTX_TARGET (0x33)
> ENG = HAB_ENG_ANY (0x00)
> 
> HAB closed targets would then fail to boot, so for that reason we
> added CONFIG_CSF_SIZE into consideration.
> 

Clear - thanks for detailed explanation.

> We can reduce the default CONFIG_CSF_SIZE but it depends on the user
> specific HAB setup. I did a quick test with RSA 4K keys and couldn't
> achieve 0x2000 length.

That is much less as we have now.

> 
> Do you think we should decrease default CONFIG_CSF_SIZE?

I think yes - if we set it for the worst case, we reduce the SPL size so
much that most boards, if they enable SECURE_BOOT, won't build. I cannot
say that imx6dl_mamoj has dead code in its SPL, it is one of the board
with the "state of art" in U-Boot, with DM and OF in SPL. But this is
also something we decided to push into U-Boot. Anyway, every board
maintainer can change it and add it to the own defconfig.

Jagan, after setting CONFIG_CSF_SIZE to 0x2060 as suggested by Breno,
board builds fine - but I have no idea if it can boots. Can you check
this ?

> Perhaps
> 0x2000 plus the maximum dek blob size (0x60) would be enough for most
> uses cases, users requiring more space can modify their
> CONFIG_CSF_SIZE.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Stefano Babic]

On 19/09/19 07:37, Jagan Teki wrote:
> Hi Stefano,
> 
> On Wed, Sep 18, 2019 at 1:29 PM Stefano Babic <sbabic@denx.de> wrote:
>>
>> Hi Jagan, Breno,
>>
>> On 17/09/19 09:13, Jagan Teki wrote:
>>> Hi Breno,
>>>
>>> On Thu, Jul 18, 2019 at 6:06 PM Breno Matheus Lima <breno.lima@nxp.com> wrote:
>>>>
>>>> In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
>>>> avoid a possible HAB failure event:
>>>>
>>>> --------- HAB Event 1 -----------------
>>>> event data:
>>>>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
>>>>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
>>>>         0x00 0x01 0x10 0x00
>>>> STS = HAB_FAILURE (0x33)
>>>> RSN = HAB_INV_ADDRESS (0x22)
>>>> CTX = HAB_CTX_TARGET (0x33)
>>>> ENG = HAB_ENG_ANY (0x00)
>>>>
>>>> As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
>>>> default") the i.MX6 SPL size limit is 68KB.
>>>>
>>>> The ROM code is copying the image size defined in boot data to its
>>>> respective load address, in case we exceed the OCRAM free region a
>>>> HAB invalid address failure event is generated.
>>>>
>>>> The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
>>>> limit based on this configuration.
>>>>
>>>> Signed-off-by: Breno Lima <breno.lima@nxp.com>
>>>> ---
>>>>  tools/spl_size_limit.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/tools/spl_size_limit.c b/tools/spl_size_limit.c
>>>> index 98ff491867..8902e30129 100644
>>>> --- a/tools/spl_size_limit.c
>>>> +++ b/tools/spl_size_limit.c
>>>> @@ -14,6 +14,9 @@ int main(int argc, char *argv[])
>>>>
>>>>  #ifdef CONFIG_SPL_SIZE_LIMIT
>>>>         spl_size_limit = CONFIG_SPL_SIZE_LIMIT;
>>>> +#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_CSF_SIZE)
>>>> +       spl_size_limit -= CONFIG_CSF_SIZE;
>>>> +#endif
>>>
>>> But, if the target enable HAB on SPL the size would be part of SPL
>>> limit, isn't ?
>>
>> Indeed - it is not clear to me, too, if it is correct, even if CSF is
>> added later by the NXP signing tools. The patch reduces significantly
>> the available space for SPL, I just wondering why just mamoj is
>> affected. Jagan, does it work without this patch applied ?
> 
> mamoj is affected since the board enables SPL_DM, SPL_OF_CONTROL. Yes,
> the build look fine without this patch.

Anyway, SPL size does not seem to much. But dropping 0x4000 to the
available size is really a lot, and I hope we can reduce this.

Regards,
Stefano




-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 3/4] imx6: spl: Reduce SPL limit size in case CONFIG_SECURE_BOOT is enabled

[Breno Matheus Lima]

Hi Stefano and Jagan,


Em qui, 19 de set de 2019 às 05:27, Stefano Babic <sbabic@denx.de> escreveu:
>
> On 19/09/19 07:37, Jagan Teki wrote:
> > Hi Stefano,
> >
> > On Wed, Sep 18, 2019 at 1:29 PM Stefano Babic <sbabic@denx.de> wrote:
> >>
> >> Hi Jagan, Breno,
> >>
> >> On 17/09/19 09:13, Jagan Teki wrote:
> >>> Hi Breno,
> >>>
> >>> On Thu, Jul 18, 2019 at 6:06 PM Breno Matheus Lima <breno.lima@nxp.com> wrote:
> >>>>
> >>>> In case CONFIG_SECURE_BOOT is enabled we need to limit the SPL size to
> >>>> avoid a possible HAB failure event:
> >>>>
> >>>> --------- HAB Event 1 -----------------
> >>>> event data:
> >>>>         0xdb 0x00 0x14 0x42 0x33 0x22 0x33 0x00
> >>>>         0x00 0x00 0x00 0x0f 0x00 0x90 0x70 0x00
> >>>>         0x00 0x01 0x10 0x00
> >>>> STS = HAB_FAILURE (0x33)
> >>>> RSN = HAB_INV_ADDRESS (0x22)
> >>>> CTX = HAB_CTX_TARGET (0x33)
> >>>> ENG = HAB_ENG_ANY (0x00)
> >>>>
> >>>> As explained in Commit 23612534fe0f ("spl: imx6: Provide a SPL_SIZE_LIMIT
> >>>> default") the i.MX6 SPL size limit is 68KB.
> >>>>
> >>>> The ROM code is copying the image size defined in boot data to its
> >>>> respective load address, in case we exceed the OCRAM free region a
> >>>> HAB invalid address failure event is generated.
> >>>>
> >>>> The maximum CSF size is defined in CONFIG_CSF_SIZE, reduce SPL size
> >>>> limit based on this configuration.
> >>>>
> >>>> Signed-off-by: Breno Lima <breno.lima@nxp.com>
> >>>> ---
> >>>>  tools/spl_size_limit.c | 3 +++
> >>>>  1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/tools/spl_size_limit.c b/tools/spl_size_limit.c
> >>>> index 98ff491867..8902e30129 100644
> >>>> --- a/tools/spl_size_limit.c
> >>>> +++ b/tools/spl_size_limit.c
> >>>> @@ -14,6 +14,9 @@ int main(int argc, char *argv[])
> >>>>
> >>>>  #ifdef CONFIG_SPL_SIZE_LIMIT
> >>>>         spl_size_limit = CONFIG_SPL_SIZE_LIMIT;
> >>>> +#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_CSF_SIZE)
> >>>> +       spl_size_limit -= CONFIG_CSF_SIZE;
> >>>> +#endif
> >>>
> >>> But, if the target enable HAB on SPL the size would be part of SPL
> >>> limit, isn't ?
> >>
> >> Indeed - it is not clear to me, too, if it is correct, even if CSF is
> >> added later by the NXP signing tools. The patch reduces significantly
> >> the available space for SPL, I just wondering why just mamoj is
> >> affected. Jagan, does it work without this patch applied ?
> >
> > mamoj is affected since the board enables SPL_DM, SPL_OF_CONTROL. Yes,
> > the build look fine without this patch.
>
> Anyway, SPL size does not seem to much. But dropping 0x4000 to the
> available size is really a lot, and I hope we can reduce this.
>

Thanks for submitting a fix for mamoj board.

We should also reduce CSF_SIZE in default_image.c and image.c to avoid
a U-Boot proper authentication failure in HAB closed devices. The
current U-Boot tools code is hardcoding CSF_SIZE as 0x2000 and
mx6ul_14x14_defconfig target is failing to boot with error below:

Authenticate image from DDR location 0x877fffc0...
bad magic magic=0x32 length=0x6131 version=0x38
bad length magic=0x32 length=0x6131 version=0x38
bad version magic=0x32 length=0x6131 version=0x38
spl: ERROR:  image authentication fail

The intent of "habv4: tools: Avoid hardcoded CSF size for SPL targets"
is to avoid such issue. I tried to apply this patch back but I'm
seeing gunzip related errors as reported by Igor Opaniuk. We may need
to understand better this dependency.

I have just submitted a patch reducing default CSF_SIZE to 0x2060,
this patch is also modifying default_image.c and image.c but CSF_SIZE
still hardcoded in U-Boot tools code.

Thanks,
Breno Lima

[U-Boot] [PATCH 2/4] habv4: tools: Avoid hardcoded CSF size for SPL targets

[sbabic at denx.de]

> Currently it's not possible to authenticate the U-Boot proper of
> mx6ul_14x14_evk_defconfig target:
> Authenticate image from DDR location 0x877fffc0...
> bad magic magic=0x0 length=0x00 version=0x3
> bad length magic=0x0 length=0x00 version=0x3
> bad version magic=0x0 length=0x00 version=0x3
> spl: ERROR:  image authentication fail
> Commit 0633e134784a ("imx: hab: Increase CSF_SIZE for i.MX6 and
> i.MX7 devices") has increased CSF_SIZE to avoid a possible issue
> when booting encrypted boot images.
> Commit d21bd69b6e95 ("tools: mkimage: add firmware-ivt image type
> for HAB verification") is hardcoding the CSF and IVT sizes, the
> new CSF size is not being considered and u-boot-ivt.img fails to
> boot.
> Avoid hardcoded CSF and IVT size to fix this issue.
> Signed-off-by: Breno Lima <breno.lima@nxp.com>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>

Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================