diff mbox series

[v2,04/23] arm: imx: hab: Optimise flow of authenticate_image on hab_entry fail

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

Commit Message

Bryan O'Donoghue Dec. 28, 2017, 6:49 p.m. UTC
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(-)

Comments

Breno Matheus Lima Dec. 29, 2017, 4:36 p.m. UTC | #1
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
Bryan O'Donoghue Dec. 29, 2017, 5:27 p.m. UTC | #2
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 mbox series

Patch

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)