| Message ID | 1515433001-13857-4-git-send-email-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix and extend i.MX HAB layer | expand |
Hi Bryan, 2018-01-08 15:36 GMT-02:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>: > There is no need to call is_enabled() twice in authenticate_image - it does > nothing but add an additional layer of indentation. > > We can check for is_enabled() at the start of the function and return the > result code directly. > > 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 | 138 ++++++++++++++++++++++++------------------------ > 1 file changed, 69 insertions(+), 69 deletions(-) > > diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c > index 9fe6d43..6f86c02 100644 > --- a/arch/arm/mach-imx/hab.c > +++ b/arch/arm/mach-imx/hab.c > @@ -428,91 +428,91 @@ int authenticate_image(uint32_t ddr_start, uint32_t image_size) > hab_rvt_entry = hab_rvt_entry_p; > hab_rvt_exit = hab_rvt_exit_p; > > - if (is_hab_enabled()) { > - printf("\nAuthenticate image from DDR location 0x%x...\n", > - ddr_start); > + if (!is_hab_enabled()) { > + puts("hab fuse not enabled\n"); > + return result; > + } Just another comment in your series, I was working on an open i.MX6UL EVK and noticed that we may have problem with SPL boards in open mode: U-Boot SPL 2018.01-rc3-39088-g59b5c4e (Jan 11 2018 - 16:32:52) Trying to boot from MMC1 hab fuse not enabled spl: ERROR: image authentication unsuccessful ### ERROR ### Please RESET the board ### The imx_hab_authenticate_image() is returning an authentication failure in this case and we cannot boot u-boot-ivt.img. In this way is not possible to run hab_status before closing the device. I think the following fix this issue: --- a/arch/arm/mach-imx/hab.c +++ b/arch/arm/mach-imx/hab.c @@ -492,7 +492,7 @@ int imx_hab_authenticate_image(uint32_t ddr_start, uint32_t image_size, if (!imx_hab_is_enabled()) { puts("hab fuse not enabled\n"); - return result; + goto hab_exit; } printf("\nAuthenticate image from DDR location 0x%x...\n", @@ -594,7 +594,8 @@ hab_exit_failure_print_status: hab_caam_clock_disable: hab_caam_clock_enable(0); - if (load_addr != 0) +hab_exit: + if (load_addr != 0 || (!imx_hab_is_enabled())) result = 0; Can you please check if is possible to add this on your series? If is too late I can submit a patch. Thanks, Breno Lima
On 11/01/18 19:34, Breno Matheus Lima wrote: > Hi Bryan, > > 2018-01-08 15:36 GMT-02:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>: >> There is no need to call is_enabled() twice in authenticate_image - it does >> nothing but add an additional layer of indentation. >> >> We can check for is_enabled() at the start of the function and return the >> result code directly. >> >> 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 | 138 ++++++++++++++++++++++++------------------------ >> 1 file changed, 69 insertions(+), 69 deletions(-) >> >> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c >> index 9fe6d43..6f86c02 100644 >> --- a/arch/arm/mach-imx/hab.c >> +++ b/arch/arm/mach-imx/hab.c >> @@ -428,91 +428,91 @@ int authenticate_image(uint32_t ddr_start, uint32_t image_size) >> hab_rvt_entry = hab_rvt_entry_p; >> hab_rvt_exit = hab_rvt_exit_p; >> >> - if (is_hab_enabled()) { >> - printf("\nAuthenticate image from DDR location 0x%x...\n", >> - ddr_start); >> + if (!is_hab_enabled()) { >> + puts("hab fuse not enabled\n"); >> + return result; >> + } > > Just another comment in your series, I was working on an open i.MX6UL > EVK and noticed that we may have problem with SPL boards in open mode: > > U-Boot SPL 2018.01-rc3-39088-g59b5c4e (Jan 11 2018 - 16:32:52) > Trying to boot from MMC1 > hab fuse not enabled > spl: ERROR: image authentication unsuccessful > ### ERROR ### Please RESET the board ### > > The imx_hab_authenticate_image() is returning an authentication > failure in this case and we cannot boot u-boot-ivt.img. In this way is > not possible to run hab_status before closing the device. So, at least on the MX7 - bootrom->authenticate_image() won't run unless the board is in locked mode, I suspect that's the same behavior on the MX6. > I think the following fix this issue: > > --- a/arch/arm/mach-imx/hab.c > +++ b/arch/arm/mach-imx/hab.c > @@ -492,7 +492,7 @@ int imx_hab_authenticate_image(uint32_t ddr_start, > uint32_t image_size, > > if (!imx_hab_is_enabled()) { > puts("hab fuse not enabled\n"); > - return result; > + goto hab_exit; Right, what you want is for uboot->authenticate_image() to return zero (OK) when the part is unlocked. If this were the first time we were adding this code in, I'd say that would be a crazy thing to do because 1. bootrom->authenticate_image() will return an error 2. uboot->authenticate_image() will not even call bootrom->authenticate_image() However since you already have a dependency on this behavior - I'll respin this code .... --- bod
diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c index 9fe6d43..6f86c02 100644 --- a/arch/arm/mach-imx/hab.c +++ b/arch/arm/mach-imx/hab.c @@ -428,91 +428,91 @@ int authenticate_image(uint32_t ddr_start, uint32_t image_size) hab_rvt_entry = hab_rvt_entry_p; hab_rvt_exit = hab_rvt_exit_p; - if (is_hab_enabled()) { - printf("\nAuthenticate image from DDR location 0x%x...\n", - ddr_start); + if (!is_hab_enabled()) { + puts("hab fuse not enabled\n"); + return result; + } - hab_caam_clock_enable(1); + printf("\nAuthenticate image from DDR location 0x%x...\n", + ddr_start); - if (hab_rvt_entry() == HAB_SUCCESS) { - /* If not already aligned, Align to ALIGN_SIZE */ - ivt_offset = (image_size + ALIGN_SIZE - 1) & - ~(ALIGN_SIZE - 1); + hab_caam_clock_enable(1); - start = ddr_start; - bytes = ivt_offset + IVT_SIZE + CSF_PAD_SIZE; + if (hab_rvt_entry() == HAB_SUCCESS) { + /* 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; } + } else { + puts("hab entry function fail\n"); + } - hab_caam_clock_enable(0); + hab_caam_clock_enable(0); #if !defined(CONFIG_SPL_BUILD) - get_hab_status(); + get_hab_status(); #endif - } else { - puts("hab fuse not enabled\n"); - } - - if ((!is_hab_enabled()) || (load_addr != 0)) + if (load_addr != 0) result = 0; return result;
There is no need to call is_enabled() twice in authenticate_image - it does nothing but add an additional layer of indentation. We can check for is_enabled() at the start of the function and return the result code directly. 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 | 138 ++++++++++++++++++++++++------------------------ 1 file changed, 69 insertions(+), 69 deletions(-)