tee-supplicant: Support multiple ta load paths by chaohub · Pull Request #306 · OP-TEE/optee_client

chaohub · 2022-04-18T17:31:01Z

CFG_TEE_CLIENT_LOAD_PATH can have multiple paths separated by :

jenswi-linaro · 2022-04-19T06:12:55Z

tee-supplicant/src/teec_ta_load.c

+	}
+
+	tofree = multi_path;
+	while ((single_path = strsep(&multi_path, ":")) != NULL) {


This parsing could be done once at startup in order to avoid doing this strdup(), strsep(), free() sequence each time a TA is to be loaded.

github-actions · 2022-05-20T02:19:24Z

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

etienne-lms · 2022-05-24T12:31:11Z

this P-R could be tags as an enhancement.

chaohub · 2022-05-24T18:29:11Z

I will address the comments and update the change.

etienne-lms

Thanks for the update.
Please fix commit message to also mention ~~TEEC_LOAD_PATH~~ TEEC_TEST_LOAD_PATH and add your Signed-off-by tag.
(edited)

etienne-lms · 2022-06-16T06:09:04Z

tee-supplicant/src/teec_ta_load.c

-				      dev_path, destination, ta, ta_size);
+	for (int i = 0; i < num_ta_prefix; i++) {
+		res = try_load_secure_module(ta_prefix[i],
+				dev_path, destination, ta, ta_size);


fix indentation

res = try_load_secure_module(ta_prefix[i], dev_path, destination, ta, ta_size);

etienne-lms · 2022-06-16T06:09:48Z

tee-supplicant/src/teec_ta_load.h

 #define TA_BINARY_FOUND 0
 #define TA_BINARY_NOT_FOUND -1

+/* support multiple TA load path */


nitpicking: s/support/Support/

etienne-lms · 2022-06-16T06:10:26Z

tee-supplicant/src/teec_ta_load.h

 #define TA_BINARY_NOT_FOUND -1

+/* support multiple TA load path */
+#define MAX_TA_PREFIX 8


It's likely there will be less than 8 alternate dir paths, however it would be more flexible to remove that limitation and allocated ta_prefix[] dynamically.

Pre-allocate enough entries is efficient if the number of entries is not large. I doubt the practicality for cases with more than 8 paths. Adding code and runt time dynamic allocation for virtually non-exist case, it's kind of over kill.

It would be better to not have built-in limitation if not strictly needed (IMHO).
I'll rely on maintainers point of view.

That's fair. Let the maintainers to pick which way to go.

Please pick a dynamic solution.

etienne-lms · 2022-06-16T06:10:45Z

tee-supplicant/src/tee_supplicant.c

 			 RPC_NUM_PARAMS * sizeof(struct tee_ioctl_param))

+char *ta_prefix[MAX_TA_PREFIX] = {0};
+int num_ta_prefix = 0;


You can (should) remove the initialization value when: global variables are default zero-initialized.

I will remove the values.

CFG_TEE_CLIENT_LOAD_PATH can have multiple paths separated by :

jforissier

Many comments from me below 😉 I propose the following instead: #311.

jforissier · 2022-06-24T08:01:04Z

tee-supplicant/src/tee_supplicant.c

+	i = 0;
+#ifdef TEEC_TEST_LOAD_PATH
+	test_ta_prefix_multipath = strdup(TEEC_TEST_LOAD_PATH);
+	if (!test_ta_prefix) {


test_ta_prefix_multipath

jforissier · 2022-06-24T08:04:43Z

tee-supplicant/src/tee_supplicant.c

+	}
+	while ((temp = strsep(&test_ta_prefix_multipath, ":")) != NULL) {
+		ta_prefix[i++] = temp;
+	}


ta_prefix[i] may be an empty string if multiple colons are found in a row.

jforissier · 2022-06-24T08:05:37Z

tee-supplicant/src/tee_supplicant.c

 		exit(EXIT_FAILURE);
 	}

+	/* Support multiple ta load paths */


I'd rather have a helper function to parse the paths. There is duplication.

jforissier · 2022-06-24T08:06:24Z

config.mk

@@ -31,6 +31,8 @@ CFG_TEE_CLIENT_LOG_FILE ?= $(CFG_TEE_FS_PARENT_PATH)/teec.log

 # CFG_TEE_CLIENT_LOAD_PATH
 #   The location of the client library file.


This line is just wrong, it should be removed. The same error is present in CMakeLists.mk.

jforissier · 2022-06-24T08:08:15Z

tee-supplicant/src/teec_ta_load.c

-	return try_load_secure_module(TEEC_LOAD_PATH,
-				      dev_path, destination, ta, ta_size);
+	for (int i = 0; i < num_ta_prefix; i++) {
+		res = try_load_secure_module(ta_prefix[i],


Note, the documentation for try_load_secure_module() lacks the description of its first two arguments.

jforissier · 2022-06-24T08:11:54Z

tee-supplicant/src/tee_supplicant.c

+exit:
+	free(test_ta_prefix_multipath);
+	free(ta_prefix_multipath);
+	free(ta_prefix);


I'm not convinced this kind of cleanup is desirable immediately before exiting.

jforissier · 2022-06-24T08:16:33Z

tee-supplicant/src/tee_supplicant.c

 	int long_index = 0;
 	int opt = 0;
+	char *test_ta_prefix_multipath = NULL;
+	char *ta_prefix_multipath = NULL;


The purpose of the variables is a bit hard to figure out from their names IMO.

jenswi-linaro reviewed Apr 19, 2022

View reviewed changes

github-actions bot added the Stale label May 20, 2022

jforissier added the enhancement label May 24, 2022

github-actions bot removed the Stale label May 25, 2022

chaohub force-pushed the multipath branch from d34b139 to 76dde66 Compare June 15, 2022 23:58

etienne-lms suggested changes Jun 16, 2022

View reviewed changes

tee-supplicant: Support multiple ta load paths

661e593

CFG_TEE_CLIENT_LOAD_PATH can have multiple paths separated by :

chaohub force-pushed the multipath branch from 76dde66 to 661e593 Compare June 22, 2022 23:27

jforissier reviewed Jun 24, 2022

View reviewed changes

		@@ -31,6 +31,8 @@ CFG_TEE_CLIENT_LOG_FILE ?= $(CFG_TEE_FS_PARENT_PATH)/teec.log

		# CFG_TEE_CLIENT_LOAD_PATH
		# The location of the client library file.

Conversation

chaohub commented Apr 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 20, 2022

Uh oh!

etienne-lms commented May 24, 2022

Uh oh!

chaohub commented May 24, 2022

Uh oh!

etienne-lms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jforissier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

etienne-lms left a comment •

edited

Loading