[U-Boot] [PATCH 0/4 RFC] imx: Implement job-ring context switching

[U-Boot] [PATCH 0/4 RFC] imx: Implement job-ring context switching

[Bryan O'Donoghue]

This series implements an RFC to save/restore CAAM settings for the
job-rings prior to performing DEK blob verification.

This follows on from a converstion with Breno and Fabio where we discussed
how i.MX HAB implementations for the i.MX6 and i.MX7 will verify job-ring
ownership when doing DEK blob verification, which contrasts to HAB
authenticate image callbacks.

https://marc.info/?l=u-boot&m=155448099126800&w=2

The objective is to make job-ring ownership normal-world when handing over
from u-boot, so that a secure-world or normal-world Linux kernel has full
access to the CAAM job-rings.

By switching job-ring ownership to secure world prior to DEK blob
verification, we ensure the BootROM will be happy with the job-ring
ownership bits. Once DEK verification is complete we switch the job rings
back to normal world so that subsequent boot phases can be in either secure
or normal world.

Please note: compile tested but not runtime tested, I don't currently have
DEK blob encrypted images to test against - hence RFC on this patchset.

Bryan O'Donoghue (4):
  crypto/fsl: Introduce API to save/restore job-ring context
  crypto/fsl: Use __sec_set_jr_context_normal
  powerpc: mpc85xx: crypto: Implement mpc85xxx specific job-ring fix
  crypto/fsl: Wrapper run_descriptor_jr_idx() to set jr permissions

 arch/powerpc/cpu/mpc85xx/cpu_init.c | 22 ++++++++++++
 drivers/crypto/fsl/jr.c             | 53 +++++++++++++++++++++++++----
 include/fsl_sec.h                   |  3 ++
 3 files changed, 71 insertions(+), 7 deletions(-)

-- 
2.20.1

[U-Boot] [PATCH 1/4] crypto/fsl: Introduce API to save/restore job-ring context

[Bryan O'Donoghue]

We need to handle the case where DEK blobs are passed to the BootROM. In
this case, unlike in HAB authentication the BootROM checks job-ring
ownership set to secure world.

One possible solution is to set the job-ring ownership to the expected
state for DEK blobs and then restore to whatever the run-time wants.

For the case where Linux runs in normal-world we would want to set the
job-ring ownership to normal-world.

The first step in the ownership context switch dance is making an API to do
it.

This patch introduces:

void __weak sec_set_jr_context_secure(void);
void __weak sec_set_jr_context_normal(void);

This can be over-ridden for a given architecture, as will be necessary for
the MPC85xxx

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/crypto/fsl/jr.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/fsl_sec.h       |  3 +++
 2 files changed, 41 insertions(+)

diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index cc8d3b02a5..7b13aa4a61 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -574,6 +574,44 @@ static int rng_init(uint8_t sec_idx)
 	return ret;
 }
 #endif
+
+static void __sec_set_jr_context_secure(uint8_t sec_idx)
+{
+	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
+	uint32_t jrown_ns;
+	int i;
+
+	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);
+	}
+
+}
+
+static void __sec_set_jr_context_normal(uint8_t sec_idx)
+{
+	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
+	uint32_t jrown_ns;
+	int i;
+
+	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);
+	}
+}
+
+void __weak sec_set_jr_context_secure(void)
+{
+	__sec_set_jr_context_secure(0);
+}
+
+void __weak sec_set_jr_context_normal(void)
+{
+	__sec_set_jr_context_normal(0);
+}
+
 int sec_init_idx(uint8_t sec_idx)
 {
 	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
diff --git a/include/fsl_sec.h b/include/fsl_sec.h
index be08a2b88b..399cfd091b 100644
--- a/include/fsl_sec.h
+++ b/include/fsl_sec.h
@@ -319,4 +319,7 @@ int sec_init_idx(uint8_t);
 int sec_init(void);
 #endif
 
+void sec_set_jr_context_secure(void);
+void sec_set_jr_context_normal(void);
+
 #endif /* __FSL_SEC_H */
-- 
2.20.1

[U-Boot] [PATCH 2/4] crypto/fsl: Use __sec_set_jr_context_normal

[Bryan O'Donoghue]

Use __sec_set_jr_context_normal() to set job-ring ownership rather than the
current in-line array walk.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/crypto/fsl/jr.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index 7b13aa4a61..65982b8369 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -616,7 +616,6 @@ 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;
 
@@ -674,11 +673,7 @@ int sec_init_idx(uint8_t sec_idx)
 #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);
-	}
+	__sec_set_jr_context_normal(sec_idx);
 
 	ret = jr_init(sec_idx);
 	if (ret < 0) {
-- 
2.20.1

[U-Boot] [PATCH 3/4] powerpc: mpc85xx: crypto: Implement mpc85xxx specific job-ring fix

[Bryan O'Donoghue]

The mpc85xxx has more than one sec block. As a result we need to have an
architecture specific version of:

void sec_set_jr_context_secure(void);
void sec_set_jr_context_normal(void);

This patch implements those functions.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 arch/powerpc/cpu/mpc85xx/cpu_init.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init.c b/arch/powerpc/cpu/mpc85xx/cpu_init.c
index cbcd62e19a..7f007f4f88 100644
--- a/arch/powerpc/cpu/mpc85xx/cpu_init.c
+++ b/arch/powerpc/cpu/mpc85xx/cpu_init.c
@@ -1056,3 +1056,25 @@ int board_late_init(void)
 	return 0;
 }
 #endif
+
+#if defined(CONFIG_ARCH_C29X)
+void sec_set_jr_context_secure(void)
+{
+	if ((SVR_SOC_VER(svr) == SVR_C292) ||
+	    (SVR_SOC_VER(svr) == SVR_C293))
+		sec_set_jr_context_secure(1);
+
+	if (SVR_SOC_VER(svr) == SVR_C293)
+		sec_set_jr_context_secure(2);
+}
+
+void sec_set_jr_context_normal(void)
+{
+	if ((SVR_SOC_VER(svr) == SVR_C292) ||
+	    (SVR_SOC_VER(svr) == SVR_C293))
+		sec_set_jr_context_normal(1);
+
+	if (SVR_SOC_VER(svr) == SVR_C293)
+		sec_set_jr_context_normal(2);
+}
+#endif
-- 
2.20.1

[U-Boot] [PATCH 4/4] crypto/fsl: Wrapper run_descriptor_jr_idx() to set jr permissions

[Bryan O'Donoghue]

This patch sets the relevant set of job-rings to secure-world prior to
calling into run_descriptor_jr_idx(). As observed by Breno Matheus Lima the
DEK blob verification layer in NXP BootROMs performs a check on job-ring
ownership and requires the permission to be set to secure world.

Once run_descriptor_jr_idx() is complete we switch back to normal-world
ownership. Normal world job-ring ownership allows Linux to run in either
secure or normal world when using the CAAM, irrespective which is
ultimately what we want to support.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/crypto/fsl/jr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index 65982b8369..8ab92ad2f1 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -389,7 +389,13 @@ out:
 
 int run_descriptor_jr(uint32_t *desc)
 {
-	return run_descriptor_jr_idx(desc, 0);
+	int ret;
+
+	sec_set_jr_context_secure();
+	ret = run_descriptor_jr_idx(desc, 0);
+	sec_set_jr_context_normal();
+
+	return ret;
 }
 
 static inline int jr_reset_sec(uint8_t sec_idx)
-- 
2.20.1

[U-Boot] [PATCH 2/4] crypto/fsl: Use __sec_set_jr_context_normal

[Breno Matheus Lima]

Hi Bryan,

Em ter, 23 de abr de 2019 às 07:20, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> escreveu:
>
> Use __sec_set_jr_context_normal() to set job-ring ownership rather than the
> current in-line array walk.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/crypto/fsl/jr.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
> index 7b13aa4a61..65982b8369 100644
> --- a/drivers/crypto/fsl/jr.c
> +++ b/drivers/crypto/fsl/jr.c
> @@ -616,7 +616,6 @@ 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;

We may also need to remove this variable otherwise we get build warning below:

drivers/crypto/fsl/jr.c: In function 'sec_init_idx':
drivers/crypto/fsl/jr.c:625:6: warning: unused variable 'i' [-Wunused-variable]
  int i;
      ^

Thanks for submitting this patch set.

I couldn't get encrypted boot working in my first attempt, doing the
exact same procedure with commit 22191ac35344 ("drivers/crypto/fsl:
assign job-rings to non-TrustZone") reverted works fine.

I will take a better look in your patch set and let you know if I find
something.

Best Regards,
Breno Matheus Lima

[U-Boot] [PATCH 1/4] crypto/fsl: Introduce API to save/restore job-ring context

[Breno Matheus Lima]

Hi Bryan,

Em ter, 23 de abr de 2019 às 07:20, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> escreveu:
>
> We need to handle the case where DEK blobs are passed to the BootROM. In
> this case, unlike in HAB authentication the BootROM checks job-ring
> ownership set to secure world.
>
> One possible solution is to set the job-ring ownership to the expected
> state for DEK blobs and then restore to whatever the run-time wants.
>
> For the case where Linux runs in normal-world we would want to set the
> job-ring ownership to normal-world.
>
> The first step in the ownership context switch dance is making an API to do
> it.
>
> This patch introduces:
>
> void __weak sec_set_jr_context_secure(void);
> void __weak sec_set_jr_context_normal(void);
>
> This can be over-ridden for a given architecture, as will be necessary for
> the MPC85xxx
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/crypto/fsl/jr.c | 38 ++++++++++++++++++++++++++++++++++++++
>  include/fsl_sec.h       |  3 +++
>  2 files changed, 41 insertions(+)
>
> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
> index cc8d3b02a5..7b13aa4a61 100644
> --- a/drivers/crypto/fsl/jr.c
> +++ b/drivers/crypto/fsl/jr.c
> @@ -574,6 +574,44 @@ static int rng_init(uint8_t sec_idx)
>         return ret;
>  }
>  #endif
> +
> +static void __sec_set_jr_context_secure(uint8_t sec_idx)
> +{
> +       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> +       uint32_t jrown_ns;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(sec->jrliodnr); i++) {
> +               jrown_ns = sec_in32(&sec->jrliodnr[i].ms);
> +               jrown_ns &= ~(JROWN_NS | JRMID_NS);

We have the following definition at drivers/crypto/fsl/jr.h:

#define JRMID_NS 0x00000001

Seems that we are setting JROWN_MID field which is not TrustZone
related, from i.MX7D Security Reference Manual:

Job Ring Owner's MID. This field defines the MID of the bus master
that is permitted to read or write the registers that are specific to
a particular Job Ring. These registers include the job ring
configuration registers, the interrupt registers, the CAAM Secure
Memory Access Permissions and Secure Memory Access Group registers and
the ring buffer registers.

Could you please double check?

Thanks,
Breno Lima

[U-Boot] [PATCH 2/4] crypto/fsl: Use __sec_set_jr_context_normal

[Bryan O'Donoghue]

On 25/04/2019 04:24, Breno Matheus Lima wrote:
> I couldn't get encrypted boot working in my first attempt, doing the
> exact same procedure with commit 22191ac35344 ("drivers/crypto/fsl:
> assign job-rings to non-TrustZone") reverted works fine.

Hi Breno,

I noticed another patch from you re: dek blob, does that address this 
issue for you are is this still a live thing ?

If you are running in secure-world, and the BootROM dek blob stuff 
validates job-ring ownership it _should_ be possible to flip the 
ownership bits to what the BootROM expects and then back again.

If its not working, presumably its because we aren't flipping ownership 
at the right time.

Maybe better to set permissions to secure-world while we are in u-boot 
and then switch to normal world before we hand over to the next boot phase.

---
bod

[U-Boot] [PATCH 2/4] crypto/fsl: Use __sec_set_jr_context_normal

[Bryan O'Donoghue]

On 30/04/2019 02:28, Bryan O'Donoghue wrote:
> 
> 
> On 25/04/2019 04:24, Breno Matheus Lima wrote:
>> I couldn't get encrypted boot working in my first attempt, doing the
>> exact same procedure with commit 22191ac35344 ("drivers/crypto/fsl:
>> assign job-rings to non-TrustZone") reverted works fine.
> 
> Hi Breno,
> 
> I noticed another patch from you re: dek blob, does that address this 
> issue for you are is this still a live thing ?
> 
> If you are running in secure-world, and the BootROM dek blob stuff 
> validates job-ring ownership it _should_ be possible to flip the 
> ownership bits to what the BootROM expects and then back again.
> 
> If its not working, presumably its because we aren't flipping ownership 
> at the right time.

It occurred to me after I went to bed.

The right thing to do is leave the BootROM settings up until we hand-off 
and then set the required post-boot settings.

Something I reckon can be ~easily done in some sort of architectural 
handover preparation function.

I'll spin that patchset.

---
bod

[U-Boot] [PATCH 1/4] crypto/fsl: Introduce API to save/restore job-ring context

[Bryan O'Donoghue]

On 25/04/2019 23:13, Breno Matheus Lima wrote:
> Hi Bryan,
> 
> Em ter, 23 de abr de 2019 às 07:20, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> escreveu:
>>
>> We need to handle the case where DEK blobs are passed to the BootROM. In
>> this case, unlike in HAB authentication the BootROM checks job-ring
>> ownership set to secure world.
>>
>> One possible solution is to set the job-ring ownership to the expected
>> state for DEK blobs and then restore to whatever the run-time wants.
>>
>> For the case where Linux runs in normal-world we would want to set the
>> job-ring ownership to normal-world.
>>
>> The first step in the ownership context switch dance is making an API to do
>> it.
>>
>> This patch introduces:
>>
>> void __weak sec_set_jr_context_secure(void);
>> void __weak sec_set_jr_context_normal(void);
>>
>> This can be over-ridden for a given architecture, as will be necessary for
>> the MPC85xxx
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   drivers/crypto/fsl/jr.c | 38 ++++++++++++++++++++++++++++++++++++++
>>   include/fsl_sec.h       |  3 +++
>>   2 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
>> index cc8d3b02a5..7b13aa4a61 100644
>> --- a/drivers/crypto/fsl/jr.c
>> +++ b/drivers/crypto/fsl/jr.c
>> @@ -574,6 +574,44 @@ static int rng_init(uint8_t sec_idx)
>>          return ret;
>>   }
>>   #endif
>> +
>> +static void __sec_set_jr_context_secure(uint8_t sec_idx)
>> +{
>> +       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
>> +       uint32_t jrown_ns;
>> +       int i;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(sec->jrliodnr); i++) {
>> +               jrown_ns = sec_in32(&sec->jrliodnr[i].ms);
>> +               jrown_ns &= ~(JROWN_NS | JRMID_NS);
> 
> We have the following definition at drivers/crypto/fsl/jr.h:
> 
> #define JRMID_NS 0x00000001
> 
> Seems that we are setting JROWN_MID field which is not TrustZone
> related, from i.MX7D Security Reference Manual:
> 
> Job Ring Owner's MID. This field defines the MID of the bus master
> that is permitted to read or write the registers that are specific to
> a particular Job Ring. These registers include the job ring
> configuration registers, the interrupt registers, the CAAM Secure
> Memory Access Permissions and Secure Memory Access Group registers and
> the ring buffer registers.

Hrmm, just seeing your response now Breno.

What we have is:
include/fsl_sec.h:#define JR_MID    2    /* Matches ROM configuration */

There's a decent argument to read what the BootROM has set for JR_MID 
and write it back ...

Let me include that in v2.

---
bod

[U-Boot] [PATCH 2/4] crypto/fsl: Use __sec_set_jr_context_normal

[Breno Matheus Lima]

Hi Bryan,

Em ter, 30 de abr de 2019 às 05:13, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> escreveu:
>
>
>
> On 30/04/2019 02:28, Bryan O'Donoghue wrote:
> >
> >
> > On 25/04/2019 04:24, Breno Matheus Lima wrote:
> >> I couldn't get encrypted boot working in my first attempt, doing the
> >> exact same procedure with commit 22191ac35344 ("drivers/crypto/fsl:
> >> assign job-rings to non-TrustZone") reverted works fine.
> >
> > Hi Breno,
> >
> > I noticed another patch from you re: dek blob, does that address this
> > issue for you are is this still a live thing ?

No, the patch I have recently submitted does not address the JR
TrustZone issue we are currently seeing with DEK blob decapsulation at
ROM level. I was not following AN12056 when I tried so I couldn't see
this other issue at first moment.

> >
> > If you are running in secure-world, and the BootROM dek blob stuff
> > validates job-ring ownership it _should_ be possible to flip the
> > ownership bits to what the BootROM expects and then back again.
> >
> > If its not working, presumably its because we aren't flipping ownership
> > at the right time.
>
> It occurred to me after I went to bed.
>
> The right thing to do is leave the BootROM settings up until we hand-off
> and then set the required post-boot settings.
>
> Something I reckon can be ~easily done in some sort of architectural
> handover preparation function.
>
> I'll spin that patchset.

Thanks for preparing a second version for this patchset, I see that
you have also replied to my other e-mail in "[PATCH 1/4] crypto/fsl:
Introduce API to save/restore job-ring context".

Your new proposal looks fine to me, I can test again.

Thanks,
Breno Lima