| Message ID | 1514486982-19059-5-git-send-email-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix and extend i.MX HAB layer | expand |
Hi Bryan, 2017-12-28 16:49 GMT-02:00 Bryan O'Donoghue <bryan.odonoghue@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@linaro.org> > Cc: Stefano Babic <sbabic@denx.de> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > Cc: Sven Ebenfeld <sven.ebenfeld@gmail.com> > Cc: George McCollister <george.mccollister@gmail.com> > Cc: Breno Matheus Lima <brenomatheus@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
On 29/12/17 16:36, Breno Matheus Lima wrote: > 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. Hi Breno. Honestly speaking I'm not a great fan of all of the noisiness of authenticate_image() - not entirely sure what value there is in printing this stuff at all but.. since it was already behaving in this way I assume it has/had some value to others. I agree with you here though it shouldn't say "No HAB Events Found!" since this is what is putatively printed out when everything works, as opposed to when its broken. I'll have a look at changing this a little later tonight
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)
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@linaro.org> Cc: Stefano Babic <sbabic@denx.de> Cc: Fabio Estevam <fabio.estevam@nxp.com> Cc: Peng Fan <peng.fan@nxp.com> Cc: Albert Aribaud <albert.u.boot@aribaud.net> Cc: Sven Ebenfeld <sven.ebenfeld@gmail.com> Cc: George McCollister <george.mccollister@gmail.com> Cc: Breno Matheus Lima <brenomatheus@gmail.com> --- arch/arm/mach-imx/hab.c | 118 ++++++++++++++++++++++++------------------------ 1 file changed, 60 insertions(+), 58 deletions(-)