[RFC] Bootstrap ta #1928

jenswi-linaro · 2017-11-09T15:55:25Z

This pull request introduces bootstrap TAs which can completely replace the current TAs depending on how we'd like to progress.

I see some ways forward:

Add this as yet another method of loading TAs
Replace the current way of loading TAs from supplicant with this
Wait until we have some security domain in place before starting to retire/deprecate 1

Doing 2 (or 3) doesn't mean that we have to remove the old code, we could just make it optional. But we'd change our current test setup to only install TAs via bootstrap TAs (or via a security domain).

Bootstrap TA are basically only the solution to the chicken and egg problem for security domains. Next step would be to introduce a security domain.

jenswi-linaro · 2017-11-09T15:56:25Z

Uses OP-TEE/optee_test#240 to install bootstrap TAs

Note that the 9000 tests will fail if bootstrap TAs are installed.

lorc · 2017-11-09T16:02:22Z

Hi @jenswi-linaro, just a quick question: what do you mean when you say "security domain"?

jenswi-linaro · 2017-11-09T16:19:22Z

Hi @lorc, this for instance: https://tools.ietf.org/html/draft-pei-opentrustprotocol-04#section-5.3

Note that Security Domains doesn't to have provide isolation between TAs in different Security Domains. A Security Domain is just a TA management domain.

lorc · 2017-11-09T16:06:53Z

scripts/sign_bsta.py

@@ -0,0 +1,77 @@
+#!/usr/bin/env python


Have you tested this script in both python2 and python3?

I have now, and it seems to work. :-)

Okay then :-)

lorc · 2017-11-09T16:07:07Z

scripts/sign_bsta.py

@@ -0,0 +1,77 @@
+#!/usr/bin/env python
+#
+# Copyright (c) 2015, Linaro Limited


2017, I think :)

lorc · 2017-11-09T16:08:09Z

scripts/sign_bsta.py

+#
+
+def uuid_parse(s):
+	from uuid import UUID


Please check this script with pycodestyle (or with pip8.py). There will be a lot of warnings.

lorc · 2017-11-09T16:30:20Z

core/tee/tadb.c

+
+	if (db->files)
+		return TEE_SUCCESS;
+


Remove extra empty line.

lorc · 2017-11-09T16:31:24Z

core/tee/tadb.c

+
+	assert(db->refc);
+	db->refc--;
+	if (db->refc)


How you can guarantee thread safety in tadb manipulation?

These functions are supposed to be called from a static TA which will guarantee thread safety.

I just realized that these functions are used from core/arch/arm/kernel/tadb_store.c which is outside the pseudo TA context. Ideally I should have a read/write lock to allow concurrent loading of TAs.

lorc · 2017-11-09T16:39:04Z

core/tee/tadb.c

+	tadb_put(db);
+}
+
+static TEE_Result populate_files(struct tee_tadb_dir *db)


Purpose of this function is bit unclear to me...

I guess I thought this function was going to be used by several functions when doing the initial design. Now it's clear it's only needed in tee_tadb_ta_create(). I'll move the implementation into tee_tadb_ta_create() instead.

lorc · 2017-11-09T16:39:27Z

core/tee/tadb.c

+	size_t idx;
+	const TEE_UUID null_uuid = { 0 };
+
+	if (db->files)


There you return if db->files != NULL...

lorc · 2017-11-09T16:40:56Z

core/tee/tadb.c

+		if (!memcmp(&entry.prop.uuid, &null_uuid, sizeof(null_uuid)))
+			continue;
+
+		if (test_file(db, entry.file_number)) {


... and there you use db->files to check if file_number is stored. But at L265 you ensured that db->files is NULL.

As this is called in a loop, db->files could have been allocated at this point. test_file() and set_file() deals with that.

lorc · 2017-11-09T16:48:27Z

core/tee/tadb.c

+	int i = 0;
+
+	if (!ta)
+		res = TEE_ERROR_OUT_OF_MEMORY;


Looks like you missed return there.

lorc · 2017-11-09T16:59:19Z

core/tee/tadb.c

+		size_t num_bytes = 0;
+
+		while (num_bytes < l) {
+			uint8_t b[1024 * 4];


Are you really sure, you want to allocate 4kb on stack?

You're right, it's a bit excessive. I'll replace it with malloc().

lorc · 2017-11-09T17:21:57Z

core/arch/arm/pta/management.c

+	res = tee_hash_get_digest_size(TEE_DIGEST_HASH_TO_ALGO(shdr->algo),
+				       &hash_size);
+	if (res != TEE_SUCCESS)
+		return res;


return TEE_ERROR_SECURITY?

lorc · 2017-11-09T17:22:15Z

core/arch/arm/pta/management.c

+
+	res = crypto_ops.acipher.alloc_rsa_public_key(&key, shdr->sig_size);
+	if (res != TEE_SUCCESS)
+		return res;


return TEE_ERROR_SECURITY?

lorc · 2017-11-09T17:27:01Z

core/arch/arm/pta/management.c

+				     sizeof(*shdr));
+	if (res)
+		goto err_free_hash_ctx;
+


Remove extra empty line.

jenswi-linaro · 2017-11-14T12:24:11Z

Addressed comments, except then one with mutex (waiting for the outcome of #1942)

lorc · 2017-11-14T12:28:46Z

core/tee/tadb.c

+			continue;
+
+		if (test_file(db, entry.file_number)) {
+			DMSG("Clearing duplicate file number %" PRIu32,


If that is not going to happen, then EMSG or at least IMSG will fit much better.

lorc · 2017-11-14T12:39:11Z

core/tee/tadb.c

+
+		res = read_ent(db, idx, &entry);
+		if (res) {
+			if (res == TEE_ERROR_ITEM_NOT_FOUND)


It is confusing, that only one successful (apart from L336) return from this function resides in the middle of it.
Now, when I grasped idea of this function, this seems obvious. But it was hard to understand from a first try.

This is just a note. I don't ask you to perform any actions.

lorc · 2017-11-14T12:41:14Z

core/tee/tadb.c

@@ -670,17 +694,25 @@ TEE_Result tee_tadb_ta_read(struct tee_tadb_ta *ta, void *buf, size_t *len)
 			return res;
 	} else {
 		size_t num_bytes = 0;
+		size_t b_size = MIN(SIZE_4K, l - num_bytes);


You zero out num_bytes at the line above. Better to leave only MIN(SIZE_4k, l). It is easier to read.

lorc · 2017-11-14T12:43:07Z

scripts/sign_bsta.py

@@ -0,0 +1,77 @@
+#!/usr/bin/env python


Okay then :-)

etienne-lms · 2017-11-13T10:15:52Z

core/include/tee/tee_svc_storage.h

+ */
+const struct tee_file_operations *tee_svc_storage_file_ops(uint32_t storage_id);
+
+


minor: extra empty line.

etienne-lms · 2017-11-13T10:19:34Z

core/include/signed_hdr.h

@@ -71,5 +72,10 @@ struct shdr {
 #define SHDR_GET_HASH(x)	(uint8_t *)(((struct shdr *)(x)) + 1)
 #define SHDR_GET_SIG(x)		(SHDR_GET_HASH(x) + (x)->hash_size)

+struct shdr_bootstrap_ta {
+	uint8_t uuid[16];


minor: uuid[sizeof(TEE_UUID)] ?

etienne-lms · 2017-11-13T10:25:45Z

core/include/tee/tadb.h

+
+struct tee_tadb_ta;
+
+struct tee_tadb_property {


maybe add a comment to describe the structure fields.

etienne-lms · 2017-11-13T10:57:49Z

core/arch/arm/kernel/tadb_store.c

+static void secstor_ta_close(struct user_ta_store_handle *h)
+{
+	struct tee_tadb_ta *ta = (struct tee_tadb_ta *)h;
+	tee_tadb_ta_close(ta);


minor: add an empty line.

jenswi-linaro · 2017-11-17T09:02:19Z

Rebased and squashed on top of master to be able to use the read lock. It turned out atomic reference counting routines where needed too, added that to the beginning of the PR.

etienne-lms

few comments on the refcount part.
looking at the ta mngt services...

etienne-lms · 2017-11-21T08:51:09Z

core/include/kernel/refcount.h

+	unsigned int val;
+};
+
+/* Increases refcount by 1, return true if val != 0 else false */


I'm not sure I understand the Api. I read this as "Return true if incremented refcount is not 0.", in which caserefcount_inc() must be changed. If not, maybe clarify the Api.

Same kind of comments on refcount_dec().

You're right. I'll update the description.

etienne-lms · 2017-11-21T08:53:03Z

core/kernel/refcount.c

+	unsigned int oval = atomic_load_uint(&r->val);
+
+	while (true) {
+		nval = oval + 1;


I think we should read back in oval an updated value of recount val.

That's actually taken care of by atomic_cas_uint()

ok, right, sorry.

etienne-lms · 2017-11-21T13:21:51Z

core/tee/tadb.c

+	 * grow the bitfield as needed.
+	 *
+	 * At the same time clean out duplicate file numbers, the first
+	 * entry with the file number has precedence.  Duplicate entries is


minor: double space beforeDuplicate and before supposed at next line.

etienne-lms · 2017-11-21T13:28:00Z

core/tee/tadb.c

+#include <tee/tee_svc_storage.h>
+#include <utee_defines.h>
+
+#define TADB_MAX_BUFFER_SIZE	(64U * 1024)


where does this comes from?

We do need some value, 64KiB is nothing for normal world to allocate as shared memory.

etienne-lms · 2017-11-21T14:20:06Z

core/tee/tadb.c

+#define TADB_TAG_SIZE		TEE_AES_BLOCK_SIZE
+#define TADB_KEY_SIZE		TEE_AES_MAX_KEY_SIZE
+
+struct user_ta_store_handle;


minor: unused

etienne-lms · 2017-11-22T08:32:34Z

core/include/tee/tadb.h

+ *		TA binary
+ * @bin_size:	Size of the binary TA
+ * @is_ta:	True if this is a TA
+ * @is_root:	True if this is a root SD or a TA without an SD


It depends on how you pronounce it, I pronounce it ess-dee

etienne-lms · 2017-11-22T08:37:53Z

core/kernel/refcount.c

+	unsigned int oval = atomic_load_uint(&r->val);
+
+	while (true) {
+		nval = oval + 1;


ok, right, sorry.

etienne-lms · 2017-11-22T08:41:20Z

core/include/tee/tadb.h

+ */
+struct tee_tadb_property {
+	TEE_UUID uuid;
+	TEE_UUID parent;/* parent security domain, zeroes if nonexistent */


minor: tab before comment.

It doesn't fit on the line then. I'll remove it now that there's a description of the struct above.

etienne-lms · 2017-11-22T09:01:32Z

minor: could you rename the pTA "management" into sometihng like "TA management"? For example management.c being a bit too generic, even in pta/ subdir. Something like ta_management.c?

jenswi-linaro · 2017-11-22T10:07:58Z

Update

jforissier · 2017-11-27T10:48:51Z

In order to give an opinion on this RFC, I need to understand the big picture. How does it work (high-level)? What problem are you trying to address? What is different from what we are currently doing? What is a bootstrap TA? Why does it have a different extension as mentioned in the optee_test PR (*.bsta)? How do you generate a .bsta? Is a different signing key required? Are bootstrap TAs still stored in the REE FS? You're mentioning security domains and how this could be a solution to the chicken-and-egg problem SDs have. How?

Sorry I don't feel like trying to figure this out from code only :(

jenswi-linaro · 2017-11-27T12:36:07Z

Some description available here:
https://docs.google.com/document/d/1fIZkKNjSkaVjN0QsuJBxtJsU3T5GjtSayC-eXklm47Q/edit?usp=sharing

jforissier

Please see my comments but overall I like this proposal.
The documentation will need updating, too (optee_design.md, section "12. Trusted Applications", and BTW this section also lacks some description of the "early TA" feature I merged some time ago...)

jforissier · 2017-11-28T07:19:55Z

core/arch/arm/pta/ta_management.c

+	offs += sizeof(bs_ta);
+
+	memset(&property, 0, sizeof(property));
+	COMPILE_TIME_ASSERT(sizeof(property.uuid) == sizeof(property.uuid));


sizeof(bs_ta.uuid)

jforissier · 2017-11-28T07:42:27Z

ta/arch/arm/link.mk

+
+$(link-out-dir)/$(binary).bsta: $(link-out-dir)/$(binary).stripped.elf \
+				$(TA_SIGN_KEY)
+	@echo '  SIGN    $@'


@$(cmd-echo-silent) (and fix above)

jforissier · 2017-11-28T08:19:55Z

ta/arch/arm/link.mk

+$(link-out-dir)/$(binary).bsta: $(link-out-dir)/$(binary).stripped.elf \
+				$(TA_SIGN_KEY)
+	@echo '  SIGN    $@'
+	$(q)$(SIGN_BSTA) --key $(TA_SIGN_KEY) --uuid $(binary) --version 0 \


s/TA_SIGN_KEY/BSTA_SIGN_KEY/? (default BSTA_SIGN_KEY ?= $(TA_SIGN_KEY))

jforissier · 2017-11-28T08:32:08Z

ta/arch/arm/link.mk

 TA_SIGN_KEY ?= $(TA_DEV_KIT_DIR)/keys/default_ta.pem

 all: $(link-out-dir)/$(binary).elf $(link-out-dir)/$(binary).dmp \
-	$(link-out-dir)/$(binary).stripped.elf $(link-out-dir)/$(binary).ta
+	$(link-out-dir)/$(binary).stripped.elf $(link-out-dir)/$(binary).ta \
+	$(link-out-dir)/$(binary).bsta


Do we really want to always generate a .bsta? Of course it is convenient for testing, but normally only one format will be used. So we should make this optional. For instance, only if $(BOOTSTRAP_TA)=y. This variable would be set or not by the Makefile for each TA. And, it could also be set globally for instance when building xtest (make BOOSTRAP_TA=y). Update: following your email and your proposal to have only one TA header (the new bsta one), the last part of this comment is not really relevant. I agree one header format would be more convenient.

jforissier · 2017-11-28T08:39:30Z

core/arch/arm/kernel/sub.mk

@@ -2,6 +2,9 @@ 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
+
+$(call force,CFG_SECURE_STORAGE_TA,y,required by CFG_MANAGEMENT_PTA)
+srcs-$(CFG_SECURE_STORAGE_TA) += tadb_store.c


For consistency with ree_fs_ta.c and early_ta.c, tadb_store.c, how about secure_storage_ta.c?

jforissier · 2017-11-28T08:58:17Z

core/include/kernel/refcount.h

+ * When val becomes 0 refcount_inc() refuses to change the value any longer
+ * and returns false.
+ *
+ * refcount_dec() returns true becuase this call caused the value to become


sp
May also be good to say that the function decrements the vallue: "refcount_dec() decrements the value and returns true when the call caused the value to become 0, false otherwise", or something like that.

jforissier · 2017-11-28T09:03:05Z

core/kernel/refcount.c

+	while (true) {
+		nval = oval + 1;
+
+		/* r->val is 0, we can't doing anything more. */


s/doing/do/

jforissier · 2017-11-28T12:24:47Z

core/include/kernel/refcount.h

+
+/* Increases refcount by 1, return true if val > 0 else false */
+bool refcount_inc(struct refcount *r);
+/* Decreases refcount by 1, return true if val > 0 else false */


return true if val == 0

jforissier · 2017-11-28T12:29:49Z

core/include/kernel/refcount.h

+ * Reference conter
+ *
+ * When val becomes 0 refcount_inc() refuses to change the value any longer
+ * and returns false.


I find the wording a bit weird, a more neutral tone may be better: "When val is 0, refcount_inc() does not change the value and returns false.". You may add "Otherwise, it increments the value and returns true".

jforissier · 2017-11-28T12:34:50Z

core/tee/sub.mk

@@ -40,6 +40,9 @@ srcs-y += tee_obj.c
 srcs-y += tee_pobj.c
 srcs-y += tee_time_generic.c

+$(call force,CFG_SECURE_STORAGE_TA,y,required by CFG_MANAGEMENT_PTA)


Not needed here

jenswi-linaro · 2017-11-28T16:07:03Z

Squashed and inserted some extra commit but not rebased to make it easier to compare with the old a2aa4a0 state.

History is reordered to instead of adding a new sign script, the old one is replaced/updated and there's no .bsta file any longer. The REE FS TA storage now supports the new TA format as well as the old format.

I missed addressing the refcount comments, will make an update commit later. Documentation will also follow later.

jenswi-linaro · 2017-11-29T12:20:44Z

Fixes problem with old filenumbers not being reused.

jenswi-linaro · 2017-11-29T12:24:23Z

Addresses the refcount comments

jenswi-linaro · 2017-11-29T12:47:39Z

Added documentation in #1986

jforissier · 2017-11-29T12:17:55Z

core/tee/tadb.c

+	size_t rl = len;
+	struct tee_fs_rpc_operation op;
+
+	assert(tadb_check_state(ta, TEE_MODE_ENCRYPT));


Should return an error instead, I think.

This is an API usage error (programmers error). In the case of tee_tadb_ta_write(), tee_tadb_ta_read() and tee_tadb_ta_close_and_commit() it's possible to return an error, but the two functions tee_tadb_ta_close_and_delete(), tee_tadb_ta_close() are void functions.

In all of the cases there's no good cleanup action that the caller can perform.There's obviously some confusion regarding what the handle is to be used for. Initially I was using two different structs for the "read" class and "write" class of functions. Perhaps I should reintroduce that instead?

If there's no cleanup possible, then indeed a return code is useless.

Initially I was using two different structs for the "read" class and "write" class of functions. Perhaps I should reintroduce that instead?

Yes, I think I would like it better.

jforissier · 2017-11-29T12:18:03Z

core/tee/tadb.c

+
+void tee_tadb_ta_close_and_delete(struct tee_tadb_ta *ta)
+{
+	assert(tadb_check_state(ta, TEE_MODE_ENCRYPT));


jforissier · 2017-11-29T12:18:15Z

core/tee/tadb.c

+	size_t sz = sizeof(ta->entry.tag);
+	size_t idx;
+
+	assert(tadb_check_state(ta, TEE_MODE_ENCRYPT));


jforissier · 2017-11-29T12:18:52Z

core/tee/tadb.c

+	const size_t sz = ta->entry.prop.custom_size + ta->entry.prop.bin_size;
+	size_t l = MIN(*len, sz - ta->pos);
+
+	assert(tadb_check_state(ta, TEE_MODE_DECRYPT));


jforissier · 2017-11-29T12:19:07Z

core/tee/tadb.c

+
+void tee_tadb_ta_close(struct tee_tadb_ta *ta)
+{
+	assert(tadb_check_state(ta, TEE_MODE_DECRYPT));


jforissier · 2017-11-29T12:34:39Z

mk/config.mk

@@ -262,6 +262,12 @@ CFG_GP_SOCKETS ?= y
 # invocation parameters referring to specific secure memories).
 CFG_SECURE_DATA_PATH ?= n

+# Enable storage for TAs in secure storage
+CFG_SECSTOR_TA ?= y


Depends on CFG_REE_FS=y.

I find the comment slightly misleading, since the bulk of the TA is actually not in secure storage but directly stored in the REE FS. How about adding: "TA binaries are stored in the REE FS and are protected by metadata in secure storage." or something like that. Will make the dependancy on CFG_REE_FS=y obvious.

Updated comment is fine. About the dependency I meant:

CFG_SECSTOR_TA ?= $(CFG_REE_FS) $(call cfg-depends-all,CFG_SECSTOR_TA,CFG_REE_FS)

jforissier · 2017-11-29T13:16:47Z

scripts/sign.py

+    from argparse import ArgumentParser
+
+    parser = ArgumentParser()
+    parser.add_argument('--uuid', required=True,


Could be optional (default: parse --in file name).

On the other hand, the name of the TA file doesn't have to be an UUID. Sounds like complicated usage to me.

jforissier · 2017-11-29T13:17:39Z

ta/arch/arm/link.mk

@@ -58,4 +58,5 @@ $(link-out-dir)/$(binary).stripped.elf: $(link-out-dir)/$(binary).elf
 $(link-out-dir)/$(binary).ta: $(link-out-dir)/$(binary).stripped.elf \
 				$(TA_SIGN_KEY)
 	@echo '  SIGN    $@'
-	$(q)$(SIGN) --key $(TA_SIGN_KEY) --in $< --out $@
+	$(q)$(SIGN) --key $(TA_SIGN_KEY) --uuid $(binary) --version 0 \


Assuming --uuid is optional, the command could remain unchanged

jforissier · 2017-11-30T07:28:32Z

How about s/bootstrap/secure storage/g? My reasoning is: bootstrapping another TA management method (OTrP, etc.) is only one way this "secure storage TA" feature may be used. It is also perfectly valid to install all TAs in secure storage. So it would be more neutral and more general to use "secure storage TA", shdr_secstor_ta, and so on.

jforissier · 2017-11-30T07:44:31Z

core/include/tee/tadb.h

+	uint32_t custom_size;
+	uint32_t bin_size;
+	bool is_ta;
+	bool is_root;


Since parent, handler, is_ta and is_root are currently not used, I'd rather introduce them later. It is fine to mention in the commit log that this may be a first step towards supporting SDs, but it should not show in the code yet. The "secure storage TA" feature is useful on its own.

jenswi-linaro · 2017-11-30T09:34:40Z

I don't agree with s/bootstrap/secure storage/g. I don't expect this to be used for anything but bootstrapping in a product. Test usage is another story, there we'll bootstrap a lot :-)

I'm expecting this flow in a product:

bootstrap the TA management with a bootstrap TA containing keys needed for step 2 or bootstrap TA negotiates new keys
the initial bootstrap TA receives to domain management TA (in some format, probably not bootstrap at least) signed with the keys from step 1
the bootstrap TA creates a root security domain and installs the received TA as management TA
the bootstrap TA isn't used any longer now that there's a security domain up and running

As you see bootstrap signed TAs are only supposed to be used for just that, that they happen to be stored in secure storage has nothing to do with what they are. The main reason why a new format was needed is to be able to protect against downgrade attack of the bootstrap TA.

jenswi-linaro · 2017-12-01T12:12:34Z

@lorc, sorry, addressed the comments now.

jenswi-linaro · 2017-12-01T12:24:07Z

Squashed and rebased, for reference the old state was bef845d

jenswi-linaro · 2017-12-01T12:29:18Z

Fixed checkpatch error

jforissier

Why is xtest _9 failing in CI?

A few more comments here and there. Note that the SPDX/copyright stuff applies to a number of files. So, with my comments addressed:

For commit b70515f ("libutils: add atomic load, store and cas"):
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 648d206 ("core: add refcount routines"), with or without my comment addressed:
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 95e5e7a ("core: fs_htree: bugfix creating empty file"):
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 9c11ac0 ("core: provide tee_svc_storage_file_ops()"):
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 1ffb3c8 ("core: add shdr type SHDR_BOOTSTRAP_TA"): s/bootstram/bootstrap/ in the commit log, with that:
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 712ae81 ("core: crypto: add struct shdr helper functions")
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 6f59c74 ("core: ree fs ta store: use new shdr_*() helpers"):
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit cf8a284 ("core: ree fs ta store: support bootstrap TA format"):
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 7e18023 ("Sign TAs as bootstrap TAs"):
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 4447470 ("core: add tadb"):
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit b7659f9 ("core: add ta storage based on tadb"):
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For commit 6f8cc0f ("core: add management pseudo TA"): I recommend appending "for secstor TAs" to the commit subject, anyway:
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey960)

jforissier · 2017-12-01T14:17:07Z

core/kernel/refcount.c

+		/*
+		 * Note that atomic_cas_uint() updates oval to the current
+		 * r->val read, regardless of return value.
+		 */


Minor:

I was a bit disturbed by this comment, because:

We're only interested in what happens with oval when the function returns false.

When the function returns true, oval already has the value of r->val, so it is not really updated anyway

I'd suggest: /* At this point atomic_cas_uint() has updated oval to the current r->val */ after the if().

Same below.

jforissier · 2017-12-01T14:24:51Z

lib/libutils/ext/include/atomic.h

- * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
- * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
- * POSSIBILITY OF SUCH DAMAGE.
+ * SPDX-License-Identifier: BSD-2-Clause


Keep the license text and add the SPDX ID before.

jforissier · 2017-12-01T14:35:08Z

core/include/kernel/refcount.h

@@ -0,0 +1,71 @@
+/*
+ * Copyright (c) 2017, Linaro Limited
+ * All rights reserved.


jforissier · 2017-12-01T14:57:09Z

core/crypto/signed_hdr.c

+		return NULL;
+	memcpy(shdr, img, shdr_size);
+
+	/* Check that the data wasn't modified before the copy was completed */


Does this add any security? I fail to imagine a case where continuing would be a problem, since the TA is signed.

If shdr->hash_size or shdr->sig_size is increased we may access data outside the allocated buffer later on.

jforissier · 2017-12-01T15:21:37Z

scripts/sign.py

-# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
-# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
-# POSSIBILITY OF SUCH DAMAGE.
+# SPDX-License-Identifier: BSD-2-Clause


Keep the license text

jforissier · 2017-12-01T16:26:41Z

mk/config.mk

+# TA binaries are stored encrypted in the REE FS and are protected by
+# metadata in secure storage.
+CFG_SECSTOR_TA ?= $(call cfg-all-enabled,CFG_REE_FS CFG_WITH_USER_TA)
+$(eval $(call cfg-depends-all,CFG_SECSTOR_TA,CFG_REE_FS CFG_WITH_USER_TA))


These two lines should go into commit "core: add ta storage based on tadb", not in "core: add management pseudo TA".

jenswi-linaro · 2017-12-01T18:30:07Z

xtest _9 is failing because there's a secure storage object created for the TA database, this changes the names of the other secure storage objects. That's the main reason why I updated the htree tests a while ago, to be able to get rid of the xtest _9 tests.

jenswi-linaro · 2017-12-01T18:39:35Z

Comments, addressed, squashed and tags applied.

lorc · 2017-12-01T19:12:41Z

Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

* Adds atomic_load_uint() and atomic_load_u32() * Adds atomic_store_uint() and atomic_store_u32() * Adds atomic_cas_uint() and atomic_cas_u32(), compare and store Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds refcount_inc() and refcount_dec() Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Fixes problem with creating an empty htree file. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds a signed header type for bootstrap TA. This type is used when there isn't any security domains installed yet. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds struct shdr helper functions to allocate and verify a struct shdr. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Uses the new shdr_*() helper functions to verify a signed header. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds support for the new bootstrap TA format to the REE FS TA storage. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Changes to TA sign script to sign TAs as Bootstrap TAs (img_type == SHDR_BOOTSTRAP_TA) instead of the legacy TA format (img_type == SHDR_TA). Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds tadb which is a database in which TAs can be stored leveraging secure storage for anti-rollback, key storage and list of TAs. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds ta storage based on tadb. The TAs has to be installed in tadb before they can be loaded. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds a pseudo TA for management of Trusted Applications and Security Domains. The pseudo TA only provides a minimal interface, a more advanced interface is supposed to be provided by a user TA using this pseudo TA. Such a TA could for instance implement Global Platforms TEE Management Framework or OTrP. The management TA currently only supports installing bootstrap packaged TAs in secure storage. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey960) Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (QEMU) Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2017-12-02T07:31:55Z

Tag applied

jenswi-linaro · 2017-12-07T06:45:14Z

What's next step here?

jforissier · 2017-12-07T07:25:05Z

Since this breaks the xtest _9 series, I'd like to merge OP-TEE/optee_test#240 first.

jforissier · 2017-12-07T14:04:12Z

All good now. Thanks.

jenswi-linaro mentioned this pull request Nov 9, 2017

xtest: add --install-tas argument OP-TEE/optee_test#240

Merged

lorc reviewed Nov 9, 2017

View reviewed changes

jenswi-linaro mentioned this pull request Nov 14, 2017

Read/write mutex #1942

Merged

lorc reviewed Nov 14, 2017

View reviewed changes

etienne-lms reviewed Nov 15, 2017

View reviewed changes

jenswi-linaro force-pushed the bootstrap_ta branch from 8549015 to f4a4b84 Compare November 17, 2017 08:59

etienne-lms reviewed Nov 21, 2017

View reviewed changes

etienne-lms reviewed Nov 22, 2017

View reviewed changes

jforissier reviewed Nov 28, 2017

View reviewed changes

jenswi-linaro force-pushed the bootstrap_ta branch 2 times, most recently from deecfc3 to 87b4bff Compare November 28, 2017 16:06

jenswi-linaro mentioned this pull request Nov 29, 2017

Doc bootstrap ta #1986

Merged

jforissier reviewed Nov 29, 2017

View reviewed changes

jforissier reviewed Nov 30, 2017

View reviewed changes

jenswi-linaro force-pushed the bootstrap_ta branch from bef845d to 6f8cc0f Compare December 1, 2017 12:23

jforissier reviewed Dec 1, 2017

View reviewed changes

jenswi-linaro force-pushed the bootstrap_ta branch from 59f4fa9 to 2ff58f2 Compare December 1, 2017 18:39

jenswi-linaro added 12 commits December 2, 2017 08:30

core: add refcount routines

14408c9

Adds refcount_inc() and refcount_dec() Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: fs_htree: bugfix creating empty file

57a6ba2

Fixes problem with creating an empty htree file. Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: provide tee_svc_storage_file_ops()

4f15bf0

Reviewed-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro force-pushed the bootstrap_ta branch from 2ff58f2 to ff305df Compare December 2, 2017 07:31

jforissier merged commit 30668b2 into OP-TEE:master Dec 7, 2017

jenswi-linaro deleted the bootstrap_ta branch December 7, 2017 14:32

jforissier mentioned this pull request Nov 7, 2019

[RFC] Add framework to support REE-FS encrypted TAs #3340

Merged

jforissier mentioned this pull request Apr 22, 2020

[Question] How to provision/store keys in TA secure storage #3766

Closed

		*/
		const struct tee_file_operations *tee_svc_storage_file_ops(uint32_t storage_id);

[RFC] Bootstrap ta #1928

[RFC] Bootstrap ta #1928

Conversation

jenswi-linaro commented Nov 9, 2017

jenswi-linaro commented Nov 9, 2017

lorc commented Nov 9, 2017

jenswi-linaro commented Nov 9, 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

jenswi-linaro Nov 13, 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

jenswi-linaro commented Nov 14, 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

jenswi-linaro commented Nov 17, 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

etienne-lms commented Nov 22, 2017

jenswi-linaro commented Nov 22, 2017

jforissier commented Nov 27, 2017

jenswi-linaro commented Nov 27, 2017

jforissier 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

jenswi-linaro commented Nov 28, 2017

jenswi-linaro commented Nov 29, 2017

jenswi-linaro commented Nov 29, 2017

jenswi-linaro commented Nov 29, 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

jenswi-linaro Nov 13, 2017 •

edited

Loading