[U-Boot] [PATCH v2 04/23] arm: imx: hab: Optimise flow of authenticate_image on hab_entry fail
Breno Matheus Lima
brenomatheus at gmail.com
Fri Dec 29 16:36:13 UTC 2017
Hi Bryan,
2017-12-28 16:49 GMT-02:00 Bryan O'Donoghue <bryan.odonoghue at linaro.org>:
> The current code disjoins an entire block of code on hab_entry pass/fail
> resulting in a large chunk of authenticate_image being offset to the right.
>
> Fix this by checking hab_entry() pass/failure and exiting the function
> directly if in an error state.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org>
> Cc: Stefano Babic <sbabic at denx.de>
> Cc: Fabio Estevam <fabio.estevam at nxp.com>
> Cc: Peng Fan <peng.fan at nxp.com>
> Cc: Albert Aribaud <albert.u.boot at aribaud.net>
> Cc: Sven Ebenfeld <sven.ebenfeld at gmail.com>
> Cc: George McCollister <george.mccollister at gmail.com>
> Cc: Breno Matheus Lima <brenomatheus at gmail.com>
> ---
> arch/arm/mach-imx/hab.c | 118 ++++++++++++++++++++++++------------------------
> 1 file changed, 60 insertions(+), 58 deletions(-)
>
> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
> index 6f86c02..f878b7b 100644
> --- a/arch/arm/mach-imx/hab.c
> +++ b/arch/arm/mach-imx/hab.c
> @@ -438,75 +438,77 @@ int authenticate_image(uint32_t ddr_start, uint32_t image_size)
>
> hab_caam_clock_enable(1);
>
> - if (hab_rvt_entry() == HAB_SUCCESS) {
> - /* If not already aligned, Align to ALIGN_SIZE */
> - ivt_offset = (image_size + ALIGN_SIZE - 1) &
> - ~(ALIGN_SIZE - 1);
> + if (hab_rvt_entry() != HAB_SUCCESS) {
> + puts("hab entry function fail\n");
> + goto hab_caam_clock_disable;
> + }
>
> - start = ddr_start;
> - bytes = ivt_offset + IVT_SIZE + CSF_PAD_SIZE;
> + /* If not already aligned, Align to ALIGN_SIZE */
> + ivt_offset = (image_size + ALIGN_SIZE - 1) &
> + ~(ALIGN_SIZE - 1);
> +
> + start = ddr_start;
> + bytes = ivt_offset + IVT_SIZE + CSF_PAD_SIZE;
> #ifdef DEBUG
> - printf("\nivt_offset = 0x%x, ivt addr = 0x%x\n",
> - ivt_offset, ddr_start + ivt_offset);
> - puts("Dumping IVT\n");
> - print_buffer(ddr_start + ivt_offset,
> - (void *)(ddr_start + ivt_offset),
> - 4, 0x8, 0);
> -
> - puts("Dumping CSF Header\n");
> - print_buffer(ddr_start + ivt_offset + IVT_SIZE,
> - (void *)(ddr_start + ivt_offset + IVT_SIZE),
> - 4, 0x10, 0);
> + printf("\nivt_offset = 0x%x, ivt addr = 0x%x\n",
> + ivt_offset, ddr_start + ivt_offset);
> + puts("Dumping IVT\n");
> + print_buffer(ddr_start + ivt_offset,
> + (void *)(ddr_start + ivt_offset),
> + 4, 0x8, 0);
> +
> + puts("Dumping CSF Header\n");
> + print_buffer(ddr_start + ivt_offset + IVT_SIZE,
> + (void *)(ddr_start + ivt_offset + IVT_SIZE),
> + 4, 0x10, 0);
>
> #if !defined(CONFIG_SPL_BUILD)
> - get_hab_status();
> + get_hab_status();
> #endif
>
> - puts("\nCalling authenticate_image in ROM\n");
> - printf("\tivt_offset = 0x%x\n", ivt_offset);
> - printf("\tstart = 0x%08lx\n", start);
> - printf("\tbytes = 0x%x\n", bytes);
> + puts("\nCalling authenticate_image in ROM\n");
> + printf("\tivt_offset = 0x%x\n", ivt_offset);
> + printf("\tstart = 0x%08lx\n", start);
> + printf("\tbytes = 0x%x\n", bytes);
> #endif
> - /*
> - * If the MMU is enabled, we have to notify the ROM
> - * code, or it won't flush the caches when needed.
> - * This is done, by setting the "pu_irom_mmu_enabled"
> - * word to 1. You can find its address by looking in
> - * the ROM map. This is critical for
> - * authenticate_image(). If MMU is enabled, without
> - * setting this bit, authentication will fail and may
> - * crash.
> - */
> - /* Check MMU enabled */
> - if (is_soc_type(MXC_SOC_MX6) && get_cr() & CR_M) {
> - if (is_mx6dq()) {
> - /*
> - * This won't work on Rev 1.0.0 of
> - * i.MX6Q/D, since their ROM doesn't
> - * do cache flushes. don't think any
> - * exist, so we ignore them.
> - */
> - if (!is_mx6dqp())
> - writel(1, MX6DQ_PU_IROM_MMU_EN_VAR);
> - } else if (is_mx6sdl()) {
> - writel(1, MX6DLS_PU_IROM_MMU_EN_VAR);
> - } else if (is_mx6sl()) {
> - writel(1, MX6SL_PU_IROM_MMU_EN_VAR);
> - }
> + /*
> + * If the MMU is enabled, we have to notify the ROM
> + * code, or it won't flush the caches when needed.
> + * This is done, by setting the "pu_irom_mmu_enabled"
> + * word to 1. You can find its address by looking in
> + * the ROM map. This is critical for
> + * authenticate_image(). If MMU is enabled, without
> + * setting this bit, authentication will fail and may
> + * crash.
> + */
> + /* Check MMU enabled */
> + if (is_soc_type(MXC_SOC_MX6) && get_cr() & CR_M) {
> + if (is_mx6dq()) {
> + /*
> + * This won't work on Rev 1.0.0 of
> + * i.MX6Q/D, since their ROM doesn't
> + * do cache flushes. don't think any
> + * exist, so we ignore them.
> + */
> + if (!is_mx6dqp())
> + writel(1, MX6DQ_PU_IROM_MMU_EN_VAR);
> + } else if (is_mx6sdl()) {
> + writel(1, MX6DLS_PU_IROM_MMU_EN_VAR);
> + } else if (is_mx6sl()) {
> + writel(1, MX6SL_PU_IROM_MMU_EN_VAR);
> }
> + }
>
> - load_addr = (uint32_t)hab_rvt_authenticate_image(
> - HAB_CID_UBOOT,
> - ivt_offset, (void **)&start,
> - (size_t *)&bytes, NULL);
> - if (hab_rvt_exit() != HAB_SUCCESS) {
> - puts("hab exit function fail\n");
> - load_addr = 0;
> - }
> - } else {
> - puts("hab entry function fail\n");
> + load_addr = (uint32_t)hab_rvt_authenticate_image(
> + HAB_CID_UBOOT,
> + ivt_offset, (void **)&start,
> + (size_t *)&bytes, NULL);
> + if (hab_rvt_exit() != HAB_SUCCESS) {
> + puts("hab exit function fail\n");
> + load_addr = 0;
> }
>
> +hab_caam_clock_disable:
> hab_caam_clock_enable(0);
>
> #if !defined(CONFIG_SPL_BUILD)
Just a suggestion here, can you please check if it's possible to move
"hab_caam_clock_disable:" after the "#if !defined(CONFIG_SPL_BUILD)"
branch?
I'm authenticating a Kernel image with your patch set applied:
=> fatload mmc 0:1 0x12000000 zImage
reading zImage
7057248 bytes read in 355 ms (19 MiB/s)
=> hab_auth_img 0x12000000 0x6baf60 0x6ba000
Authenticate image from DDR location 0x12000000...
ivt_offset = 0x6ba000, ivt addr = 0x126ba000
ivt entry = 0x12000000, dcd = 0x00000000, csf = 0x126ba020
Dumping IVT
126ba000: 412000d1 12000000 00000000 00000000 .. A............
126ba010: 00000000 126ba000 126ba020 00000000 ......k. .k.....
Dumping CSF Header
126ba020: 425000d4 000c00be 00001703 50000000 ..PB...........P
126ba030: 020c00be 01000009 90040000 000c00ca ................
126ba040: 001dc501 e4070000 000c00be 02000009 ................
126ba050: e8090000 001400ca 001dc502 3c0d0000 ...............<
Secure boot enabled
HAB Configuration: 0xcc, HAB State: 0x99
No HAB Events Found!
Calling authenticate_image in ROM
ivt_offset = 0x6ba000
start = 0x12000000
bytes = 0x6baf60
Secure boot enabled
HAB Configuration: 0xcc, HAB State: 0x99
No HAB Events Found!
Then I'm modifying the content in ivt->self for generating an error:
=> mw 0x126ba014 0x126aa030
=> hab_auth_img 0x12000000 0x6baf60 0x6ba000
Authenticate image from DDR location 0x12000000...
ivt->self 0x126aa030 pointer is 0x126ba000
Secure boot enabled
HAB Configuration: 0xcc, HAB State: 0x99
No HAB Events Found!
=>
In this situation the "hab_rvt_authenticate_image()" is not executed,
It's a bit confusing to receive a "No HAB Events Found!" message after
running hab_auth_img on this case.
Thanks,
Breno Lima
More information about the U-Boot
mailing list