xtest: add --install-tas argument #240

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

Adds --install-tas argument which will install all bootstrap TAs
(/lib/optee_armtz/*.bsta) in the OP-TEE TA database.

Signed-off-by: Jens Wiklander jens.wiklander@linaro.org

Needed by OP-TEE/optee_os#1928 to install bootstrap TAs.

jbech-linaro · 2017-11-09T16:15:18Z

Seems to be unhappy about the pta_management.h
--- edit saw, the dependency in the other commit. ---

jenswi-linaro · 2017-11-09T16:20:00Z

Thanks @jbech-linaro, so there's a mutual dependency between the two PRs.

etienne-lms · 2017-11-21T16:09:06Z

host/xtest/xtest_main.c

@@ -65,6 +66,7 @@ void usage(char *program)
 	printf("applets:\n");
 	printf("\t--sha-perf [opts]  SHA performance testing tool (-h for usage)\n");
 	printf("\t--aes-perf [opts]  AES performance testing tool (-h for usage)\n");
+	printf("\t--install-tas [opts] Install TAs (-h for usage)\n");


minor: remove [opts] and (-h for usage)

etienne-lms · 2017-11-21T16:12:15Z

host/xtest/install_tas.c

+	TEEC_UUID uuid = PTA_MANAGEMENT_UUID;
+	TEEC_Context ctx;
+	TEEC_Session sess;
+	const char *ta_dir = "/lib/optee_armtz";


#ifndef TA_DIR # ifdef __ANDROID__ #define TA_DIR "/system/lib/optee_armtz" # else #define TA_DIR "/lib/optee_armtz" # endif #endif

-> see regression_1000.c
const char *ta_dir = TA_DIR;

jenswi-linaro · 2017-11-22T13:37:49Z

Update

jenswi-linaro · 2017-11-24T19:51:38Z

Update to pass all test together with OP-TEE/optee_os#1928

jforissier · 2017-11-29T10:31:32Z

It would be more flexible as xtest --install-ta <ta-path>, because TA_DIR is not relevant for secstor TAs, and you may not want to install all TAs in a given directory.

jforissier · 2017-11-29T10:45:13Z

In noticed that if I install the TAs several times, more objects keep being added into the secure storage (/data/tee/<some_number>.ta). Is that expected?

jenswi-linaro · 2017-11-29T12:19:59Z

Thanks
I'll update the --install-ta command
It's not expected that objects should be added all the time, I'll fix.

jenswi-linaro · 2017-11-29T13:41:24Z

Update

jforissier

In "regression: remove 9xxx test series", you should say why you are removing it.
--install-ta should be enabled only if CFG_SECSTOR_TA_MGMT_PTA=y
Since "legacy" TAs are till supported, i'd rather not remove regression_1008. And load_corrupt_ta that your are modifying here should be a new function install_corrupt_ta instead.

jforissier · 2017-11-29T13:45:09Z

host/xtest/install_ta.c

+
+		if (S_ISDIR(sb.st_mode))
+			install_dir(&sess, argv[i]);
+		else if (S_ISREG(sb.st_mode))


Nitpick: S_ISLNK is probably OK, too

That's why I'm using stat() instead of lstat()

Ah, right, I missed that. It's fine then.

jforissier · 2017-11-29T13:55:52Z

host/xtest/xtest_main.c

@@ -66,7 +66,8 @@ void usage(char *program)
 	printf("applets:\n");
 	printf("\t--sha-perf [opts]  SHA performance testing tool (-h for usage)\n");
 	printf("\t--aes-perf [opts]  AES performance testing tool (-h for usage)\n");
-	printf("\t--install-tas      Install TAs\n");
+	printf("\t--install-ta [directory or list of TAs]\n");


Nitpicking: several directories may be given, and the naming convention should be explained (<dir>/*.ta).
How about:

--install-ta DIR|FILE... Install Trusted Application(s). If DIR is given, all files matching DIR/*.ta are installed.

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

host/xtest/install_ta.c

+	res = TEEC_InvokeCommand(sess, PTA_SECSTOR_TA_MGMT_BOOTSTRAP, &op,
+				 &err_origin);
+	if (res)
+		errx(1, "install_ta: TEEC_InvokeCommand: %#" PRIx32 " err_origin %#" PRIx32, res, err_origin);


jenswi-linaro · 2017-11-29T14:29:20Z

If "legacy" testing is to be kept in 1008, what are supposed to do when the uuid of the TA we're testing with is installed in the tadb and the TA isn't fetch from REE FS TA storage?

jenswi-linaro · 2017-11-29T14:34:23Z

Update

jforissier · 2017-11-29T14:47:10Z

If "legacy" testing is to be kept in 1008, we do have to make sure the tested UUIDs are not installed in the tadb. AFAICT, there are only two UUIDs:

fake_uuid (used in load_fake_ta(), this one is not a problem because it does not exist as a TA UUID
create_fail_test_ta_uuid (used in load_corrupt_ta()). The TA file is c3f6e2c0-3548-11e1-b86c-0800200c9a66.ta. Since this is all in xtest, maybe we could make --install-ta DIR ignore DIR/c3f6e2c0-3548-11e1-b86c-0800200c9a66.ta?

jforissier · 2017-11-30T07:48:07Z

We also need --uninstall-ta. I can propose some code if you want. --list-ta would be good also but maybe not as important.

jenswi-linaro · 2017-11-30T10:42:53Z

Why is the legacy testing so important, in the end the same code is tested (core/crypto/signed_hdr.c)?

PTA_SECSTOR_TA_MGMT currently only supports installing TAs, not listing or uninstalling. I'm not sure that will ever be exposed directly to normal world either.

jforissier · 2017-11-30T14:38:45Z

Why is the legacy testing so important, in the end the same code is tested (core/crypto/signed_hdr.c)?

Maybe it's not.

PTA_SECSTOR_TA_MGMT currently only supports installing TAs, not listing or uninstalling. I'm not sure that will ever be exposed directly to normal world either.

OK, but an uninstall option is still useful for testing. Case in point:

root@HiKey960:/ U=cb3e5ba0-adf1-11e0-998b-0002a5d5c51b; \
  for i in `seq 1 1500`; do \
    echo >&2 $i && \
    xtest --install-ta /lib/optee_armtz/$U.ta && \
    xtest --uninstall-ta $U; \
    if [ $? -ne 0 ]; then \
      break; \
    fi; \
  done >/dev/null
[...]
1006
1007
1008
1009
xtest: install_ta: TEEC_InvokeCommand: 0xffff000c err_origin 0x4

This tells me we have a memory leak somewhere :(

jbech-linaro · 2017-11-30T15:00:55Z

This tells me we have a memory leak somewhere :(

Try it using Valgrind, it has helped me look at various OP-TEE components in the past, including xtest (If you have no rootfs including it, let me know and I can share something with you).

jforissier · 2017-11-30T15:03:57Z

Valgrind on the TEE core? ;-)

jbech-linaro · 2017-11-30T15:17:35Z

Valgrind on the TEE core? ;-)

Ah, sorry, that could be harder 😅 ... I was reading it as you suspected a memory leak from xtest, but that is of course not the case here.

jenswi-linaro · 2017-11-30T15:30:40Z

Memory leak plugged in #1928

jenswi-linaro · 2017-11-30T15:34:01Z

Next step is to add a test TA handles a security domain. That TA will be able to uninstall TAs etc. The reason I don't want it directly available to normal world is that it would be a nice security hole that would be enabled with testing enabled or something.

jforissier · 2017-11-30T16:10:10Z

Memory leak plugged in #1928

Confirmed.

[uninstall] The reason I don't want it directly available to normal world is that it would be a nice security hole that would be enabled with testing enabled or something.

As long as we have a simple way to exercise the install/uninstall code, it's fine.

jenswi-linaro · 2017-11-30T16:50:15Z

As long as we have a simple way to exercise the install/uninstall code, it's fine.

We will not have it in this PR, but hopefully the next.

jforissier · 2017-11-30T17:30:30Z

@jenswi-linaro sure, that was my understanding. Thanks.

jforissier

One additional comment about TA_TEST_DIR. Pending issues:

In "regression: remove 9xxx test series", you should say why you are removing it.
--install-ta should be enabled only if CFG_SECSTOR_TA_MGMT_PTA=y

jforissier · 2017-12-04T07:22:44Z

host/xtest/xtest_test.h

+# endif
+#endif
+
+#ifndef TA_TEST_DIR


Looks like TA_TEST_DIR is not used anymore

It's still used in host/xtest/regression_1000.c. Should I move it back to host/xtest/regression_1000.c again?

It's not used in the HEAD of this PR, right?

$ git fetch origin pull/240/head From https://github.com/OP-TEE/optee_test * branch refs/pull/240/head -> FETCH_HEAD $ git checkout FETCH_HEAD HEAD is now at 80b8e5941579... [review] xtest: add --install-tas argument $ git grep TA_TEST_DIR host/xtest/xtest_test.h:#ifndef TA_TEST_DIR host/xtest/xtest_test.h:# define TA_TEST_DIR "/data/tee/optee_armtz" host/xtest/xtest_test.h:# define TA_TEST_DIR "/tmp/optee_armtz"

So I think it should remain in host/xtest/regression_1000.c until it finally gets removed by "regression 1008: test with corrupt BSTA".

Aha, sorry I confused it with TA_DIR.

jenswi-linaro · 2017-12-04T10:06:40Z

Addressed the comments, except the one about TA_TEST_DIR.
Rebased and squashed

jenswi-linaro · 2017-12-04T10:17:36Z

Rebased and addressed the TA_TEST_DIR stuff too.

jbech-linaro · 2017-12-04T10:21:24Z

How does this affect the AOSP builds? I theory it should just work I guess, but it'd be good to have some confidence that it still works before pushing this. Maybe Yongqin or Vee could help out testing.

vchong · 2017-12-04T10:34:11Z

I'll run a test later today and report back.

jforissier · 2017-12-04T10:32:48Z

host/xtest/install_ta.c

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


jforissier · 2017-12-04T10:41:09Z

host/xtest/regression_1000.c

@@ -31,6 +32,11 @@
 #include <ta_sims_test.h>
 #include <ta_concurrent.h>
 #include <sdp_basic.h>
+#include <pta_secstor_ta_mgmt.h>


Missing #ifdef

vchong · 2017-12-04T15:01:02Z

Please add changes in host/xtest/Makefile to Android.mk too. Thanks!

vchong · 2017-12-05T11:25:31Z

Sorry for the delay. Had several build errors to fix. So:

xtest -> ok
xtest --install-ta /system/lib/optee_armtz -> ok

hikey:/ # xtest --install-ta /system/lib/optee_armtz                                                                   
Searching directory "/system/lib/optee_armtz" for TAs                                                                  
Installing "12345678-5b69-11e4-9dbb-101f74f00099.ta"                                                                   
Installing "484d4143-2d53-4841-3120-4a6f636b6542.ta"                                                                   
Installing "5b9e0e40-2636-11e1-ad9e-0002a5d5c51b.ta"                                                                   
Installing "5ce0c432-0ab0-40e5-a056-782ca0e6aba2.ta"                                                                   
Installing "5dbac793-f574-4871-8ad3-04331ec17f24.ta"                                                                   
Installing "614789f2-39c0-4ebf-b235-92b32ac107ed.ta"                                                                   
Installing "731e279e-aafb-4575-a771-38caa6f0cca6.ta"                                                                   
Installing "873bcd08-c2c3-11e6-a937-d0bf9c45c61c.ta"                                                                   
Installing "8aaaf200-2450-11e4-abe2-0002a5d5c51b.ta"                                                                   
Installing "b689f2a7-8adf-477a-9f99-32e90c0ad0a2.ta"                                                                   
Installing "b6c53aba-9669-4668-a7f2-205629d00f86.ta"                                                                   
Installing "c3f6e2c0-3548-11e1-b86c-0800200c9a66.ta"                                                                   
Installing "cb3e5ba0-adf1-11e0-998b-0002a5d5c51b.ta"                                                                   
Installing "d17f73a0-36ef-11e1-984a-0002a5d5c51b.ta"                                                                   
Installing "e13010e0-2ae1-11e5-896a-0002a5d5c51b.ta"                                                                   
Installing "e626662e-c0e2-485c-b8c8-09fbce6edf3d.ta"                                                                   
Installing "e6a33ed4-562b-463a-bb7e-ff5e15a493c8.ta"                                                                   
Installing "f157cda0-550c-11e5-a6fa-0002a5d5c51b.ta"                                                                   
Installing TAs done

xtest -> ok
Repeat 2 and 3 -> ok
xtest --install-ta <foo_uuid>.ta -> ok
xtest --install-ta <bar_uuid>.ta -> ok
xtest --install-ta /system/lib/optee_armtz/*.ta

Installing "/system/lib/optee_armtz/12345678-5b69-11e4-9dbb-101f74f00099.ta"                                           
Installing "/system/lib/optee_armtz/484d4143-2d53-4841-3120-4a6f636b6542.ta"                                           
Installing "/system/lib/optee_armtz/5b9e0e40-2636-11e1-ad9e-0002a5d5c51b.ta"                                           
Installing "/system/lib/optee_armtz/5ce0c432-0ab0-40e5-a056-782ca0e6aba2.ta"                                           
Installing "/system/lib/optee_armtz/5dbac793-f574-4871-8ad3-04331ec17f24.ta"                                           
Installing "/system/lib/optee_armtz/614789f2-39c0-4ebf-b235-92b32ac107ed.ta"                                           
Installing "/system/lib/optee_armtz/731e279e-aafb-4575-a771-38caa6f0cca6.ta"                                           
Installing "/system/lib/optee_armtz/873bcd08-c2c3-11e6-a937-d0bf9c45c61c.ta"                                           
Installing "/system/lib/optee_armtz/8aaaf200-2450-11e4-abe2-0002a5d5c51b.ta"                                           
Installing "/system/lib/optee_armtz/b689f2a7-8adf-477a-9f99-32e90c0ad0a2.ta"                                           
Installing "/system/lib/optee_armtz/b6c53aba-9669-4668-a7f2-205629d00f86.ta"                                           
Installing "/system/lib/optee_armtz/c3f6e2c0-3548-11e1-b86c-0800200c9a66.ta"                                           
Installing "/system/lib/optee_armtz/cb3e5ba0-adf1-11e0-998b-0002a5d5c51b.ta"                                           
Installing "/system/lib/optee_armtz/d17f73a0-36ef-11e1-984a-0002a5d5c51b.ta"                                           
Installing "/system/lib/optee_armtz/e13010e0-2ae1-11e5-896a-0002a5d5c51b.ta"                                           
Installing "/system/lib/optee_armtz/e626662e-c0e2-485c-b8c8-09fbce6edf3d.ta"                                           
Installing "/system/lib/optee_armtz/e6a33ed4-562b-463a-bb7e-ff5e15a493c8.ta"                                           
Installing "/system/lib/optee_armtz/f157cda0-550c-11e5-a6fa-0002a5d5c51b.ta"                                           
Installing TAs done

xtest -> ok

jenswi-linaro · 2017-12-05T13:25:45Z

Added (untested) Android patches

vchong · 2017-12-05T13:45:22Z

Added (untested) Android patches

That's what I have so should be ok. Thanks!

jenswi-linaro · 2017-12-05T13:49:12Z

Do I smell a tested-by tag? ;-)

vchong · 2017-12-05T13:54:34Z

Continued testing from #240 (comment)

rm -rf /data/tee/* -> ok
adb push *.ta files created from an old (but new enough) manifest build to /system/lib/optee_armtz and rerun xtest -> ok

vchong · 2017-12-05T13:55:45Z

Do I smell a tested-by tag? ;-)

Oops, forgot. :)

Tested-by: Victor Chong <victor.chong@linaro.org> (hikey aosp)

jenswi-linaro · 2017-12-07T06:44:05Z

Squashed and tag applied

jforissier · 2017-12-07T07:22:49Z

host/xtest/install_ta.c

+#include <errno.h>
+#include <fnmatch.h>
+#include <inttypes.h>
+#include <pta_secstor_ta_mgmt.h>


Build regression not addressed

Don't just read/say that include too quickly 😄

Ha! Got it 😉

jforissier · 2017-12-07T09:02:06Z

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Adds --install-tas argument which will install all bootstrap TAs (/lib/optee_armtz/*.bsta) in the OP-TEE TA database. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Removes the 9xxx test series as there's new internal tests in OP-TEE testing this instead. Also when TAs are stored in secure storage all the file these tests depends on will changed in an even more unpredictable way. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Removes the load_fake_ta() test, it's not applicable with bootstrap TAs. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Updates case 1008 to corrupt bootstrap TAs instead. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Tested-by: Victor Chong <victor.chong@linaro.org> (hikey aosp) Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2017-12-07T09:04:58Z

Tag applied

jenswi-linaro mentioned this pull request Nov 9, 2017

[RFC] Bootstrap ta OP-TEE/optee_os#1928

Merged

etienne-lms reviewed Nov 21, 2017

View reviewed changes

jforissier reviewed Nov 29, 2017

View reviewed changes

jforissier reviewed Dec 4, 2017

View reviewed changes

jenswi-linaro force-pushed the ta_mgt branch from 80b8e59 to 07f3b19 Compare December 4, 2017 10:05

jenswi-linaro force-pushed the ta_mgt branch from 07f3b19 to 8d7f12d Compare December 4, 2017 10:17

jforissier reviewed Dec 4, 2017

View reviewed changes

jenswi-linaro force-pushed the ta_mgt branch 2 times, most recently from 216cb12 to 3a79af5 Compare December 7, 2017 06:43

jforissier reviewed Dec 7, 2017

View reviewed changes

jenswi-linaro force-pushed the ta_mgt branch from 3a79af5 to c3188b1 Compare December 7, 2017 08:52

jenswi-linaro added 4 commits December 7, 2017 10:04

xtest: add --install-tas argument

7d38e9d

Adds --install-tas argument which will install all bootstrap TAs (/lib/optee_armtz/*.bsta) in the OP-TEE TA database. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

regression 1008: remove load_fake_ta() test

7fbdab9

Removes the load_fake_ta() test, it's not applicable with bootstrap TAs. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

regression 1008: test with corrupt BSTA

523fc94

Updates case 1008 to corrupt bootstrap TAs instead. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Tested-by: Victor Chong <victor.chong@linaro.org> (hikey aosp) Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro force-pushed the ta_mgt branch from c3188b1 to 523fc94 Compare December 7, 2017 09:04

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

jenswi-linaro deleted the ta_mgt branch December 7, 2017 09:29

xtest: add --install-tas argument #240

xtest: add --install-tas argument #240

Conversation

jenswi-linaro commented Nov 9, 2017

jbech-linaro commented Nov 9, 2017 • edited Loading

jenswi-linaro commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 22, 2017

jenswi-linaro commented Nov 24, 2017

jforissier commented Nov 29, 2017

jforissier commented Nov 29, 2017

jenswi-linaro commented Nov 29, 2017

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

jenswi-linaro commented Nov 29, 2017 • edited Loading

jenswi-linaro commented Nov 29, 2017

jforissier commented Nov 29, 2017

jforissier commented Nov 30, 2017

jenswi-linaro commented Nov 30, 2017

jforissier commented Nov 30, 2017

jbech-linaro commented Nov 30, 2017

jforissier commented Nov 30, 2017

jbech-linaro commented Nov 30, 2017

jenswi-linaro commented Nov 30, 2017

jenswi-linaro commented Nov 30, 2017

jforissier commented Nov 30, 2017

jenswi-linaro commented Nov 30, 2017

jforissier commented Nov 30, 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

jenswi-linaro commented Dec 4, 2017

jenswi-linaro commented Dec 4, 2017

jbech-linaro commented Dec 4, 2017 • edited Loading

vchong commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchong commented Dec 4, 2017

vchong commented Dec 5, 2017 • edited Loading

jenswi-linaro commented Dec 5, 2017

vchong commented Dec 5, 2017

jenswi-linaro commented Dec 5, 2017

vchong commented Dec 5, 2017

vchong commented Dec 5, 2017

jenswi-linaro commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented Dec 7, 2017

jenswi-linaro commented Dec 7, 2017

jbech-linaro commented Nov 9, 2017 •

edited

Loading

jenswi-linaro commented Nov 29, 2017 •

edited

Loading

jbech-linaro commented Dec 4, 2017 •

edited

Loading

vchong commented Dec 5, 2017 •

edited

Loading