[U-Boot] [PATCH v2 0/2] Fix CAAM for TrustZone enable for warp7

[U-Boot] [PATCH v2 0/2] Fix CAAM for TrustZone enable for warp7

[Bryan O'Donoghue]

V2:
- Add an explicit assignment of JRMID when setting job-ring ownership
  Required on my reference part where the JRMID field is not set on the
  third job-ring

V1:
This series is the u-boot fix to a problem we encountered when enabling
OPTEE/TrustZone on the WaRP7. The symptom is once TrustZone is activated
the first page of CAAM registers becomes read-only, read-zero from the
perspective of Linux and other non TrustZone contexts.

Offlining the problem with Peng Fan[1] we eventually came to realise the
problem could be worked around by

1. Making Linux skip RNG initialisation - a set of patches should be
   hitting LKML to do just that.

2. Initialising the RNG either from u-boot or OPTEE. In this case u-boot is
   the right place to-do that because there's upstream code in u-boot that
   just works. Patch #2 does that for the WaRP7.

3. Ensuring the job-ring registers are assigned to the non TrustZone mode.
   On the i.MX7 after the BootROM runs the job-ring registers are assigned
   to TrustZone. Patch #1 does that for all CAAM hardware.

On point #3 this ordinarily isn't a problem because unless TrustZone is
activated the restrictions on the job-ring registers don't kick in, its
only after enabling TrustZone that Linux will loose access to the job-ring
registers.

Finally should OPTEE or another TEE want to do things with the job-ring
registers it will have sufficient privilege to assign whichever job-ring
registers it wants to OPTEE/TEE but will naturally then have to arbitrate
with Linux to inform the Kernel CAAM driver which job-ring registers it can
and cannot access.

That arbitration process is for a future putative OPTEE/TEE CAAM driver to
solve and is out of scope of this patchset.

[1] Thanks for all of your help BTW - Peng, there's no way this would be
    working without you giving direction on how.

Bryan O'Donoghue (2):
  drivers/crypto/fsl: assign job-rings to non-TrustZone
  warp7 : run sec_init for CAAM RNG

 board/warp7/warp7.c     | 6 +++++-
 drivers/crypto/fsl/jr.c | 9 +++++++++
 drivers/crypto/fsl/jr.h | 2 ++
 3 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.7.4

[U-Boot] [PATCH v2 1/2] drivers/crypto/fsl: assign job-rings to non-TrustZone

[Bryan O'Donoghue]

After enabling TrustZone various parts of the CAAM silicon become
inaccessible to non TrustZone contexts. The job-ring registers are designed
to allow non TrustZone contexts like Linux to still submit jobs to CAAM
even after TrustZone has been enabled.

The default job-ring permissions after the BootROM look like this for
job-ring zero.

ms=0x00008001 ls=0x00008001

The MS field is JRaMIDR_MS (job ring MID most significant).

Referring to "Security Reference Manual for i.MX 7Dual and 7Solo
Applications Processors, Rev. 0, 03/2017" section 8.10.4 we see that
JROWN_NS controls whether or not a job-ring is accessible from non
TrustZone.

Bit 15 (TrustZone) is the logical inverse of bit 3 hence the above value of
0x8001 shows that JROWN_NS=0 and TrustZone=1.

Clearly then as soon as TrustZone becomes active the job-ring registers are
no longer accessible from Linux, which is not what we want.

This patch explicitly sets all job-ring registers to JROWN_NS=1 (non
TrustZone) by default and to the Non-Secure MID 001. Both settings are
required to successfully assign a job-ring to non-secure mode. If a piece
of TrustZone firmware requires ownership of job-ring registers it can unset
the JROWN_NS bit itself.

This patch in conjunction with a modification of the Linux kernel to skip
HWRNG initialisation makes CAAM usable to Linux with TrustZone enabled.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Alex Porosanu <alexandru.porosanu@nxp.com>
Cc: Ruchika Gupta <ruchika.gupta@nxp.com>
Cc: Aneesh Bansal <aneesh.bansal@nxp.com>
Link: https://github.com/OP-TEE/optee_os/issues/1408
Link: https://tinyurl.com/yam5gv9a
---
 drivers/crypto/fsl/jr.c | 9 +++++++++
 drivers/crypto/fsl/jr.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index a6dad01..34bd070 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -579,6 +579,8 @@ int sec_init_idx(uint8_t sec_idx)
 {
 	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
 	uint32_t mcr = sec_in32(&sec->mcfgr);
+	uint32_t jrown_ns;
+	int i;
 	int ret = 0;
 
 #ifdef CONFIG_FSL_CORENET
@@ -634,6 +636,13 @@ int sec_init_idx(uint8_t sec_idx)
 #endif
 #endif
 
+	/* Set ownership of job rings to non-TrustZone mode by default */
+	for (i = 0; i < ARRAY_SIZE(sec->jrliodnr); i++) {
+		jrown_ns = sec_in32(&sec->jrliodnr[i].ms);
+		jrown_ns |= JROWN_NS | JRMID_NS;
+		sec_out32(&sec->jrliodnr[i].ms, jrown_ns);
+	}
+
 	ret = jr_init(sec_idx);
 	if (ret < 0) {
 		printf("SEC initialization failed\n");
diff --git a/drivers/crypto/fsl/jr.h b/drivers/crypto/fsl/jr.h
index f546226..ef515e7 100644
--- a/drivers/crypto/fsl/jr.h
+++ b/drivers/crypto/fsl/jr.h
@@ -34,6 +34,8 @@
 #define JRNSLIODN_MASK		0x0fff0000
 #define JRSLIODN_SHIFT		0
 #define JRSLIODN_MASK		0x00000fff
+#define JROWN_NS		0x00000008
+#define JRMID_NS		0x00000001
 
 #define JQ_DEQ_ERR		-1
 #define JQ_DEQ_TO_ERR		-2
-- 
2.7.4

[U-Boot] [PATCH v2 1/2] drivers/crypto/fsl: assign job-rings to non-TrustZone

[Auer, Lukas]

On Fri, 2018-01-26 at 02:09 +0000, Bryan O'Donoghue wrote:
> After enabling TrustZone various parts of the CAAM silicon become
> inaccessible to non TrustZone contexts. The job-ring registers are
> designed
> to allow non TrustZone contexts like Linux to still submit jobs to
> CAAM
> even after TrustZone has been enabled.
> 
> The default job-ring permissions after the BootROM look like this for
> job-ring zero.
> 
> ms=0x00008001 ls=0x00008001
> 
> The MS field is JRaMIDR_MS (job ring MID most significant).
> 
> Referring to "Security Reference Manual for i.MX 7Dual and 7Solo
> Applications Processors, Rev. 0, 03/2017" section 8.10.4 we see that
> JROWN_NS controls whether or not a job-ring is accessible from non
> TrustZone.
> 
> Bit 15 (TrustZone) is the logical inverse of bit 3 hence the above
> value of
> 0x8001 shows that JROWN_NS=0 and TrustZone=1.
> 
> Clearly then as soon as TrustZone becomes active the job-ring
> registers are
> no longer accessible from Linux, which is not what we want.
> 
> This patch explicitly sets all job-ring registers to JROWN_NS=1 (non
> TrustZone) by default and to the Non-Secure MID 001. Both settings
> are
> required to successfully assign a job-ring to non-secure mode. If a
> piece
> of TrustZone firmware requires ownership of job-ring registers it can
> unset
> the JROWN_NS bit itself.
> 
> This patch in conjunction with a modification of the Linux kernel to
> skip
> HWRNG initialisation makes CAAM usable to Linux with TrustZone
> enabled.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Alex Porosanu <alexandru.porosanu@nxp.com>
> Cc: Ruchika Gupta <ruchika.gupta@nxp.com>
> Cc: Aneesh Bansal <aneesh.bansal@nxp.com>
> Link: https://github.com/OP-TEE/optee_os/issues/1408
> Link: https://tinyurl.com/yam5gv9a
> ---
>  drivers/crypto/fsl/jr.c | 9 +++++++++
>  drivers/crypto/fsl/jr.h | 2 ++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
> index a6dad01..34bd070 100644
> --- a/drivers/crypto/fsl/jr.c
> +++ b/drivers/crypto/fsl/jr.c
> @@ -579,6 +579,8 @@ int sec_init_idx(uint8_t sec_idx)
>  {
>  	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
>  	uint32_t mcr = sec_in32(&sec->mcfgr);
> +	uint32_t jrown_ns;
> +	int i;
>  	int ret = 0;
>  
>  #ifdef CONFIG_FSL_CORENET
> @@ -634,6 +636,13 @@ int sec_init_idx(uint8_t sec_idx)
>  #endif
>  #endif
>  
> +	/* Set ownership of job rings to non-TrustZone mode by
> default */
> +	for (i = 0; i < ARRAY_SIZE(sec->jrliodnr); i++) {
> +		jrown_ns = sec_in32(&sec->jrliodnr[i].ms);
> +		jrown_ns |= JROWN_NS | JRMID_NS;
> +		sec_out32(&sec->jrliodnr[i].ms, jrown_ns);
> +	}
> +
>  	ret = jr_init(sec_idx);
>  	if (ret < 0) {
>  		printf("SEC initialization failed\n");
> diff --git a/drivers/crypto/fsl/jr.h b/drivers/crypto/fsl/jr.h
> index f546226..ef515e7 100644
> --- a/drivers/crypto/fsl/jr.h
> +++ b/drivers/crypto/fsl/jr.h
> @@ -34,6 +34,8 @@
>  #define JRNSLIODN_MASK		0x0fff0000
>  #define JRSLIODN_SHIFT		0
>  #define JRSLIODN_MASK		0x00000fff
> +#define JROWN_NS		0x00000008
> +#define JRMID_NS		0x00000001
>  
>  #define JQ_DEQ_ERR		-1
>  #define JQ_DEQ_TO_ERR		-2

Hi Bryan,

just successfully tested this on my imx7d board. Your addition in v2
was quite important since job ring 2 fails to probe otherwise (was
wondering why that happened yesterday). The CAAM and all job rings
probe successfully now.

Tested-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>

[U-Boot] [PATCH v2 2/2] warp7 : run sec_init for CAAM RNG

[Bryan O'Donoghue]

This patch adds a sec_init call into board_init. Doing so in conjunction
with the patch "drivers/crypto/fsl: assign job-rings to non-TrustZone"
enables use of the CAAM in Linux when OPTEE/TrustZone is active.

u-boot will initialise the RNG and assign ownership of the job-ring
registers to a non-TrustZone context. Linux then simply has to detect or be
told to skip RNG initialisation.

This change is safe both for the OPTEE/TrustZone boot path and the regular
non-OPTEE/TrustZone boot path.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Marco Franchi <marco.franchi@nxp.com>
Cc: Vanessa Maegima <vanessa.maegima@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
---
 board/warp7/warp7.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/board/warp7/warp7.c b/board/warp7/warp7.c
index 337e76b..219ab6f 100644
--- a/board/warp7/warp7.c
+++ b/board/warp7/warp7.c
@@ -16,6 +16,7 @@
 #include <asm/io.h>
 #include <common.h>
 #include <fsl_esdhc.h>
+#include <fsl_sec.h>
 #include <i2c.h>
 #include <mmc.h>
 #include <asm/arch/crm_regs.h>
@@ -225,6 +226,10 @@ int board_init(void)
 		setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info1);
 	#endif
 
+	#ifdef CONFIG_FSL_CAAM
+		sec_init();
+	#endif
+
 	return 0;
 }
 
@@ -366,5 +371,4 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
 
 	return 0;
 }
-
 #endif /* ifdef CONFIG_USB_GADGET */
-- 
2.7.4

[U-Boot] [PATCH v2 2/2] warp7 : run sec_init for CAAM RNG

[Auer, Lukas]

On Fri, 2018-01-26 at 02:09 +0000, Bryan O'Donoghue wrote:
> This patch adds a sec_init call into board_init. Doing so in
> conjunction
> with the patch "drivers/crypto/fsl: assign job-rings to non-
> TrustZone"
> enables use of the CAAM in Linux when OPTEE/TrustZone is active.
> 
> u-boot will initialise the RNG and assign ownership of the job-ring
> registers to a non-TrustZone context. Linux then simply has to detect
> or be
> told to skip RNG initialisation.
> 
> This change is safe both for the OPTEE/TrustZone boot path and the
> regular
> non-OPTEE/TrustZone boot path.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Marco Franchi <marco.franchi@nxp.com>
> Cc: Vanessa Maegima <vanessa.maegima@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  board/warp7/warp7.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/board/warp7/warp7.c b/board/warp7/warp7.c
> index 337e76b..219ab6f 100644
> --- a/board/warp7/warp7.c
> +++ b/board/warp7/warp7.c
> @@ -16,6 +16,7 @@
>  #include <asm/io.h>
>  #include <common.h>
>  #include <fsl_esdhc.h>
> +#include <fsl_sec.h>
>  #include <i2c.h>
>  #include <mmc.h>
>  #include <asm/arch/crm_regs.h>
> @@ -225,6 +226,10 @@ int board_init(void)
>  		setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f,
> &i2c_pad_info1);
>  	#endif
>  
> +	#ifdef CONFIG_FSL_CAAM
> +		sec_init();
> +	#endif
> +
>  	return 0;
>  }
>  
> @@ -366,5 +371,4 @@ int g_dnl_bind_fixup(struct usb_device_descriptor
> *dev, const char *name)
>  
>  	return 0;
>  }
> -
>  #endif /* ifdef CONFIG_USB_GADGET */

Hi Bryan,

this fails to apply for me on current HEAD. It seems like you have
additional modifications to wrap7.c in your tree (there is no
CONFIG_USB_GADGET on master).

Regarding the patch, would it make sense to put sec_init() somewhere
else, so that it does not have to be duplicated in the board file for
all platforms with CAAM?

Thanks,
Lukas

[U-Boot] [PATCH v2 2/2] warp7 : run sec_init for CAAM RNG

[Bryan O'Donoghue]

On 26/01/18 09:09, Auer, Lukas wrote:
> Hi Bryan,
> 
> this fails to apply for me on current HEAD. It seems like you have
> additional modifications to wrap7.c in your tree (there is no
> CONFIG_USB_GADGET on master).

I'm carrying a few patches locally and upstreaming gradually - got 
caught out here...

> Regarding the patch, would it make sense to put sec_init() somewhere
> else, so that it does not have to be duplicated in the board file for
> all platforms with CAAM?

It does... to me. Looking at these .. I'd say leave the old 
powerpc/freescale stuff alone.

This works for me as an alternative when I tested it

diff --git a/arch/arm/mach-imx/mx7/soc.c b/arch/arm/mach-imx/mx7/soc.c
index d160e80..d399fd8 100644
--- a/arch/arm/mach-imx/mx7/soc.c
+++ b/arch/arm/mach-imx/mx7/soc.c
@@ -261,6 +261,9 @@ int arch_misc_init(void)
         else
                 env_set("soc", "imx7s");
  #endif
+       #ifdef CONFIG_FSL_CAAM
+               sec_init();
+       #endif

         return 0;
  }

perhaps this would work for other i.mx processors

diff --git a/arch/arm/mach-imx/mx6/soc.c b/arch/arm/mach-imx/mx6/soc.c
index 43cb581..679c23b 100644
--- a/arch/arm/mach-imx/mx6/soc.c
+++ b/arch/arm/mach-imx/mx6/soc.c
@@ -515,6 +515,10 @@ int board_postclk_init(void)

         set_ldo_voltage(LDO_SOC, 1175); /* Set VDDSOC to 1.175V */

+#ifdef CONFIG_FSL_CAAM
+       sec_init();
+#endif
+
         return 0;
  }

diff --git a/arch/arm/mach-imx/mx7ulp/soc.c b/arch/arm/mach-imx/mx7ulp/soc.c
index 454665a..dc3d601 100644
--- a/arch/arm/mach-imx/mx7ulp/soc.c
+++ b/arch/arm/mach-imx/mx7ulp/soc.c
@@ -57,6 +57,11 @@ int arch_cpu_init(void)
  #ifdef CONFIG_BOARD_POSTCLK_INIT
  int board_postclk_init(void)
  {
+
+#ifdef CONFIG_FSL_CAAM
+       sec_init();
+#endif
+
         return 0;
  }
  #endif

I'd say the right thing to do is - fix it for all i.MX7D/S and let 
others with access to mx6/mx7ulp etc test/patch themselves.

Anyway I'll send a generic patch for i.mx7s/d in arch_misc_init()

---
bod

[U-Boot] [PATCH v2 2/2] warp7 : run sec_init for CAAM RNG

[Auer, Lukas]

On Fri, 2018-01-26 at 11:32 +0000, Bryan O'Donoghue wrote:
> 
> On 26/01/18 09:09, Auer, Lukas wrote:
> > Hi Bryan,
> > 
> > this fails to apply for me on current HEAD. It seems like you have
> > additional modifications to wrap7.c in your tree (there is no
> > CONFIG_USB_GADGET on master).
> 
> I'm carrying a few patches locally and upstreaming gradually - got 
> caught out here...
> 
> > Regarding the patch, would it make sense to put sec_init()
> > somewhere
> > else, so that it does not have to be duplicated in the board file
> > for
> > all platforms with CAAM?
> 
> It does... to me. Looking at these .. I'd say leave the old 
> powerpc/freescale stuff alone.
> 
> This works for me as an alternative when I tested it
> 
> diff --git a/arch/arm/mach-imx/mx7/soc.c b/arch/arm/mach-
> imx/mx7/soc.c
> index d160e80..d399fd8 100644
> --- a/arch/arm/mach-imx/mx7/soc.c
> +++ b/arch/arm/mach-imx/mx7/soc.c
> @@ -261,6 +261,9 @@ int arch_misc_init(void)
>          else
>                  env_set("soc", "imx7s");
>   #endif
> +       #ifdef CONFIG_FSL_CAAM
> +               sec_init();
> +       #endif
> 
>          return 0;
>   }
> 
> perhaps this would work for other i.mx processors
> 
> diff --git a/arch/arm/mach-imx/mx6/soc.c b/arch/arm/mach-
> imx/mx6/soc.c
> index 43cb581..679c23b 100644
> --- a/arch/arm/mach-imx/mx6/soc.c
> +++ b/arch/arm/mach-imx/mx6/soc.c
> @@ -515,6 +515,10 @@ int board_postclk_init(void)
> 
>          set_ldo_voltage(LDO_SOC, 1175); /* Set VDDSOC to 1.175V */
> 
> +#ifdef CONFIG_FSL_CAAM
> +       sec_init();
> +#endif
> +
>          return 0;
>   }
> 
> diff --git a/arch/arm/mach-imx/mx7ulp/soc.c b/arch/arm/mach-
> imx/mx7ulp/soc.c
> index 454665a..dc3d601 100644
> --- a/arch/arm/mach-imx/mx7ulp/soc.c
> +++ b/arch/arm/mach-imx/mx7ulp/soc.c
> @@ -57,6 +57,11 @@ int arch_cpu_init(void)
>   #ifdef CONFIG_BOARD_POSTCLK_INIT
>   int board_postclk_init(void)
>   {
> +
> +#ifdef CONFIG_FSL_CAAM
> +       sec_init();
> +#endif
> +
>          return 0;
>   }
>   #endif
> 
> I'd say the right thing to do is - fix it for all i.MX7D/S and let 
> others with access to mx6/mx7ulp etc test/patch themselves.
> 
> Anyway I'll send a generic patch for i.mx7s/d in arch_misc_init()
> 
> ---
> bod

I agree, that's a good way to add the initialization code.