core: add interface to load and decrypt/authenticate user TAs #1513

jforissier · 2017-05-05T15:38:49Z

Some use cases may require custom load and verification methods for
user-mode Trusted Applications. Introduce struct user_ta_store_ops with
open(), get_size(), read() and close() functions to abstract these
tasks from the ELF loader code. Do the communication with
tee-supplicant as well as the hashing and signature verification of the
TA binary in core/arch/arm/kernel/ree_fs_ta.c, which may be disabled
and replaced by a different implementation if need be.

CC: Zeng Tao prime.zeng@hisilicon.com
Signed-off-by: Jerome Forissier jerome.forissier@linaro.org
Tested-by: Jerome Forissier jerome.forissier@linaro.org (HiKey, QEMU)

jenswi-linaro · 2017-05-08T13:18:19Z

core/arch/arm/kernel/ree_fs_ta.c

+	if (!*shdr)
+		return TEE_ERROR_SECURITY;
+	memcpy(*shdr, nw_ta, shdr_size);
+


I think we should check shdr_size == SHDR_GET_SIZE(*shdr)

Right, will fix.

jenswi-linaro · 2017-05-08T15:00:52Z

core/arch/arm/kernel/ree_fs_ta.c

+		 * Last read: time to check if our digest matches the expected
+		 * one (from the signed header)
+		 */
+		res = check_digest(h);


This seems a bit fragile. What if there's some mismatch that prevents h->offs from reaching h->nw_ta_size?
I don't have a good idea how to fix this right now.

Well the algorithm is unchanged actually, and we can consider it is a contract between the TA store and the TEE core that ta_get_size() bytes should be read (unless the core considers this size is unacceptable for some reason, or some other error occurs).

OK, please update the description of the interface to make that clear.

jforissier · 2017-05-09T06:25:03Z

Updated. I'll probably rephrase the commit subject to be "core: add interface to load user TAs" because user_ta_store_ops has nothing to do with authentication or encryption (which is entirely left to the implementation).

jenswi-linaro · 2017-05-09T08:31:46Z

With my last comment addressed:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

etienne-lms

Few minor comment (could be discarded).
Yet, please crosscheck the comment about the assert() in init_session_with_ta_store().

etienne-lms · 2017-05-09T08:25:06Z

core/sub.mk

@@ -2,9 +2,11 @@ subdirs-y += kernel
 subdirs-y += tee
 subdirs-y += drivers

+ifeq ($(CFG_WITH_USER_TA)$(CFG_REE_FS_TA),yy)


nitpicking: prevent failure if one is not set and the other falsely set to yy:
ifeq ($(CFG_WITH_USER_TA)-$(CFG_REE_FS_TA),y-y)

etienne-lms · 2017-05-09T08:28:27Z

core/arch/arm/kernel/user_ta.c

-	DMSG("   Load dynamic TA");
-	/* load and verify */
-	res = ta_load(uuid, signed_ta, &s->ctx);
+	DMSG("Load user TA %pUl", (void *)uuid);


minor: is this trace (the uuid address) useful ?

It's the "special" format pUl ("pointer to UUID in little-endian format") so it will print the UUID, not the address ;)

nice, i didn't notice that (i saw it was merged more than 1 year ago!)

etienne-lms · 2017-05-09T08:32:24Z

core/arch/arm/kernel/user_ta.c

-
-	DMSG("      dyn TA : %pUl", (void *)&s->ctx->uuid);
-
+	assert(!memcmp(uuid, &s->ctx->uuid, sizeof(TEE_UUID)));


Replace assert() with a non debug test.

etienne-lms · 2017-05-09T08:35:27Z

core/arch/arm/kernel/user_ta.c

-		     ta_head->flags, opt_flags, man_flags);
+	if ((ta_head->flags & optional_flags) != ta_head->flags ||
+	    (ta_head->flags & mandatory_flags) != mandatory_flags) {
+		EMSG("TA flag issue: flags=%x optional=%X mandatory=%X",


minor: ("issue" already in previous code) trace all field in either uppercase or lowercase.

etienne-lms · 2017-05-09T08:50:05Z

core/arch/arm/kernel/ree_fs_ta.c

+	if (!phta)
+		return TEE_ERROR_OUT_OF_MEMORY;
+
+	*ta = phys_to_virt(phta, MEM_AREA_NSEC_SHM);


minor: thread_rpc_alloc_payload() already insures that the overall buffer in non secure memory. Could assert or panic here.

etienne-lms · 2017-05-09T09:06:32Z

core/arch/arm/include/kernel/user_ta.h

+static inline TEE_Result tee_ta_register_ta_store(
+			const struct user_ta_store_ops *ops __unused)
+{
+	return TEE_SUCCESS;


Should return TEE_ERROR_NOT_SUPPORTED

etienne-lms · 2017-05-09T09:55:28Z

core/arch/arm/kernel/user_ta.c

-	DMSG("      dyn TA : %pUl", (void *)&s->ctx->uuid);
-
+	if (memcmp(uuid, &s->ctx->uuid, sizeof(TEE_UUID)));
+		panic();


Really panic here ? We can reach this case if the REE provides a TA binary that does not contain the requested TA. I think TEE should not panic here, but simply refuse to load the TA. I believe it should not be possible for the REE to make the TEE panicking.
Sorry if I was not that explicit in my previous comment.

This "can't happen" since ta_load() already does a memcmp() and returns an error on mismatch. The assert() should be good enough if that's even needed.

Exactly, I'll remove this check and while I'm at it, drop init_session_with_ta_store() and call ta_load() directly from tee_ta_init_user_ta_session().

true. my fault. this test is useless.

jforissier · 2017-05-09T11:45:59Z

Updated

etienne-lms

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

etienne-lms · 2017-05-09T11:00:34Z

core/arch/arm/kernel/user_ta.c

-	DMSG("      dyn TA : %pUl", (void *)&s->ctx->uuid);
-
+	if (memcmp(uuid, &s->ctx->uuid, sizeof(TEE_UUID)));
+		panic();


true. my fault. this test is useless.

jenswi-linaro · 2017-05-09T12:01:02Z

Still OK with me.

Some use cases may require custom load and verification methods for user-mode Trusted Applications. Introduce struct user_ta_store_ops with open(), get_size(), read() and close() functions to abstract these tasks from the ELF loader code. Do the communication with tee-supplicant as well as the hashing and signature verification of the TA binary in core/arch/arm/kernel/ree_fs_ta.c, which may be disabled and replaced by a different implementation if need be. CC: Zeng Tao <prime.zeng@hisilicon.com> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey, QEMU) Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

jforissier mentioned this pull request May 5, 2017

core: add interface for user TA loading and decryption/authentication #1498

Closed

jenswi-linaro reviewed May 8, 2017

View reviewed changes

etienne-lms reviewed May 9, 2017

View reviewed changes

jforissier force-pushed the custom-ta-load2 branch from e508168 to 744c693 Compare May 9, 2017 09:38

etienne-lms reviewed May 9, 2017

View reviewed changes

etienne-lms approved these changes May 9, 2017

View reviewed changes

jforissier force-pushed the custom-ta-load2 branch from 3700b0f to af7809d Compare May 9, 2017 13:05

jforissier force-pushed the custom-ta-load2 branch from af7809d to ee664c1 Compare May 10, 2017 07:54

jforissier merged commit ee664c1 into OP-TEE:master May 10, 2017

jforissier deleted the custom-ta-load2 branch May 10, 2017 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add interface to load and decrypt/authenticate user TAs #1513

core: add interface to load and decrypt/authenticate user TAs #1513

jforissier commented May 5, 2017

jenswi-linaro May 8, 2017

jforissier May 9, 2017

jenswi-linaro May 8, 2017

jforissier May 9, 2017

jenswi-linaro May 9, 2017

jforissier commented May 9, 2017

jenswi-linaro commented May 9, 2017

etienne-lms left a comment

etienne-lms May 9, 2017

jforissier May 9, 2017

etienne-lms May 9, 2017

jforissier May 9, 2017

etienne-lms May 9, 2017

etienne-lms May 9, 2017

etienne-lms May 9, 2017

etienne-lms May 9, 2017

jforissier May 9, 2017

etienne-lms May 9, 2017

jforissier May 9, 2017

etienne-lms May 9, 2017

jenswi-linaro May 9, 2017

jforissier May 9, 2017

etienne-lms May 9, 2017

jforissier commented May 9, 2017

etienne-lms left a comment

etienne-lms May 9, 2017

jenswi-linaro commented May 9, 2017


		DMSG(" dyn TA : %pUl", (void *)&s->ctx->uuid);

		assert(!memcmp(uuid, &s->ctx->uuid, sizeof(TEE_UUID)));

core: add interface to load and decrypt/authenticate user TAs #1513

core: add interface to load and decrypt/authenticate user TAs #1513

Conversation

jforissier commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented May 9, 2017

jenswi-linaro commented May 9, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented May 9, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented May 9, 2017