[U-Boot] [PATCH] [RFC] imx: dek_blob: Fix lock-up on dek_blob command

[U-Boot] [PATCH] [RFC] imx: dek_blob: Fix lock-up on dek_blob command

[Henri Roosen]

The function blob_encap_dek accesses a CAAM register
 CONFIG_SYS_FSL_JR0_ADDR + 0x102c, before the CAAM clock has been enabled,
which causes the system to lock-up at the dek_blob command.

This patch enables and disables the CAAM clock, because this is also done in
arch/arm/imx-common/hab.c:authenticate_image(). However in my opinion
controlling the clock should be done in one of the underlaying layers, so
this needs further cleanup.

Please comment on a clean implementation of controlling of the CAAM clock.

Signed-off-by: Henri Roosen <henri.roosen@ginzinger.com>
---
 arch/arm/imx-common/cmd_dek.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm/imx-common/cmd_dek.c b/arch/arm/imx-common/cmd_dek.c
index ada8adf..72a7d49 100644
--- a/arch/arm/imx-common/cmd_dek.c
+++ b/arch/arm/imx-common/cmd_dek.c
@@ -30,20 +30,22 @@ static int blob_encap_dek(const u8 *src, u8 *dst, u32 len)
 {
 	int ret = 0;
 	u32 jr_size = 4;
-
-	u32 out_jr_size = sec_in32(CONFIG_SYS_FSL_JR0_ADDR + 0x102c);
-	if (out_jr_size != jr_size) {
-		hab_caam_clock_enable(1);
-		sec_init();
-	}
+	u32 out_jr_size;
 
 	if (!((len == 128) | (len == 192) | (len == 256))) {
 		debug("Invalid DEK size. Valid sizes are 128, 192 and 256b\n");
 		return -1;
 	}
 
+	hab_caam_clock_enable(1);
+	out_jr_size = sec_in32(CONFIG_SYS_FSL_JR0_ADDR + 0x102c);
+	if (out_jr_size != jr_size) {
+		sec_init();
+	}
+
 	len /= 8;
 	ret = blob_dek(src, dst, len);
+	hab_caam_clock_enable(0);
 
 	return ret;
 }
-- 
2.1.4

[U-Boot] [PATCH] [RFC] imx: dek_blob: Fix lock-up on dek_blob command

[Stefano Babic]

Hi Henri,

I have not forgotten your patc, just postponed after the release.

On 13/10/2017 12:53, Henri Roosen wrote:
> The function blob_encap_dek accesses a CAAM register
>  CONFIG_SYS_FSL_JR0_ADDR + 0x102c, before the CAAM clock has been enabled,
> which causes the system to lock-up at the dek_blob command.
> 

Agree with analyses - if clock is not enabled, it hangs.

> This patch enables and disables the CAAM clock, because this is also done in
> arch/arm/imx-common/hab.c:authenticate_image(). However in my opinion
> controlling the clock should be done in one of the underlaying layers, so
> this needs further cleanup.
> 
> Please comment on a clean implementation of controlling of the CAAM clock.

I am just thinking about what you are meaning with underlying layer. The
thing is that this cmd_dek is quite unrelated to a underlying crypto
driver, that for CAAM does not exists in U-Boot.

One point is that all HAB related functions are in
arch/arm/mach-imx/hab.c - having a hab.c and cmd_dek.c can be confusing.
One way could be to add accessors to access CAAM (read / write), and
this should take care if clock is enabled, letting it transparent to the
CAAM "user".

Let me know if you would like to rework this - else I beg you to rebase
it on current tree (imx-coomon is now mach-imx), and add comments inside
the code about the reason clock must be enabled.

Best regards,
Stefano Babic

> 
> Signed-off-by: Henri Roosen <henri.roosen@ginzinger.com>
> ---
>  arch/arm/imx-common/cmd_dek.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/imx-common/cmd_dek.c b/arch/arm/imx-common/cmd_dek.c
> index ada8adf..72a7d49 100644
> --- a/arch/arm/imx-common/cmd_dek.c
> +++ b/arch/arm/imx-common/cmd_dek.c
> @@ -30,20 +30,22 @@ static int blob_encap_dek(const u8 *src, u8 *dst, u32 len)
>  {
>  	int ret = 0;
>  	u32 jr_size = 4;
> -
> -	u32 out_jr_size = sec_in32(CONFIG_SYS_FSL_JR0_ADDR + 0x102c);
> -	if (out_jr_size != jr_size) {
> -		hab_caam_clock_enable(1);
> -		sec_init();
> -	}
> +	u32 out_jr_size;
>  
>  	if (!((len == 128) | (len == 192) | (len == 256))) {
>  		debug("Invalid DEK size. Valid sizes are 128, 192 and 256b\n");
>  		return -1;
>  	}
>  
> +	hab_caam_clock_enable(1);
> +	out_jr_size = sec_in32(CONFIG_SYS_FSL_JR0_ADDR + 0x102c);
> +	if (out_jr_size != jr_size) {
> +		sec_init();
> +	}
> +
>  	len /= 8;
>  	ret = blob_dek(src, dst, len);
> +	hab_caam_clock_enable(0);
>  
>  	return ret;
>  }
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================