Re: [U-Boot] [PATCH v2 04/23] arm: imx: hab: Optimise flow of authenticate_image on hab_entry fail

[Bryan O'Donoghue]

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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 04/23] arm: imx: hab: Optimise flow of authenticate_image on hab_entry fail

[Breno Matheus Lima]

Hi Bryan,

2017-12-28 16:49 GMT-02:00 Bryan O'Donoghue :
> 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 
> Cc: Stefano Babic 
> Cc: Fabio Estevam 
> Cc: Peng Fan 
> Cc: Albert Aribaud 
> Cc: Sven Ebenfeld 
> Cc: George McCollister 
> Cc: Breno Matheus Lima 
> ---
>  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);
> -