[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Breno Matheus Lima]

Commit 22191ac35344 ("drivers/crypto/fsl: assign job-rings to
 non-TrustZone") breaks HABv4 encrypted boot support in the
following i.MX devices:

- i.MX6UL
- i.MX7S
- i.MX7D
- i.MX7ULP

For preparing a HABv4 encrypted boot image it's necessary to
encapsulate the generated DEK in a blob. In devices listed
above the blob generation function takes into consideration
the Job Ring TrustZone ownership configuration (JROWN_NS)
and can be only decapsulated by the same configuration.

The ROM code expects DEK blobs encapsulated by the Secure World
environments which commonly have JROWN_NS = 0.

As U-Boot is running in Secure World we must have JROWN_NS = 0
so the blobs generated by dek_blob tool can be decapsulated
by the ROM code.

Linux Kernel is booting by default in TrustZone Secure World in
most of targets. Make job-rings assignment to non-Secure dependent
of CONFIG_OPTEE and CONFIG_ARMV7_BOOT_SEC_DEFAULT to avoid a Kernel
crash when booting Linux in non-Secure World.

OP-TEE users can still use dek_blob command as job ring assignment
is also dependent of CONFIG_CMD_DEKBLOB configuration.

Signed-off-by: Breno Lima <breno.lima@nxp.com>
---
 drivers/crypto/fsl/jr.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index cc8d3b02a5..23d5a64da0 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -635,11 +635,15 @@ 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);
+	if ((IS_ENABLED(CONFIG_OPTEE) ||
+	     !IS_ENABLED(CONFIG_ARMV7_BOOT_SEC_DEFAULT)) &&
+	     !IS_ENABLED(CONFIG_CMD_DEKBLOB)) {
+		/* Set ownership of job rings to  non-TrustZone mode. */
+		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);
-- 
2.17.1

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Bryan O'Donoghue]

On 05/04/2019 17:16, Breno Matheus Lima wrote:
> +	if ((IS_ENABLED(CONFIG_OPTEE) ||
> +	     !IS_ENABLED(CONFIG_ARMV7_BOOT_SEC_DEFAULT)) &&
> +	     !IS_ENABLED(CONFIG_CMD_DEKBLOB)) {

So.

How does this patch work if you want to do HABv4 encrypted 
authentication of an OP-TEE image prior to booting it ?

i.e.

You'll switch the job-ring ownership over to non-trusted world, call HAB 
and the BootROM will fail to authenticate.

How about we improve the hab driver in u-boot ?

1. I notice somebody has already added a save_gd()/restore_gd() pair
2. How about a save_jr()/restore_jr() pair and
3. a bootrom_jr_setup() ?

In pseudocode then

do_hab_auth()
{
	save_jr();		// save current job-ring context
	bootrom_jr_setup();	// set job-ring to bootrom mode
	hab_rvt_authenticate();
	restore_jr();		// restore previous job-ring context
}

That gets us out of the nasty business of

1. Constraining the use case of CONFIG_OPTEE=y &&
    authenticate(encrypted-dek-blob);

2. Pushing changes into upstream OP-TEE for job-ring assignment

IMV - we should save and restore the job ring context so that all of 
this stuff will "just work"

That should work for

BootROM -> u-boot -> HABv4 auth(encryptedblob) -> Linux

and

BootROM -> u-boot -> run optee -> HABv4 auth(encrypted blob) -> Linux

Basically you've described and additional dependency the BootROM has, so 
lets just "switch context" prior to calling into the BootROM and restore 
to a default non-secure job-ring assignment after.

---
bod

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Bryan O'Donoghue]

On 06/04/2019 16:21, Bryan O'Donoghue wrote:
> 
> 1. I notice somebody has already added a save_gd()/restore_gd() pair

I'm referring to save_gd()/restore_gd() here : 
https://source.codeaurora.org/external/imx/uboot-imx/tree/arch/arm/mach-imx/hab.c?h=imx_v2018.03_4.14.78_1.0.0_ga

but... it's still the right idea for this problem.

Save context
Switch to bootrom mode
Restore context

Job done

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Breno Matheus Lima]

Hi Bryan,

Em sáb, 6 de abr de 2019 às 12:21, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> escreveu:
>
>
>
> On 05/04/2019 17:16, Breno Matheus Lima wrote:
> > +     if ((IS_ENABLED(CONFIG_OPTEE) ||
> > +          !IS_ENABLED(CONFIG_ARMV7_BOOT_SEC_DEFAULT)) &&
> > +          !IS_ENABLED(CONFIG_CMD_DEKBLOB)) {
>
> So.
>
> How does this patch work if you want to do HABv4 encrypted
> authentication of an OP-TEE image prior to booting it ?

Thanks for your comments and for pointing this out, I agree that this
can be an issue for OP-TEE users that may want to decrypt additional
boot images at U-Boot level.

The main issue is that all users are currently impacted in first
authentication/decryption executed at BootROM level:

BootROM -> HABv4 -> U-Boot

We may need to come up with a solution suitable for all use cases.

>
> i.e.
>
> You'll switch the job-ring ownership over to non-trusted world, call HAB
> and the BootROM will fail to authenticate.
>
> How about we improve the hab driver in u-boot ?
>
> 1. I notice somebody has already added a save_gd()/restore_gd() pair
> 2. How about a save_jr()/restore_jr() pair and
> 3. a bootrom_jr_setup() ?
>
> In pseudocode then
>
> do_hab_auth()
> {
>         save_jr();              // save current job-ring context
>         bootrom_jr_setup();     // set job-ring to bootrom mode
>         hab_rvt_authenticate();
>         restore_jr();           // restore previous job-ring context
> }
>
> That gets us out of the nasty business of
>
> 1. Constraining the use case of CONFIG_OPTEE=y &&
>     authenticate(encrypted-dek-blob);
>
> 2. Pushing changes into upstream OP-TEE for job-ring assignment
>
> IMV - we should save and restore the job ring context so that all of
> this stuff will "just work"
>
> That should work for
>
> BootROM -> u-boot -> HABv4 auth(encryptedblob) -> Linux
>
> and
>
> BootROM -> u-boot -> run optee -> HABv4 auth(encrypted blob) -> Linux
>
> Basically you've described and additional dependency the BootROM has, so
> lets just "switch context" prior to calling into the BootROM and restore
> to a default non-secure job-ring assignment after.

Yes, this can work for OP-TEE users decrypting additional boot images
at U-Boot level, however all users won't be able to
authenticate/decrypt the initial boot image at BootROM level. Out of
reset CAAM job rings are assigned to TrustZone secure world and
BootROM code is expecting blobs generated by the same environment.

What about moving the job rings assignment to OP-TEE level? Something
similar as we currently have in imx-optee-os project?

https://source.codeaurora.org/external/imx/imx-optee-os/tree/core/drivers/caam/hal/imx_6_7/hal_jr.c?h=imx_4.14.78_1.0.0_ga#n31

U-Boot is running in TrustZone secure world in most of targets, in my
opinion it makes sense to have at least 1 job ring assigned to
TrustZone secure world.

Best regards,
Breno Lima

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Bryan O'Donoghue]

On 06/04/2019 22:41, Breno Matheus Lima wrote:
> Hi Bryan,
> 
> Em sáb, 6 de abr de 2019 às 12:21, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> escreveu:
>>
>>
>>
>> On 05/04/2019 17:16, Breno Matheus Lima wrote:
>> Basically you've described and additional dependency the BootROM has, so
>> lets just "switch context" prior to calling into the BootROM and restore
>> to a default non-secure job-ring assignment after.
> 
> Yes, this can work for OP-TEE users decrypting additional boot images
> at U-Boot level, however all users won't be able to
> authenticate/decrypt the initial boot image at BootROM level.

I don't understand that comment, perhaps you can give an example.

> Out of
> reset CAAM job rings are assigned to TrustZone secure world and
> BootROM code is expecting blobs generated by the same environment.
> 
> What about moving the job rings assignment to OP-TEE level? Something
> similar as we currently have in imx-optee-os project?

TBH, I see that as something that should be done _anyway_ not instead 
i.e. after u-boot, if you want to do 'stuff' with the CAAM you are either

1. Running in non-secure Linux, in which case you need the job-rings
    assigned to non-secure mode or

2. You are running inside of a TEE, in which case you absolutely need
    to have at least one job-ring

... and for the second case the right thing to do is to arbitrate 
ownership of job-rings via a DTB

> https://source.codeaurora.org/external/imx/imx-optee-os/tree/core/drivers/caam/hal/imx_6_7/hal_jr.c?h=imx_4.14.78_1.0.0_ga#n31
> 
> U-Boot is running in TrustZone secure world in most of targets, in my
> opinion it makes sense to have at least 1 job ring assigned to
> TrustZone secure world.

But if u-boot is running in secure-world

save_jr_context();
setup_some_new_jr_context();
hab_authenticate_something();
restore_jr_context();

As a "quick fix", that's the way I'd do it. Just pivoting on 
CONFIG_OPTEE is pretty easy to break i.e. you can have CONFIG_OPTEE 
defined in your u-boot config but not actually be executing a TEE, in 
which case by the time you boot Linux your JR assignment is wrong..

The correct and flexible fix is passing a DTB descriptor that u-boot, 
OPTEE and Kernel can put data into so that there's a canonical 
description of which execution environment owns what.

---
bod

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Breno Matheus Lima]

Hi Bryan,

Em dom, 7 de abr de 2019 às 05:05, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> escreveu:
>
>
>
> On 06/04/2019 22:41, Breno Matheus Lima wrote:
> > Hi Bryan,
> >
> > Em sáb, 6 de abr de 2019 às 12:21, Bryan O'Donoghue
> > <bryan.odonoghue@linaro.org> escreveu:
> >>
> >>
> >>
> >> On 05/04/2019 17:16, Breno Matheus Lima wrote:
> >> Basically you've described and additional dependency the BootROM has, so
> >> lets just "switch context" prior to calling into the BootROM and restore
> >> to a default non-secure job-ring assignment after.
> >
> > Yes, this can work for OP-TEE users decrypting additional boot images
> > at U-Boot level, however all users won't be able to
> > authenticate/decrypt the initial boot image at BootROM level.
>
> I don't understand that comment, perhaps you can give an example.
>

You can take a look in application note AN12056:
https://www.nxp.com/docs/en/application-note/AN12056.pdf

Section 2.2 provides more details on the encrypted boot sequence, as
you can see in Figure 1 the U-Boot image is decrypted at bootROM level
after a system reset. Section 4 explains how to encrypt and sign an
U-Boot image.

As I mentioned in commit log, the ROM code expects DEK blobs
encapsulated by the Secure World environments which commonly have
JROWN_NS = 0.

> > Out of
> > reset CAAM job rings are assigned to TrustZone secure world and
> > BootROM code is expecting blobs generated by the same environment.
> >
> > What about moving the job rings assignment to OP-TEE level? Something
> > similar as we currently have in imx-optee-os project?
>
> TBH, I see that as something that should be done _anyway_ not instead
> i.e. after u-boot, if you want to do 'stuff' with the CAAM you are either
>
> 1. Running in non-secure Linux, in which case you need the job-rings
>     assigned to non-secure mode or
>
> 2. You are running inside of a TEE, in which case you absolutely need
>     to have at least one job-ring
>
> ... and for the second case the right thing to do is to arbitrate
> ownership of job-rings via a DTB
>
> > https://source.codeaurora.org/external/imx/imx-optee-os/tree/core/drivers/caam/hal/imx_6_7/hal_jr.c?h=imx_4.14.78_1.0.0_ga#n31
> >
> > U-Boot is running in TrustZone secure world in most of targets, in my
> > opinion it makes sense to have at least 1 job ring assigned to
> > TrustZone secure world.
>
> But if u-boot is running in secure-world
>
> save_jr_context();
> setup_some_new_jr_context();
> hab_authenticate_something();
> restore_jr_context();

This can only work if we do similar operation in CMD_DEKBLOB:

save_jr_context();
setup_some_new_jr_context();
blob_dek()
restore_jr_context();

Both operations blob_dek() and hab_authenticate_image() at U-Boot
level must have the Job Ring assigned for TrustZone secure world. The
first authentication/decryption at bootROM level is expecting a DEK
blobs generated by TrustZone secure world.

>
> As a "quick fix", that's the way I'd do it. Just pivoting on
> CONFIG_OPTEE is pretty easy to break i.e. you can have CONFIG_OPTEE
> defined in your u-boot config but not actually be executing a TEE, in
> which case by the time you boot Linux your JR assignment is wrong..

Can you please provide more details on how this can break users that
has CONFIG_OPTEE defined and are not executing a TEE? From my
understanding Linux Kernel will be running in TZ secure world and JRs
assigned to TZ non-secure world, CAAM driver can still be used on this
condition (Similar as we currently have for mx7dsabresd target).

In order to have a quick fix available, what about delaying the Job
Ring assignment in U-Boot?

Perhaps we can provide an U-Boot command to set the Job Ring
ownership, users can add this command in their boot script just before
booting Kernel and/or OP-TEE.

>
> The correct and flexible fix is passing a DTB descriptor that u-boot,
> OPTEE and Kernel can put data into so that there's a canonical
> description of which execution environment owns what.
>

Yes, I agree. We need a more flexible fix here.

Best regards,
Breno Lima

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Bryan O'Donoghue]

On 07/04/2019 19:56, Breno Matheus Lima wrote:
> Hi Bryan,
> 
> Em dom, 7 de abr de 2019 às 05:05, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> escreveu:
>>
>>
>>
>> On 06/04/2019 22:41, Breno Matheus Lima wrote:
>> save_jr_context();
>> setup_some_new_jr_context();
>> hab_authenticate_something();
>> restore_jr_context();
> 
> This can only work if we do similar operation in CMD_DEKBLOB:
> 
> save_jr_context();
> setup_some_new_jr_context();
> blob_dek()
> restore_jr_context();
> 
> Both operations blob_dek() and hab_authenticate_image() at U-Boot
> level must have the Job Ring assigned for TrustZone secure world. The
> first authentication/decryption at bootROM level is expecting a DEK
> blobs generated by TrustZone secure world.

Ah right, yes good point, I wasn't entirely following you on the DEK bit 
of it.

Yes we would need to save and restore context for both cases i.e. any 
time we call into the BootROM and the BootROM wants to perform CAAM 
operations as a result.

>> As a "quick fix", that's the way I'd do it. Just pivoting on
>> CONFIG_OPTEE is pretty easy to break i.e. you can have CONFIG_OPTEE
>> defined in your u-boot config but not actually be executing a TEE, in
>> which case by the time you boot Linux your JR assignment is wrong..
> 
> Can you please provide more details on how this can break users that
> has CONFIG_OPTEE defined and are not executing a TEE? 

My point here is you can't presuppose how a boot flow will work simply 
due to a CONFIG option.

And indeed if you run an NXP TEE with a CAAM driver you still have the 
problem that the initial jr ownership defined by u-boot is now wrong by 
the time the kernel runs.

> From my
> understanding Linux Kernel will be running in TZ secure world and JRs
> assigned to TZ non-secure world, CAAM driver can still be used on this
> condition (Similar as we currently have for mx7dsabresd target).

Linux will be running in _normal_ world. A TEE will be running in secure 
world.

Then again a TEE might not be involved.

As soon as you guys want to land your CAAM driver in upstream OP-TEE I 
think a conversation needs to be had re: assignment of job-ring ownership.

Basically if your TEE wants a CAAM job-ring, that's fine but, that 
allocation needs to be stuffed into a DTB the kernel can access.


> In order to have a quick fix available, what about delaying the Job
> Ring assignment in U-Boot?
> 
> Perhaps we can provide an U-Boot command to set the Job Ring
> ownership, users can add this command in their boot script just before
> booting Kernel and/or OP-TEE.

Technically it could work but, how is it a better solution to have all 
users of the CAAM have to modify their boot flow, than put a context 
save/restore wrapper into hab_auth() and blob_dek() that hides the detail ?

> 
>>
>> The correct and flexible fix is passing a DTB descriptor that u-boot,
>> OPTEE and Kernel can put data into so that there's a canonical
>> description of which execution environment owns what.
>>
> 
> Yes, I agree. We need a more flexible fix here.

DTB is the way to go.

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Fabio Estevam]

Hi Bryan,

On Mon, Apr 8, 2019 at 5:10 AM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:

> DTB is the way to go.

Could you please submit a patch that fixes the regression?

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Bryan O'Donoghue]

On 08/04/2019 13:58, Fabio Estevam wrote:
> Hi Bryan,
> 
> On Mon, Apr 8, 2019 at 5:10 AM Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
> 
>> DTB is the way to go.
> 
> Could you please submit a patch that fixes the regression?
> 

I can publish something soon, sure.

[U-Boot] [PATCH] crypto: fsl: jr: Make job-rings assignment non-Secure dependent

[Breno Matheus Lima]

Hi Bryan,

Seems that my last email didn't get in U-Boot mailing list, I'm sending again.

Em sáb, 6 de abr de 2019 às 12:21, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> escreveu:
>
>
>
> On 05/04/2019 17:16, Breno Matheus Lima wrote:
> > +     if ((IS_ENABLED(CONFIG_OPTEE) ||
> > +          !IS_ENABLED(CONFIG_ARMV7_BOOT_SEC_DEFAULT)) &&
> > +          !IS_ENABLED(CONFIG_CMD_DEKBLOB)) {
>
> So.
>
> How does this patch work if you want to do HABv4 encrypted
> authentication of an OP-TEE image prior to booting it ?

Thanks for your comments and for pointing this out, I agree that this
can be an issue for OP-TEE users that may want to decrypt additional
boot images at U-Boot level.

The main issue is that all users are currently impacted in first
authentication/decryption executed at BootROM level:

BootROM -> HABv4 -> U-Boot

We may need to come up with a solution suitable for all use cases

>
> i.e.
>
> You'll switch the job-ring ownership over to non-trusted world, call HAB
> and the BootROM will fail to authenticate.
>
> How about we improve the hab driver in u-boot ?
>
> 1. I notice somebody has already added a save_gd()/restore_gd() pair
> 2. How about a save_jr()/restore_jr() pair and
> 3. a bootrom_jr_setup() ?
>
> In pseudocode then
>
> do_hab_auth()
> {
>         save_jr();              // save current job-ring context
>         bootrom_jr_setup();     // set job-ring to bootrom mode
>         hab_rvt_authenticate();
>         restore_jr();           // restore previous job-ring context
> }
>
> That gets us out of the nasty business of
>
> 1. Constraining the use case of CONFIG_OPTEE=y &&
>     authenticate(encrypted-dek-blob);
>
> 2. Pushing changes into upstream OP-TEE for job-ring assignment
>
> IMV - we should save and restore the job ring context so that all of
> this stuff will "just work"
>
> That should work for
>
> BootROM -> u-boot -> HABv4 auth(encryptedblob) -> Linux
>
> and
>
> BootROM -> u-boot -> run optee -> HABv4 auth(encrypted blob) -> Linux
>
> Basically you've described and additional dependency the BootROM has, so
> lets just "switch context" prior to calling into the BootROM and restore
> to a default non-secure job-ring assignment after.

Yes, this can work for OP-TEE users decrypting additional boot images
at U-Boot level, however all users won't be able to
authenticate/decrypt the initial boot image at BootROM level. Out of
reset CAAM job rings are assigned to TrustZone secure world and
BootROM code is expecting blobs generated by the same environment.

What about moving the job rings assignment to OP-TEE level? Something
similar as we currently have in imx-optee-os project?

https://source.codeaurora.org/external/imx/imx-optee-os/tree/core/drivers/caam/hal/imx_6_7/hal_jr.c?h=imx_4.14.78_1.0.0_ga#n31

U-Boot is running in TrustZone secure world in most of targets, in my
opinion it makes sense to have at least 1 job ring assigned to
TrustZone secure world.

Best regards,
Breno Lima