Hi Bryan, 2017-12-28 16:49 GMT-02:00 Bryan O'Donoghue <bryan.odonog...@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.odonog...@linaro.org> > Cc: Stefano Babic <sba...@denx.de> > Cc: Fabio Estevam <fabio.este...@nxp.com> > Cc: Peng Fan <peng....@nxp.com> > Cc: Albert Aribaud <albert.u.b...@aribaud.net> > Cc: Sven Ebenfeld <sven.ebenf...@gmail.com> > Cc: George McCollister <george.mccollis...@gmail.com> > Cc: Breno Matheus Lima <brenomath...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot