Early TAs #1733

jforissier · 2017-08-04T07:03:01Z

This series implements a feature called "early TAs", which is a way to link Trusted Applications into the TEE binary so that they may be loaded independently of the REE file system and tee-supplicant.

One use case is being able to invoke a TA from a Linux driver during the boot sequence, before user mode has been spawn. Obviously, In this use case, any service depending on tee-supplicant won't be available, but it can be useful anyway.

Another use case is to prevent uncontrolled rollback of user TAs. Since TAs are effectively part of the TEE binary, they follow the same life cycle (authentication by the previous stage of th boot loader, controlled updates...).

Note about compression: commit #.6 imports some files from zlib into core/lib/zlib. I could probably have re-used what we already have in lib/libzlib, but I thought that since we had some discussions about possibly removing our (partial) Trusted UI support, this whole directory would probably go away and we would be better off with zlib code under the core/ folder. I can change that of course.

igoropaniuk · 2017-08-04T08:22:17Z

core/arch/arm/kernel/early_ta.c

+	strm->zfree = zfree;
+	st = inflateInit(strm);
+	if (st != Z_OK) {
+		DMSG("Early TA decompression initialization error (%d)", st);


igoropaniuk · 2017-08-04T08:28:10Z

core/arch/arm/kernel/early_ta.c

+	if (ta->uncompressed_size)
+		*size = ta->uncompressed_size;
+	else
+		*size = ta->size;


minor: blank line

*size is set twice. Looks like an error: I guess *size = h->early_ta->size; below should be removed.

Oops! Will fix. I introduced this bug when I addressed @igoropaniuk's comment.

igoropaniuk · 2017-08-04T08:28:24Z

core/arch/arm/kernel/early_ta.c

+		return TEE_ERROR_BAD_PARAMETERS;
+	if (data)
+		memcpy(data, src, len);
+	h->offs += len;


minor: blank line

igoropaniuk · 2017-08-04T08:29:36Z

core/arch/arm/kernel/early_ta.c

+{
+	z_stream *strm = &h->strm;
+	size_t total = 0;
+	uint8_t *tmpbuf = NULL;


unneeded initialization

My thoughts exactly, but GCC 6.2.1 doesn't like it:

$ make PLATFORM=hikey CFG_EARLY_TA=y out/arm-plat-hikey/core/arch/arm/kernel/early_ta.o CHK out/arm-plat-hikey/conf.mk CHK out/arm-plat-hikey/include/generated/conf.h CC out/arm-plat-hikey/core/arch/arm/kernel/early_ta.o core/arch/arm/kernel/early_ta.c: In function ‘early_ta_read’: core/arch/arm/kernel/early_ta.c:198:3: error: ‘tmpbuf’ may be used uninitialized in this function [-Werror=maybe-uninitialized] free(tmpbuf); ^ core/arch/arm/kernel/early_ta.c:148:11: note: ‘tmpbuf’ was declared here uint8_t *tmpbuf; ^ cc1: all warnings being treated as errors mk/compile.mk:146: recipe for target 'out/arm-plat-hikey/core/arch/arm/kernel/early_ta.o' failed make: *** [out/arm-plat-hikey/core/arch/arm/kernel/early_ta.o] Error 1

igoropaniuk · 2017-08-04T08:31:34Z

core/arch/arm/kernel/early_ta.c

+	.get_size = early_ta_get_size,
+	.read = early_ta_read,
+	.close = early_ta_close,
+	.priority = 5,


Please add comment about priority field, what does it stand for

Comment added in core/arch/arm/kernel/elf_load.h.

Suggest to define those magic priority values in user_ta.h (i.e EARLY_TA_STORE_PRIORITY and REE_TA_STORE_PRIORITY).

I think user_ta.h should not need to know about the various TA stores, so I'd rather avoid that.

igoropaniuk · 2017-08-04T08:33:00Z

scripts/ta_bin_to_c.py

+	return parser.parse_args()
+
+def main():
+	import array


Possible holy war, but https://stackoverflow.com/questions/1024049/is-it-pythonic-to-import-inside-functions

I'll change. I also have a slight preference for imports at the top, probably because I'm essentially developing in C ;)

igoropaniuk · 2017-08-04T08:38:37Z

core/core.mk

@@ -94,6 +94,12 @@ libdir = core/lib/libfdt
 include mk/lib.mk
 endif

+ifeq ($(CFG_ZLIB),y)


I guess when CFG_ZLIB isn't set early_ta.c won't compile?
Is it possible to use early TA feature without compression support?

Right. Making compression support optional is doable of course, but it would make the code a bit harder to read (several #ifdef's in early_ta.c).

igoropaniuk · 2017-08-04T08:40:59Z

core/arch/arm/kernel/early_ta.c

+		DMSG("Early TA decompression initialization error (%d)", st);
+		return false;
+	}
+	return true;


minor: blank line

igoropaniuk · 2017-08-04T08:45:06Z

scripts/ta_bin_to_c.py

+	f = open(args.out, 'w')
+	f.write('/* Generated from ' + args.ta + ' by ' +
+		os.path.basename(__file__) + ' */\n\n')
+	f.write('#include <compiler.h>\n');


why not to move creation of stub to separate function?

This would mean moving almost everything from main() to generate_file(), no? So what would be the benefit?

igoropaniuk · 2017-08-04T08:46:06Z

core/arch/arm/kernel/early_ta.c

+#include <kernel/early_ta.h>
+#include <kernel/linker.h>
+#include <kernel/user_ta.h>
+#include <zlib.h>


minor: include order

igoropaniuk · 2017-08-04T08:46:39Z

core/arch/arm/kernel/early_ta.c

+out:
+	if (!data)
+		free(tmpbuf);
+	return ret;


minor: blank line

etienne-lms · 2017-08-07T09:49:53Z

core/arch/arm/kernel/early_ta.c

+	if (ta->uncompressed_size)
+		*size = ta->uncompressed_size;
+	else
+		*size = ta->size;


*size is set twice. Looks like an error: I guess *size = h->early_ta->size; below should be removed.

etienne-lms · 2017-08-07T09:55:23Z

core/arch/arm/kernel/early_ta.c

+		total += out;
+		FMSG("%zu bytes", out);
+		if (!data) {
+			strm->next_out = tmpbuf;


minor: strm->next_out = tmpbuf; seems useless: already set.

No, that's for the case we're asked to read more than 1K of uncompressed data and throw them away (data == NULL). We read 1K chunks at a time. By resetting the pointer on each loop I try to read as much as possible each time.

You mean we need to reset the output buffer point to the temp buff start address ?
Maybe this deserves a small comment. (maybe not :)

Exactly. I will add a comment.

etienne-lms · 2017-08-07T10:01:15Z

core/arch/arm/kernel/early_ta.c

+	 *   stream nor the end of the output buffer were met
+	 * - Z_BUF_ERROR when there is still input to process but the output
+	 *   buffer is full
+	 * - Z_STREAM_END when the end of the intput stream was reached


Could you elaborate a bit these comments, stating which return values relate excepted successful content read? (the loop implementation is not that obvious)

I suppose while ((st == Z_OK) && (total != len)) is obvious, right? So it's only the Z_BUF_ERROR case, not being an error here, that deserves some clarification?

Reading again the current comment, I found it clear. Maybe move Z_STREAM_END description next to Z_OK. It would look less like an error case (as right after Z_BUF_ERROR);

Minor: add a terminal dots to each bullet.

etienne-lms · 2017-08-07T10:01:56Z

core/arch/arm/kernel/early_ta.c

+			strm->avail_out = MIN(len - total, 1024U);
+		}
+	} while ((st == Z_OK || st == Z_BUF_ERROR) && (total != len));
+	if (st != Z_OK && st != Z_STREAM_END && st != Z_BUF_ERROR) {


I would expect here (once read loop is done) that st == Z_BUF_ERROR is an error case.

Right, I'll remove this check.

etienne-lms · 2017-08-07T10:02:49Z

core/arch/arm/kernel/early_ta.c

+	}
+	ret = TEE_SUCCESS;
+out:
+	if (!data)


minor: can call free(tmpbuf) straight since pointer is NULL initialized.

etienne-lms · 2017-08-07T10:17:27Z

mk/config.mk

@@ -176,6 +176,26 @@ CFG_WITH_USER_TA ?= y
 # case you implement your own TA store
 CFG_REE_FS_TA ?= y

+# Support for loading user TAs from a special section in the TEE binary


minor: add terminal dot.

etienne-lms · 2017-08-07T10:19:47Z

mk/config.mk

@@ -176,6 +176,26 @@ CFG_WITH_USER_TA ?= y
 # case you implement your own TA store
 CFG_REE_FS_TA ?= y

+# Support for loading user TAs from a special section in the TEE binary
+# Such TAs are available even before tee-supplicant is available (hence their
+# name), but note that many services exported to TAs may need tee-supplicant,


I think EARLY_TA should be rename BUILTIN_TA.

Indeed, BUILTIN_TA better describes what they really are. If no one objects I'll rename everything once we're done with the review. Otherwise it will bee too complex to follow the changes.

etienne-lms · 2017-08-07T10:23:09Z

mk/config.mk

+# so early use is limited to a subset of the TEE Internal Core API (crypto...)
+# To use this feature, set EARLY_TA to the paths to one or more TA ELF file(s)
+# For example:
+#   EARLY_TA=path/to/8aaaf200-2450-11e4-abe2-0002a5d5c51b.stripped.elf


From this description it is not clear whether one should define several EARLY_TA values (sic) each defining the full path of the TA elf file or if EARLY_TA is expected a unique directory path where one can store several TA ELF files to be embedded in the core binary.

As EARLY_TA is a configuration directive, I would suggest to prefix it CFG_EARLY_TA_PATH.

I'll update the comment. What's expected is one or more paths to TA files separated with spaces, such as:

EARLY_TA="../optee_test/out/ta/crypt/cb3e5ba0-adf1-11e0-998b-0002a5d5c51b.stripped.elf ../optee_test/out/ta/aes_perf/e626662e-c0e2-485c-b8c8-09fbce6edf3d.stripped.elf"

Regarding the lack of CFG_ prefix: you're mostly right but there's a trick (that I should document): all CFG_ variables are written to $(out-dir)/conf.mk and any change to the values in this file causes a re-compilation of everything. So, consider the build steps I have given as an example:

$ make CFG_EARLY_TA=y # (1) <build some TAs> # (2) $ make ... EARLY_TA=/some/path # (3)

You can't give CFG_EARLY_TA_PATH during step (1) since we assume the TAs have not been built yet (they need optee_os to be built first so that dev_kit is available). So you can't have CFG_EARLY_TA_PATH set to the same value in steps (1) and (3) thus causing a full rebuilt of optee_os at step (3).

I propose to rename EARLY_TA and use BUILTIN_TA_PATHS instead (will do it at the same time I replace "early" with "builtin").

etienne-lms · 2017-08-07T13:05:34Z

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

+
+struct early_ta {
+	TEE_UUID uuid;
+	uint32_t flags;


flags seems useless.

Leftover from a previous implementation... will remove, thanks

etienne-lms · 2017-08-07T13:09:44Z

core/arch/arm/kernel/elf_load.h

+	 * For debug purposes only.
+	 */
+	const char *description;
+


minor: remove empty line (for consistency) ?

etienne-lms · 2017-08-07T13:09:48Z

core/arch/arm/kernel/elf_load.h

@@ -62,6 +69,13 @@ struct user_ta_store_ops {
 	 * Close a TA handle. Do nothing if @h == NULL.
 	 */
 	void (*close)(struct user_ta_store_handle *h);
+


minor: remove empty line (for consistency) ?

etienne-lms · 2017-08-07T13:15:31Z

core/arch/arm/kernel/early_ta.c

+		total += out;
+		FMSG("%zu bytes", out);
+		if (!data) {
+			strm->next_out = tmpbuf;


You mean we need to reset the output buffer point to the temp buff start address ?
Maybe this deserves a small comment. (maybe not :)

etienne-lms · 2017-08-07T13:19:50Z

core/arch/arm/kernel/kern.ld.S

@@ -329,6 +336,11 @@ SECTIONS
 		KEEP(*(.rodata.dtdrv))
 		__rodata_dtdrv_end = .;
 #endif
+#ifdef CFG_EARLY_TA
+		__rodata_early_ta_start = .;


. = ALIGN(8);

Finally remove the 8 byte alignment.

etienne-lms · 2017-08-07T13:21:14Z

core/arch/arm/kernel/user_ta.c

+	     ops->priority);
+
+	SLIST_FOREACH(e, &uta_store_list, link) {
+		if (e->priority > ops->priority)


Add assert(e->priority != ops->priority); before.

I thought I would allow equal priorities, but it may be a bad idea (invocation order would depend on registration order which may not be well controlled...). I will add the assert.

etienne-lms · 2017-08-07T13:22:06Z

core/arch/arm/plat-sunxi/kern.ld.S

@@ -129,6 +129,11 @@ SECTIONS

 	.rodata : ALIGN(4) {
 		__rodata_start = .;
+#ifdef CFG_EARLY_TA
+		__rodata_early_ta_start = .;


. = ALIGN(8);

etienne-lms · 2017-08-07T13:31:20Z

core/arch/arm/kernel/early_ta.c

+	 *   stream nor the end of the output buffer were met
+	 * - Z_BUF_ERROR when there is still input to process but the output
+	 *   buffer is full
+	 * - Z_STREAM_END when the end of the intput stream was reached


Reading again the current comment, I found it clear. Maybe move Z_STREAM_END description next to Z_OK. It would look less like an error case (as right after Z_BUF_ERROR);

Minor: add a terminal dots to each bullet.

etienne-lms · 2017-08-07T13:36:10Z

core/arch/arm/kernel/early_ta.c

+#define for_each_early_ta(_ta) \
+	for (_ta = &__rodata_early_ta_start; ta < &__rodata_early_ta_end; \
+	     _ta = (const struct early_ta *)				  \
+		   ROUNDUP((vaddr_t)ta + sizeof(*ta) + ta->size, 8))


Few ta to replace with _ta in the macro.

Yeah, good catch

etienne-lms · 2017-08-08T07:36:35Z

core/arch/arm/kernel/early_ta.c

+		return TEE_ERROR_OUT_OF_MEMORY;
+
+	if (ta->uncompressed_size) {
+		/* TA is compressed */


minor: I think this comment is useless

OK, it is redundant with /* 0: not compressed */ in early_ta.h. Will remove it.

etienne-lms · 2017-08-08T07:43:03Z

core/arch/arm/kernel/early_ta.c

+	.get_size = early_ta_get_size,
+	.read = early_ta_read,
+	.close = early_ta_close,
+	.priority = 5,


Suggest to define those magic priority values in user_ta.h (i.e EARLY_TA_STORE_PRIORITY and REE_TA_STORE_PRIORITY).

etienne-lms · 2017-08-08T07:45:10Z

core/arch/arm/kernel/early_ta.c

+				 " (compressed, uncompressed %u)",
+				 ta->uncompressed_size);
+		DMSG("Early TA %pUl size %u%s", (void *)&ta->uuid, ta->size,
+		     msg);


Must reset msg[0] = '\0' in the loop.

etienne-lms · 2017-08-08T07:46:36Z

core/arch/arm/kernel/elf_load.h

+	 * Higher priority instances are tried first when a TA is looked up.
+	 */
+	SLIST_ENTRY(user_ta_store_ops) link;
+	int priority; /* Lower value means higher priority */


Minor: comment above the field (rather than appended in the same line) ?

etienne-lms · 2017-08-08T07:47:55Z

core/arch/arm/kernel/ree_fs_ta.c

@@ -324,11 +324,13 @@ static void ta_close(struct user_ta_store_handle *h)
 	free(h);
 }

-static const struct user_ta_store_ops ops = {
+static struct user_ta_store_ops ops = {


Really need to remove the const qualifier ?

Yes, because link is modified when the structure is put in the TA store list.

right. thanks.

etienne-lms · 2017-08-08T07:48:49Z

core/arch/arm/kernel/user_ta.c

+	SLIST_FOREACH(e, &uta_store_list, link) {
+		/*
+		 * Do not allow equal priorities to avoid any dependency on
+		 * registration order


minor: terminal dot.

etienne-lms · 2017-08-08T07:51:25Z

mk/config.mk

+#   $ make ... CFG_EARLY_TA=y # Enable the feature to avoid later recompilation
+#   <build some TAs>
+#   $ make ... EARLY_TA_PATHS=<paths> # Embbed the TA(s) (will re-link OP-TEE)
+ifneq ($(EARLY_TA),)


s/EARLY_TA/EARLY_TA_PATHS/

etienne-lms · 2017-08-08T07:53:14Z

core/sub.mk

+cleanfiles += $(sub-dir-out)/early_ta_$$(early-ta-$1-uuid).c
+cflags-remove-early-ta-$1-y := -pedantic
+endef
+$(foreach f, $(EARLY_TA), $(eval $(call process_early_ta,$(f))))


s/EARLY_TA/EARLY_TA_PATHS/

Could you check that this works ok even if EARLY_TA_PATHS is empty or even not defined ?

It does work

etienne-lms · 2017-08-08T15:17:48Z

core/arch/arm/kernel/user_ta.c

+		res = ta_load(uuid, store, &s->ctx);
+		if (res == TEE_SUCCESS) {
+			s->ctx->ops = &user_ta_ops;
+			return res;


Here, I wonder if we should try other stores only if ta_load() returns TEE_ERROR_ITEM_NOT_FOUND.
I mean that if the target uuid was found in the early TAs, the TA binary is expected valid. For example, if core lacks of heap was loading an early TA, it should not try the load the TA from the REE.

In the same scope, how should core behave of an early TA shows formatting issues. I think it should panic if in debug mode, and simply refuse to load REE TA instead.

Not trying the next store in case of error other than TEE_ERROR_ITEM_NOT_FOUND is also a valid option I think, but why would it be better? I'd rather keep the current code for the moment. If we later find out that some use cases require a different behavior, we'll change the logic.

With this code if an early TA has formatting issues and there isn't a backup version in REE it would return TEE_ERROR_ITEM_NOT_FOUND instead of the actual error code.

OK, how about this?

SLIST_FOREACH(store, &uta_store_list, link) { DMSG("Lookup user TA %pUl (%s)", (void *)uuid, store->description); res = ta_load(uuid, store, &s->ctx); if (res == TEE_ERROR_ITEM_NOT_FOUND) continue; if (res == TEE_SUCCESS) s->ctx->ops = &user_ta_ops; else DMSG("res=0x%x", res); return res; } return TEE_ERROR_ITEM_NOT_FOUND;

Looks good.

etienne-lms · 2017-08-08T15:18:42Z

core/arch/arm/kernel/early_ta.c

+	.get_size = early_ta_get_size,
+	.read = early_ta_read,
+	.close = early_ta_close,
+	.priority = 5,


etienne-lms · 2017-08-08T15:21:42Z

I think this change is pretty nice as it is. See my last comment regarding load error cases.

I have not reviewed the zlib implementation :) and will only ack the python scripting.

jforissier · 2017-08-11T08:03:35Z

Update: fixed issue that tee.elf would not always be re-linked when TAs are added or removed.

etienne-lms · 2017-08-11T08:17:04Z

core/arch/arm/kernel/link.mk

+$(if $(strip $(filter-out $(link-objs), $(old-link-objs))
+	     $(filter-out $(old-link-objs), $(link-objs))), FORCE_LINK := FORCE)
+endef
+$(eval $(call check-link-objs))


I fear this will not force relink if one updates an already existing TA ELF file.
Maybe it could be more simple to simple force relink if EARLY_TA_PATH is defined.

If a TA is modified, the C file will become outdated and will be generated again. It's not something that is handled by this. This only takes care of the situation when no files are modified (because they are already all present and up-to-date from a previous build), but tee.elf should still be re-linked to add/remove files.

airbak · 2017-08-12T09:35:10Z

core/arch/arm/kernel/sub.mk

@@ -1,6 +1,8 @@
 ifeq ($(CFG_WITH_USER_TA),y)
 srcs-y += user_ta.c
 srcs-$(CFG_REE_FS_TA) += ree_fs_ta.c
+srcs-$(CFG_EARLY_TA) += early_ta.c
+cflags-early_ta.c-y += -Wno-discarded-qualifiers


Compile error with gcc version 4.9.2 20140904:
cc1: error: unrecognized command line option "-Wno-discarded-qualifiers" [-Werror]

airbak · 2017-08-12T09:56:29Z

core/arch/arm/kernel/early_ta.c

+{
+	int st;
+
+	strm->next_in = ta->ta;


core/arch/arm/kernel/early_ta.c:77:16: error: assignment discards 'const' qualifier from pointer target type [-Werror]
ZLIB_CONST defined in ./lib/libzlib/include/zconf.h, but not defined in ./core/lib/zlib/zconf.h.

Thanks for the hint, I'll fix this whole const handling issue by defining ZLIB_CONST and removing -Wno-discarded-qualifiers.

airbak · 2017-08-14T07:55:52Z

mk/config.mk

+#     EARLY_TA_PATHS="path/to/8aaaf200-2450-11e4-abe2-0002a5d5c51b.stripped.elf \
+#                     path/to/cb3e5ba0-adf1-11e0-998b-0002a5d5c51b.stripped.elf"
+# Typical build steps:
+#   $ make ... CFG_EARLY_TA=y # Enable the feature to avoid later recompilation


As CFG_EARLY_TA=y, EARLY_TA_PATHS=test_ta_uuid.
When test_ta and optee_os are both modified, need to make twice optee_os.
May it add "make headers_install"? Build headers for TA first, then build TA, finally build optee_os.

Please note that, if you build optee_os with CFG_EARLY_TA=y first, and EARLY_TA_PATHS not set as explained in the comment, then optee_os will not be built twice. Only the link step will occur again when you run make EARLY_TA_PATHS=test_ta_uuid.elf.

That being said, I will see if I can do what you propose (add make headers_install). Shouldn't be that hard.

jforissier · 2017-08-16T16:48:04Z

All review comments addressed (or replied to), I think.

We have lots of review fix-ups so I will probably squash the commits together properly before asking for a last round of reviews.

When doing so, should I rename "early" to "built-in" as suggested previously (that is, CFG_BUILTIN_TA, BUILTIN_TA_PATHS, struct builtin_ta etc.)?

jenswi-linaro · 2017-08-17T10:25:54Z

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

+	uint32_t size;
+	uint32_t uncompressed_size; /* 0: not compressed */
+	const uint8_t ta[]; /* @size bytes */
+} __aligned(8);


Why this alignment?

It helps iterate on the early_ta structs in the .rodata section. Without this, the compiler tends to align the structure on either 4 or 8 bytes and it depends on the arch.

What's the problem with different alignment depending on the arch?

At first I used __alignof__(struct early_ta) (instead of hard-coding 8) for rounding up the size of structs when iterating (here). It was OK on QEMU but I was 4 bytes off on D02 (64-bits). So I forced 8-byte alignement as an easy fix ;)
I'll take a closer look and see if I can get rid of it.

Fixed by applying __aligned(__alignof__(struct early_ta)) to each variable of this type (see the Python script). Results in all variables being 4-bytes aligned on 32-bit as well as 64-bit platforms with no unwanted padding. I noticed that applying the same align attribute to the type or the variable is not equivalent, but could not figure out if it's expected or not...

jenswi-linaro · 2017-08-17T11:26:27Z

core/arch/arm/kernel/user_ta.c

+		res = ta_load(uuid, store, &s->ctx);
+		if (res == TEE_SUCCESS) {
+			s->ctx->ops = &user_ta_ops;
+			return res;


With this code if an early TA has formatting issues and there isn't a backup version in REE it would return TEE_ERROR_ITEM_NOT_FOUND instead of the actual error code.

Apply changes proposed in review comment [1] (handle load errors differently). Link: [1] OP-TEE#1733 (comment) Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

etienne-lms

minor comment. looks good to me.

etienne-lms · 2017-08-22T08:11:42Z

core/arch/arm/plat-sunxi/kern.ld.S

@@ -129,6 +129,12 @@ SECTIONS

 	.rodata : ALIGN(4) {
 		__rodata_start = .;
+#ifdef CFG_EARLY_TA
+		. = ALIGN(8);


alignment to remove

etienne-lms · 2017-08-22T08:12:39Z

core/arch/arm/kernel/kern.ld.S

@@ -329,6 +336,11 @@ SECTIONS
 		KEEP(*(.rodata.dtdrv))
 		__rodata_dtdrv_end = .;
 #endif
+#ifdef CFG_EARLY_TA
+		__rodata_early_ta_start = .;


Finally remove the 8 byte alignment.

etienne-lms · 2017-08-22T08:24:00Z

mk/config.mk

+# Such TAs are available even before tee-supplicant is available (hence their
+# name), but note that many services exported to TAs may need tee-supplicant,
+# so early use is limited to a subset of the TEE Internal Core API (crypto...)
+# To use this feature, set EARLY_TA_PATHS to the paths to one or more TA ELF


As EARLY_TA_PATHS is a configuration directive that affects the core embedded content, i would rather have it prefixed CFG_. Your feeling ?

No CFG_ to avoid re-building: #1733 (comment)

ok, I see the point (I had forgotten that). However it looks rather like a workaround on the config to suite the current build process.

I wonder if we could split the optee_os build in 2:

build core only (libs + core): i.e make core

build user land only (libs + devkit): i.e make ta_devkit

In most cases (no early TA), one could run make all to get everything in place.
For early TAs support: first build the user libs, then build the (early) TAs, then build core with the early TAs. This would help generating a conf.mk that would reflect the real configuration used to build the core binary. This is change is related to that some platform could allow the TAs to build their own 'static' libraries independently from the core configuration: enabling some debug, profiling,traces for some TAs but not for all. But maybe that should be discussed later on...

I have already added make ta_dev_kit, after @airbak's suggestion above. So indeed, we could very well use CFG_EARLY_TA_PATHS as you suggest and still avoid re-compilation.
Let me give it a try and update ;-)

@etienne-lms ultimately I think CFG_ is not very convenient. During testing I have changed several times the list of applications (adding one or two, changing them), and with CFG_ the tee.bin is fully recompiled each time and it's a bit painful.
So I'm not changing the prefix. We can change later depending on the use cases.

etienne-lms · 2017-08-22T09:21:36Z

This P-R went to a good point (at least to my appraisal). You would like some review/ack tags or do we change early_ta into builtin_ta or whatever before tagging?

jforissier · 2017-08-22T13:32:48Z

@etienne-lms I have cleaned up and reordered the series, let's gather the review tags now.
GitHub shows the commits in a peculiar order (sorted by date it seems, when parent/child relationship would make more sense IMO). Here is the logical order for reviewing:

git log --oneline --reverse 5d0d5ac..HEAD

19e34cf Remove leading spaces and dots in DMSG() messages
f997070 Add target ta_dev_kit
1cc2bde Add description string to user_ta_store_ops
6ae578d Add support for several user TA stores
748fbdb Add support for early Trusted Applications
386f211 core: link.mk: force link of tee.elf when object list has changed
b40d654 Import zlib v1.2.11
22469c5 Add support for compressed early TAs

jenswi-linaro · 2017-08-22T14:25:04Z

core/arch/arm/kernel/user_ta.c

@@ -625,16 +625,19 @@ TEE_Result tee_ta_init_user_ta_session(const TEE_UUID *uuid,
 			struct tee_ta_session *s)
 {
 	const struct user_ta_store_ops *store;
-	TEE_Result res = TEE_ERROR_ITEM_NOT_FOUND;
+	TEE_Result res;


I though these changes where to go into 6ae578d "Add support for several user TA stores"

My mistake, will fix.

jenswi-linaro · 2017-08-22T14:28:26Z

core/arch/arm/kernel/kern.ld.S

@@ -329,6 +335,11 @@ SECTIONS
 		KEEP(*(.rodata.dtdrv))
 		__rodata_dtdrv_end = .;
 #endif
+#ifdef CFG_EARLY_TA
+		__rodata_early_ta_start = .;


Is __rodata_dtdrv_end guaranteed to end on a well aligned address?

Good catch. Strictly speaking we should have . = ALIGN(__alignof__(struct early_ta)) but that can't be understood by the linker. Our options are:

Hard-code 4, which I expect to be the value of __alignof__(struct early_ta) for pretty much all platforms currently.

Use 8 for greater safety (?)

Add a definition to asm-defines.c.

I am tempted to do (3) although it may look weird to use this "asm" file for that purpose. Thoughts?

We're usually doing 2. in the link script.

jenswi-linaro · 2017-08-22T14:30:10Z

core/sub.mk

+recipe-early-ta-$1 = scripts/ta_bin_to_c.py --ta $1 \
+		--out $(sub-dir-out)/early_ta_$$(early-ta-$1-uuid).c
+cleanfiles += $(sub-dir-out)/early_ta_$$(early-ta-$1-uuid).c
+cflags-remove-early-ta-$1-y := -pedantic


Why can't -pedantic be used?

out/arm/core/early_ta_8aaaf200-2450-11e4-abe2-0002a5d5c51b.c:17:8: error: initialization of a flexible array member [-Werror=pedantic] .ta = { ^

Putting a __extension__ in front of the const struct early_ta ... would eliminate the need of removing the -pedantic flag. It relies on a compiler extension after all.

Ha! I didn't know about this one. Fixed, thanks.

jforissier · 2017-08-23T06:17:28Z

Updated.

jenswi-linaro · 2017-08-23T08:42:24Z

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

jforissier · 2017-08-23T12:59:51Z

Jens' comments addressed and R-b applied.

git log --oneline --reverse master..HEAD
d1d4669 core/sub.mk: add missing dependency on the TA key processing script
0a7f5cf Remove leading spaces and dots in DMSG() messages
99af84e Add target ta_dev_kit
fcf98f3 Add description string to user_ta_store_ops
66b786d Add support for several user TA stores
afc3560 Add support for early Trusted Applications
08940b4 core: link.mk: force link of tee.elf when object list has changed
cf9108e Import zlib v1.2.11
0f96189 Add support for compressed early TAs

etienne-lms · 2017-08-24T09:40:32Z

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

Note: jens' r-b tag is missing on some commits.

jforissier · 2017-08-24T11:17:47Z

@etienne-lms thanks, I'll take care of adding the missing R-b before merging.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Some debug messages have various amounts of leading spaces and dots (.) probably in an attempt to better align the text. It is unreliable because debug traces include function names and line numbers, which introduce random offsets. Remove these characters. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Adds support for `make ta_dev_kit`, to build the user space libraries only and copy them (as well as the related header files and make files) to the export directory. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

In preparation for the introduction of a new TA store type, add a description string to the user_ta_store_ops structure and print it when a user-mode TA is being loaded. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Replace the pointer to the user-mode TA load operations with a list so that several implementations may be used simultaneously. Each store has its own priority. Make tee_ta_init_user_ta_session() iterate on the list and stop as soon as a matching TA is found. This is preparatory work for the introduction of a new TA store. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Early TAs are user-mode Trusted Applications that are embedded at link time in the TEE binary. A special read-only data section is used to store them (.rodata.early_ta). A Python script takes care of converting the TAs into a C source file with the proper linker section attribute. The feature is disabled by default. To enable it, the paths to the TA binaries have to be given in $(EARLY_TA_PATHS). They should be ELF files. Typical build steps: $ make ... CFG_EARLY_TA=y ta_dev_kit # (1) $ # ... build the TAs ... # (2) $ make ... EARLY_TA_PATHS=path/to/<uuid>.stripped.elf # (3) Notes: - Setting CFG_EARLY_TA=y during the first step (1) is not necessary, but it will avoid rebuilding libraries during the third step (3) - CFG_EARLY_TA is automatically enabled when EARLY_TA_PATHS is non-empty in step (3) - Several TAs may be given in $(EARLY_TA_PATHS) (3) Early TAs are given a higher load priority than REE FS TAs, since they should be available even before tee-supplicant is ready. Suggested-by: Zeng Tao <prime.zeng@hisilicon.com> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Since the parent commit ("Add support for early Trusted Applications"), the link step of tee.elf may pull object files generated from Trusted Application binaries. This is controlled by $(EARLY_TA_PATHS). Adding or removing files should cause tee.elf to be re-linked, even when no re-compilation or change in the configuration variables occur. This is not the case currently. For example: $ make EARLY_TA_PATHS="a.elf b.elf" $ make EARLY_TA_PATHS="a.elf" # Should re-link without b.elf The link recipe is modified so that the link step is forced when the object list changes. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Import the decompression code from zlib v1.2.11. From the project's README: "zlib 1.2.11 is a general purpose data compression library. [...] The data format used by the zlib library is described by RFCs (Request for Comments) 1950 to 1952 in the files rfc1950 (zlib format), rfc1951 (deflate format) and rfc1952 (gzip format)." This code will be used in a later commit to decompress early TAs. Only the inflate() function is needed, and the library is configured without gzip support. The source files that are not required for inflate() are left aside. The library is licensed under a permissive license, see `zlib.h`. Link: http://tools.ietf.org/html/rfc1950 Link: http://tools.ietf.org/html/rfc1951 Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Add decompression code to the early TA loader and update the Python script accordingly. The compression algorithm is "deflate", which is used by zlib and gzip in particular. It allows for compression ratios comprised between 3 (for bigger TAs) and 4.7 (for smaller ones). Those numbers were observed with 32-bit TAs (QEMU). On QEMU (armv7), the code size overhead when CFG_EARLY_TA=y, including the decompressor, is 12K when DEBUG=0 or 20K when DEBUG=1. The decompressor allocates about 39K of heap. Another library compatible with zlib was tried for comparison [1]. The code size overhead with miniz was 8K (DEBUG=0) or 16K (DEBUG=1). On the other hand, the dynamic allocation was about 43K, so the total memory required was about same. Speed was not compared. In the end, zlib was preferred for licensing reasons and because it is widely used. Link: [1] https://github.com/richgel999/miniz Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (QEMU) Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (QEMUv8, pager) Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (D02 32/64 bits) Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (D02 32/64 bits, pager) Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

igoropaniuk reviewed Aug 4, 2017

View reviewed changes

jforissier force-pushed the early-ta branch 2 times, most recently from 55a838e to 9ec0c57 Compare August 4, 2017 13:08

etienne-lms reviewed Aug 7, 2017

View reviewed changes

etienne-lms reviewed Aug 8, 2017

View reviewed changes

etienne-lms reviewed Aug 11, 2017

View reviewed changes

airbak reviewed Aug 12, 2017

View reviewed changes

airbak reviewed Aug 14, 2017

View reviewed changes

vchong mentioned this pull request Aug 16, 2017

How do I control GPIO? #944

Closed

jenswi-linaro reviewed Aug 17, 2017

View reviewed changes

etienne-lms reviewed Aug 22, 2017

View reviewed changes

jforissier force-pushed the early-ta branch from 35bb748 to 22469c5 Compare August 22, 2017 13:24

jenswi-linaro reviewed Aug 22, 2017

View reviewed changes

jforissier force-pushed the early-ta branch from 0ab95ee to 0f96189 Compare August 23, 2017 12:58

core/sub.mk: add missing dependency on the TA key processing script

2d6d361

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

jforissier added 8 commits August 25, 2017 14:51

jforissier force-pushed the early-ta branch from 0f96189 to 485d56d Compare August 25, 2017 12:53

jforissier merged commit 509a980 into OP-TEE:master Aug 25, 2017

jforissier deleted the early-ta branch August 25, 2017 15:27

jforissier mentioned this pull request Oct 6, 2017

Static TA cannot use TEE_* api -- a patch proposal for enabling "Static User TA" linked into optee #1295

Closed

etienne-lms mentioned this pull request Apr 14, 2021

SPM: Add SPMC documentation OP-TEE/optee_docs#107

Merged

Early TAs #1733

Early TAs #1733

Conversation

jforissier commented Aug 4, 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

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

igoropaniuk Aug 4, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier Aug 17, 2017 • edited Loading

igoropaniuk Aug 4, 2017 •

edited

Loading

etienne-lms Aug 8, 2017 •

edited

Loading

jforissier Aug 17, 2017 •

edited

Loading