Minimal HUK implementation without full CAAM driver by Emantor · Pull Request #3160 · OP-TEE/optee_os

Emantor · 2019-07-31T04:51:01Z

This PR implements a minimal implementation to retrieve a Master Key Verification Blob (MKVB) from the CAAM inside of i.MX processors for use as a Hardware Unique Key (HUK). In contrast to the full CAAM implementation offered by NXP, this only implements the necessary bits to retrieve MKVB once, does not allocate job rings to the secure world and has no conflicts with the linux kernel CAAM driver, since the CAAM is not accessed after the MKVB is retrieved.
Things I'm not sure about:

Access through the structs to the registers:
I followed the style implemented by @bryanodonoghue, it may be more readable to switch to a normal #define based register access model

Related:
#2892 - initial i.MX HUK issue
#3149 - full NXP CAAM driver

jforissier

Reviewing "imx: enable CAAM clocks before accessing registers"

jforissier

Minor comments on "imx_caam: implement tee_otp_get_hw_unique_key".

Please add parentheses to function name in commit subject: tee_otp_get_hw_unique_key()

Emantor · 2019-07-31T12:11:48Z

I have a rebased version against the recent master commits and will squash the review commits and suggested changes from @jforissier at the same time on the next push.

etienne-lms

minor comments for "imx: enable CAAM clocks before accessing registers".

etienne-lms

Acked-by: Etienne Carriere <[email protected]> with minor comment.

etienne-lms

comment for commit "plat-imx: register definitions for CAAM".

etienne-lms

comments on commit "imx_caam: implement tee_otp_get_hw_unique_key"

etienne-lms

For commit "imx_caam: add functions for jr init and reset":
Can remove following sentence from commit message: "Will be used by the tee_otp_get_hw_unique key implementation in the next commit."

The CAAM clocks need to be enabled, otherwise access to the CAAM might result in a bus stall. Signed-off-by: Rouven Czerwinski <[email protected]>

Add __attribute__((packed)) to the CAAM struct definitions to ensure the compiler does not insert padding into the structures. Signed-off-by: Rouven Czerwinski <[email protected]>

Register Definitions in the same style as used by the jobring allocation code, by extending the structure definitions and inserting padding if the registers are undocumented. Signed-off-by: Rouven Czerwinski <[email protected]>

Minimal implementation for tee_otp_get_hw_unique_key using the Master Key Verification Blob (MKVB) produced by the CAAM. Only the first 16 bytes are copied into the hw unique key structure, since the MKVB is 32 bytes long. Signed-off-by: Rouven Czerwinski <[email protected]>

Emantor · 2019-08-01T13:21:05Z

I have rebased against latest master and changed to stack allocation. With the alignement within the structure to 64 bytes we should always flush and invalidate the intended date, without hitting stack data above or below. I also tried to incorporated all of the review comments, Thanks @jforissier, @etienne-lms, @jenswi-linaro & @bryanodonoghue. @etienne-lms I did not apply your Ack since I think the code has changed enough to require another review.

etienne-lms

for commits "plat-imx: pack caam structs", imx: enable CAAM clocks before accessing registers:

Acked-by: Etienne Carriere <[email protected]>.

for commit "imx_caam: implement tee_otp_get_hw_unique_key": see comments.

@etienne-lms

Address the review comments from @etienne-lms. Signed-off-by: Rouven Czerwinski <[email protected]>

cneveux · 2019-08-02T09:13:57Z

@Emantor

I understand that you want to have the HUK generated with the CAAM but I don't see the interest of bypassing all the work we, NXP, are doing to enable the CAAM driver. In your PR you have to enable the CAAM anyway to use the Job Ring. In plus you have missed a condition that is really needed to be full secure. This is the selection of the master key.

In the driver we want to add and that has been improved and validated on all i.MX devices (taking care of all boot modes). We are doing the same in a more generic way.

Could you explain us what is the added value of your implementation that is a kind of copy of what has been submitted in the PR #3149?

Emantor · 2019-08-02T09:45:40Z

I understand that you want to have the HUK generated with the CAAM but I don't see the interest of bypassing all the work we, NXP, are doing to enable the CAAM driver. In your PR you have to enable the CAAM anyway to use the Job Ring.

I am interested in generating the MKVB for use as a HUK. I am not interested in a CAAM driver implementation, since I don't see the use case for it. The CAAM driver adds a lot of code which increases the TCB for my secure OS. For our use cases a full CAAM driver within OP-TEE is not needed, since we don't require hardware acceleration for the crypto primitives.

In plus you have missed a condition that is really needed to be full secure. This is the selection of the master key.

According to the documentation at hand, the OTPMK is used as the default Master Key CAAM input.
I am well aware that the OTPMK is not available on not HAB-enabled devices and a test key is used.
I'd argue that it does not make much sense to deploy devices without HAB and with OP-TEE.

In the driver we want to add and that has been improved and validated on all i.MX devices (taking care of all boot modes). We are doing the same in a more generic way.
Which is also currently incompatible to the mainline kernel due to possible clock problems. If somebody i.e. decides to add power management to the Linux CAAM driver, OP-TEE will be stuck when trying to access the CAAM. I agree that a solution to the problem exists which is to move the clocks into the secure world. However this requires discussions with the upstream kernel maintainers on how this implementation is going to work.
Could you explain us what is the added value of your implementation that is a kind of copy of what has been submitted in the PR #3149?

It adds the required minimum amount of code to derive a HUK for OP-TEE on bootup without requiring a switch of the full crypto implementations to a hardware devices.
It allows usage of all CAAM jobrings within Linux
The CAAM is not touched after OP-TEE initialization, which eliminates all problems stemming from OP-TEE requiring the CAAM clock to stay enabled.
This code can later be refactored to take advantage of the NXP implementations provided in the full CAAM driver.

ricardosalveti · 2019-08-02T15:46:40Z

I also share similar concerns and needs with @Emantor, so I'm also interested at having just the minimum to derive a HUK instead of having the full CAAM support in place.

Emantor · 2019-08-02T15:49:53Z

@dmcilvaney this may also be relevant to your interests for the Microsoft IOT devices.

cneveux · 2019-08-05T06:14:23Z

@ricardosalveti, @Emantor
Well, thinks is that your are not taking care of all devices and all devices boot mode.
You can not hardcode the number of Job Rings, number is function of device type. You can not assume that JR#0 is always available for the OPTEE at boot time. This is function of the boot mode if there is a previous SW doing the authentication of booting OSes first.
Next we (NXP) are doing the implementation of the CAAM driver to satisfy everybody. This driver is working since long time and this is why we are upstreaming it now. The driver is not so complex and no heavy, the driver is doing the minimum if you don't enable the other crypto algorithm.
What @Emantor did, is just copying the work I submitted and re-write it. If you want to keep this version feel free to keep in your own source tree but please let us push our work that is expected by many customers (directly or not).

Thanks for you understanding.

Emantor · 2019-08-05T07:42:36Z

@ricardosalveti, @Emantor
Well, thinks is that your are not taking care of all devices and all devices boot mode.
You can not hardcode the number of Job Rings, number is function of device type. You can not assume that JR#0 is always available for the OPTEE at boot time. This is function of the boot mode if there is a previous SW doing the authentication of booting OSes first.

I tried to find some documentation on which job ring is actually used by the HAB, but all I could find were discussion on the LKML and U-Boot. Do you have more documentation internally that you could share?

To be clear: this HUK patch breaks the case where your boot chain is SPL->OP-TEE->U-Boot->kernel and U-Boot tries to use HAB to authenticate additional images. Are there similar restrictions on the other job rings? Is the simple solution to switch to job ring 2?

This is a bug in this PR and should consequently be fixed.

Next we (NXP) are doing the implementation of the CAAM driver to satisfy everybody. This driver is working since long time and this is why we are upstreaming it now. The driver is not so complex and no heavy, the driver is doing the minimum if you don't enable the other crypto algorithm.

With a diffstat of +13,804 −62 for the current CAAM HUK patch set I'd call this a lot more complex :-). Of course not all code paths are used, but getting the whole PR reviewed takes time. As an alternative for now I posted this PR.
It also requires currently undocumented device tree changes, i.e. secure-status: okay for the CAAM ctrl node and a job ring.
And your PR notes that it currently can't be used without other PRs which have not been posted yet and makes it quite hard to judge whether the HUK derivation works correctly. IMO the HUK PR can only be reviewed in conjunction with the SNVS patches.

What @Emantor did, is just copying the work I submitted and re-write it. If you want to keep this version feel free to keep in your own source tree but please let us push our work that is expected by many customers (directly or not).

I'm not restricting you ability to push your changes upstream in any way. I'm just proposing a solution for now which is easier to review and test and has no immediate impact on the kernel.

Emantor · 2019-08-05T10:37:01Z

@MrVan any comments on this pull request? This requires your Reviewed-by because of the plat-imx changes.

In order to allow a SPL -> OP-TEE -> U-Boot -> HAB authenticate -> Linux boot flow, use job ring two instead of one, since one might be used by the HAB. Signed-off-by: Rouven Czerwinski <[email protected]>

Emantor · 2019-08-06T09:12:24Z

I pushed a patch which switches to using job ring two, which is the same ring used as by the full caam implementation for the CFG_CAAM_DT=n case.

@etienne-lms

Address review comments from @etienne-lms. Signed-off-by: Rouven Czerwinski <[email protected]>

Handle job ring reset timeouts by passing the return value. Signed-off-by: Rouven Czerwinski <[email protected]>

Those were forgotten in the previous commits, fix them now. Signed-off-by: Rouven Czerwinski <[email protected]>

etienne-lms

comments for the 3 last fixup commits

@etienne-lms

Fixes the leftover review comments from @etienne-lms. Signed-off-by: Rouven Czerwinski <[email protected]>

Emantor · 2019-09-05T13:51:59Z

@etienne-lms, @clementfaure any comments on this? This is a valid minimal implementation which does not require the whole CAAM stack as implemented by NXP, but can be refactored as the necessary NXP commits are added.

etienne-lms · 2019-09-06T07:12:22Z

@Emantor, #3228 is not far from landing. Hence maybe consider refactoring on the P-R to plug HUK over the proposed CAAM support. Should find a way to use only features you need from it.

ricardosalveti · 2019-11-04T17:16:18Z

+	if (mkvb.jr.outring[0].status != 0)
+		ret = TEE_ERROR_SECURITY;
+		goto out;
+


Missing { } otherwise it always ends up going to 'goto out'.

Yes, this is already addressed in my local branch. I plan to rebase onto the CAAM work, but that might take a bit of refactoring to correctly disable the CAAM driver after HUK generation.

Great, thanks for the heads up.

etienne-lms · 2019-11-08T06:42:27Z

+	uint32_t reg_val = 0;
+	uint32_t timeout = 1000;
+
+	io_write32((vaddr_t)&ctrl->jrcfg[MKVB_JR].jrcr, 1);


It would be nice that raw value (1 here) at lines 50, 55, 59, ... are replaced with macros, i.e. CAAM_JRCR_xxx BIT(0), CAAM_JRINTR_STATE_MASK GENMASK_32(3, 2), ...

etienne-lms · 2019-11-08T06:43:29Z

+
+static TEE_Result caam_get_mkvb(uint8_t *dest)
+{
+	struct imx_mkvb mkvb = { 0 };


minor: prefer { }

etienne-lms · 2019-11-08T06:47:25Z

+			return ret;
+		mkvb_retrieved = true;
+	}
+	memcpy(&hwkey->data, &stored_key, sizeof(hwkey->data));


prefer add empty line after if() { } block and before last return.

etienne-lms · 2019-11-08T06:48:08Z

 	if (!caam)
 		return TEE_ERROR_GENERIC;

+	caam_enable_clocks();


add an empty line below.

etienne-lms · 2019-11-08T06:54:12Z

+	do {
+		reg_val = io_read32((vaddr_t)&ctrl->jrcfg[MKVB_JR].jrintr);
+		reg_val &= 0xc;
+	} while ((reg_val & 0x1) && --timeout);


1 does not seems the expect value, since mask at line 58.

github-actions · 2020-01-10T00:17:21Z

This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed pull request at any time.

From pull-request OP-TEE#3160 imx: enable CAAM clocks before accessing registers The CAAM clocks need to be enabled, otherwise access to the CAAM might result in a bus stall. Signed-off-by: Rouven Czerwinski <[email protected]> plat-imx: pack caam structs Add __attribute__((packed)) to the CAAM struct definitions to ensure the compiler does not insert padding into the structures. Signed-off-by: Rouven Czerwinski <[email protected]> plat-imx: register definitions for CAAM Register Definitions in the same style as used by the jobring allocation code, by extending the structure definitions and inserting padding if the registers are undocumented. Signed-off-by: Rouven Czerwinski <[email protected]> imx_caam: implement tee_otp_get_hw_unique_key Minimal implementation for tee_otp_get_hw_unique_key using the Master Key Verification Blob (MKVB) produced by the CAAM. Only the first 16 bytes are copied into the hw unique key structure, since the MKVB is 32 bytes long. Signed-off-by: Rouven Czerwinski <[email protected]> Signed-off-by: Ricardo Salveti <[email protected]>

Emantor force-pushed the topic/caam_huk branch 2 times, most recently from e182156 to fe8393e Compare July 31, 2019 07:32

jforissier reviewed Jul 31, 2019

View reviewed changes

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

Comment thread core/arch/arm/plat-imx/imx_caam.h Outdated

jforissier reviewed Jul 31, 2019

View reviewed changes

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

0xB0D reviewed Jul 31, 2019

View reviewed changes

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

etienne-lms reviewed Jul 31, 2019

View reviewed changes

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

Comment thread core/arch/arm/plat-imx/imx_caam.c Outdated

etienne-lms reviewed Jul 31, 2019

View reviewed changes

Comment thread core/arch/arm/plat-imx/imx_caam.h Outdated

etienne-lms reviewed Jul 31, 2019

View reviewed changes

Comment thread core/arch/arm/plat-imx/imx_caam.h Outdated

Comment thread core/arch/arm/plat-imx/imx_caam.h Outdated

etienne-lms reviewed Jul 31, 2019

View reviewed changes

Emantor added 4 commits August 1, 2019 15:15

imx: enable CAAM clocks before accessing registers

b93a58e

The CAAM clocks need to be enabled, otherwise access to the CAAM might result in a bus stall. Signed-off-by: Rouven Czerwinski <[email protected]>

plat-imx: pack caam structs

22628ab

Add __attribute__((packed)) to the CAAM struct definitions to ensure the compiler does not insert padding into the structures. Signed-off-by: Rouven Czerwinski <[email protected]>

plat-imx: register definitions for CAAM

1b23700

Register Definitions in the same style as used by the jobring allocation code, by extending the structure definitions and inserting padding if the registers are undocumented. Signed-off-by: Rouven Czerwinski <[email protected]>

Emantor force-pushed the topic/caam_huk branch from af97a85 to e8d9e1b Compare August 1, 2019 13:17

Emantor marked this pull request as ready for review August 1, 2019 13:17

etienne-lms reviewed Aug 1, 2019

View reviewed changes

Comment thread core/arch/arm/plat-imx/drivers/imx_caam.c Outdated

Comment thread core/arch/arm/plat-imx/drivers/imx_caam.c Outdated

[Review] static & goto out, remove parenthesis

1936c6a

Address the review comments from @etienne-lms. Signed-off-by: Rouven Czerwinski <[email protected]>

[Amend] use second jobring

ef13711

In order to allow a SPL -> OP-TEE -> U-Boot -> HAB authenticate -> Linux boot flow, use job ring two instead of one, since one might be used by the HAB. Signed-off-by: Rouven Czerwinski <[email protected]>

etienne-lms reviewed Aug 6, 2019

View reviewed changes

[Review] fix review comments from etienne

8d48dbc

Address review comments from @etienne-lms. Signed-off-by: Rouven Czerwinski <[email protected]>

Emantor force-pushed the topic/caam_huk branch 2 times, most recently from a0ee937 to dcb7369 Compare August 7, 2019 05:57

Emantor added 2 commits August 7, 2019 08:40

[Review] handle reset timeout

caa5c4a

Handle job ring reset timeouts by passing the return value. Signed-off-by: Rouven Czerwinski <[email protected]>

[Review] fix none replaced job ring numbers

dd874c0

Those were forgotten in the previous commits, fix them now. Signed-off-by: Rouven Czerwinski <[email protected]>

Emantor force-pushed the topic/caam_huk branch from dcb7369 to dd874c0 Compare August 7, 2019 06:40

etienne-lms suggested changes Aug 7, 2019

View reviewed changes

[Review] fix more review comments

37fe29c

Fixes the leftover review comments from @etienne-lms. Signed-off-by: Rouven Czerwinski <[email protected]>

ricardosalveti reviewed Nov 4, 2019

View reviewed changes

etienne-lms reviewed Nov 8, 2019

View reviewed changes

github-actions Bot added the Stale label Jan 10, 2020

github-actions Bot closed this Jan 20, 2020

Emantor mentioned this pull request Feb 14, 2020

Hardware Unique Key (HUK) generation using the CAAM Master Key Verification Blob (MKVB) for i.MX platforms. #3608

Merged

Conversation

Emantor commented Jul 31, 2019

Uh oh!

jforissier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jforissier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Emantor commented Jul 31, 2019

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Emantor commented Aug 1, 2019

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cneveux commented Aug 2, 2019

Uh oh!

Emantor commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardosalveti commented Aug 2, 2019

Uh oh!

Emantor commented Aug 2, 2019

Uh oh!

cneveux commented Aug 5, 2019

Uh oh!

Emantor commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Emantor commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Emantor commented Aug 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Emantor commented Aug 2, 2019 •

edited

Loading

Emantor commented Aug 5, 2019 •

edited

Loading

Emantor commented Aug 5, 2019 •

edited

Loading