[RFC] tee-supplicant: add support for accessing custom REE service by divneil · Pull Request #170 · OP-TEE/optee_client

divneil · 2019-09-12T08:49:36Z

Signed-off-by: Divneil Rai Wadhawan divneil.r.wadhawan@intel.com

divneil · 2019-09-12T08:52:47Z

This patch is related to OP-TEE/optee_os#3270.
Ticket: OP-TEE/optee_os#3266

This is also an early drop for sharing the idea about accessing custom REE service

Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@intel.com>

jforissier · 2019-09-13T08:14:40Z

 set (SRC
 	src/tee_client_api.c
 	src/teec_trace.c
+	src/ree_service_api.c


Alphabetical order preferred, please

jforissier · 2019-09-13T08:15:10Z

 TEEC_SRCS	:= tee_client_api.c \
-		   teec_trace.c
+		   teec_trace.c \
+		   ree_service_api.c


Alphabetical order preferred, please

jforissier · 2019-09-13T08:15:34Z

+
+#include <tee_client_api.h>
+#include <teec_trace.h>
+#include <ree_service_api.h>


Alphabetical order preferred, please

jforissier · 2019-09-13T08:17:09Z

 	uint8_t clockSeqAndNode[8];
 } TEEC_UUID;

+typedef TEEC_UUID REEC_UUID;


I added to differentiate whose UUID it is. Currently, with the name it always seems it is of TEE. I will remove it.

jforissier · 2019-09-13T08:17:38Z

 	src/tee_supplicant.c
 	src/teec_ta_load.c
+	src/tee_service.c
+	src/tee_service_handle.c


Ordering, and name could be better. ree_service.c and ree_service_handle.c maybe?

I used tee_ because similar data flow direction exists in other files like teec_ta_load.c (TA binary -> optee_os). Please confirm if you want to change the names.

jforissier · 2019-09-13T08:20:04Z

 */
 #define OPTEE_MSG_RPC_CMD_FTRACE	11

+#define OPTEE_MSG_RPC_CMD_GENERIC	30


Name is too generic. OPTEE_MSG_RPC_CMD_REE_SERVICE? (or OPTEE_MSG_RPC_CMD_REE_SRVC).

Sure. Will move to OPTEE_MSG_RPC_CMD_REE_SERVICE

jforissier · 2019-09-13T08:20:50Z

+ * [in]     param[0].u.value.a  OPTEE_MRC_GENERIC_CLOSE
+ * [in]     param[0].u.value.b  TA instance id
+ */
+#define OPTEE_MRC_GENERIC_CLOSE		2


OPTEE_MRC_REE_SERVICE_OPEN, OPTEE_MRC_REE_SERVICE_CLOSE

jforissier · 2019-09-13T08:21:27Z

+#include <teec_trace.h>
+#include <tee_supplicant.h>
+#include <optee_msg_supplicant.h>
+#include <ree_service_api.h>


jforissier · 2019-09-13T08:40:45Z

+TEEC_Result ree_rcv_params(void *service, size_t *num_params,
+					struct tee_params *params);
+TEEC_Result ree_snd_params(void *service, size_t num_params,
+				struct tee_params *params, int32_t error);


ree_service_rcv() and ree_service_snd() maybe (having service here is more useful than params IMO).

More general question: where is the actual service executed, in which context? As far as I can tell, you have two modes:

A dynamic library is loaded by tee-supplicant and a callback is invoked. Code therefore runs in the context of the tee-supplicant RPC loop.

A message queue interface. Code executes in a different process.

Is this correct? I think these function should be commented so it's clear who should use them and for which scenario.

What if someone needs to run the service in kernel space? (i.e., TA wants to talk to a Linux driver). That's a use case I've heard before.

ree_service_rcv() and ree_service_snd() maybe (having service here is more useful than params IMO).

Sure.

A dynamic library is loaded by tee-supplicant and a callback is invoked. Code therefore runs in the context of the tee-supplicant RPC loop.

A message queue interface. Code executes in a different process.

Yes. The example code is in linaro-swg/optee_examples#63. I will fork out a new directory in optee_examples. Currently, in hello_world/host/main.c, there's a code in #if0 (line number 107). It enables Message Queue based params processing.

think these function should be commented so it's clear who should use them and for which scenario.

Sure.

What if someone needs to run the service in kernel space? (i.e., TA wants to talk to a Linux driver). That's a use case I've heard before.

Hmm, for now, I didn't had this requirement, but as I understand, it will involve patching teepriv. A convoluted way could be to write a ree_service and have that service talk to driver.

jforissier · 2019-09-13T08:43:06Z

+{
+	size_t i;
+	TEEC_Result res = TEEC_SUCCESS;
+	TEEC_Result (*process_tee_params)(size_t num_params, struct tee_params *params);


This API has to be declared and documented in some header file, because it needs to be implemented by the code that actually provides the service, right?

My bad. Since, dynamic library loading finds the symbol, so, it doesn't complain. I will fix it.

etienne-lms

some minor comments...

etienne-lms · 2019-09-12T15:54:18Z

+ * This functions finds the service for tee based on UUID
+ * based on either message queue or dynamic lib.
+ * o Message queue is useful when we want to directly pass
+ *   some date to CA.


s/date/data/

Missed in this patch. I will update in next commit

etienne-lms · 2019-09-13T07:18:54Z

+	if (!uuid || !uuid_str)
+		return TEEC_ERROR_BAD_PARAMETERS;
+
+	/* Convert to the uuid string */


style: seems simpler with less snprintf():

snprintf(uuid_str, size, "%08x-%04x-%04x-%02x%02x-", uuid->timeLow, uuid->timeMid, uuid->timeHiAndVersion); idx = strlen(uuid_str); snprintf(uuid_str + idx, size - idx, "%02x%02x%02x%02x%02x%02x%02x%02x", uuid->clockSeqAndNode[0], uuid->clockSeqAndNode[1], uuid->clockSeqAndNode[2], uuid->clockSeqAndNode[3], uuid->clockSeqAndNode[4], uuid->clockSeqAndNode[5], uuid->clockSeqAndNode[6], uuid->clockSeqAndNode[7]);

etienne-lms · 2019-09-13T12:34:04Z

+			param_type == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT ||
+			param_type == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT)
+		return true;
+	return false;


minor style: prefer a switch/case, here and in is_param_type_memref().

switch (param_type) { case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: return true; default: return false; }

Also rename is_param_type_{value|memref}() into param_type_is_{value|memref}().

Same comment for equivalent functions in tee_service.c.

I have modified optee-examples and optee_os, and came here. I used is in the beginning to have the function name as question: "Is it a memref?"

For consistency with other OP-TEE OS source, I suggest to use foo_is_fine() rather than is_foo_find().

It's just a static function so anything within reason goes. I think that is_param_type_memref() looks nice, but I don't have a strong opinion in this.

etienne-lms · 2019-09-13T12:36:24Z

+	if (result != TEEC_SUCCESS)
+		return result;
+
+	struct service *s = malloc(sizeof(struct service));


style: prefer declaration at function entry.

etienne-lms · 2019-09-13T12:37:51Z

+	snprintf(filename, sizeof(filename), "/data/%s", uuid_str);
+	fp = fopen(filename, "w");
+	if (!fp) {
+		printf("Failed to create a file for token\n");


prefer the optee log macros EMSG() for error traces.

etienne-lms · 2019-09-13T12:39:16Z

+	size = fwrite(uuid_str, 1, strlen(uuid_str), fp);
+	if (size != strlen(uuid_str)) {
+		printf("Failed to write to %s\n", filename);
+		result = TEEC_ERROR_GENERIC;


should fclose(fp); here.

Fixed error handling all over.

etienne-lms · 2019-09-13T12:40:51Z

+	}
+
+	if (s->buf)
+		free(s->buf);


minor style: remove the test, calling free(NULL); is safe,

etienne-lms · 2019-09-13T12:41:40Z

+					params[idx].u.memref.size;
+		}
+		idx++;
+		if (idx == 4)


TEE_PARAM_NUM

etienne-lms · 2019-09-13T12:42:25Z

+	long msg_size[2] = {0};
+	size_t size, attr_sz, value_sz, mtype_sz = sizeof(long);
+
+	if (!s || !num_params || !params)


add || (num_params > TEE_PARAM_NUM)

The service caller expects us to fill num_params. It is pointer here.

divneil · 2019-09-18T09:27:27Z

+	if (result != TEEC_SUCCESS)
+		return result;
+
+	struct service *s = malloc(sizeof(struct service));


divneil · 2019-09-18T09:39:06Z

+	size = fwrite(uuid_str, 1, strlen(uuid_str), fp);
+	if (size != strlen(uuid_str)) {
+		printf("Failed to write to %s\n", filename);
+		result = TEEC_ERROR_GENERIC;


Fixed error handling all over.

divneil · 2019-09-18T09:41:55Z

+	long msg_size[2] = {0};
+	size_t size, attr_sz, value_sz, mtype_sz = sizeof(long);
+
+	if (!s || !num_params || !params)


The service caller expects us to fill num_params. It is pointer here.

divneil · 2019-09-18T09:49:07Z

+#include <tee_service.h>
+#include <tee_service_handle.h>
+
+static bool is_param_type_value(uint64_t param_type)


tee-supplicant/src/tee_service.c and libteec/src/ree_service_api.c defines these same functions. Is there any existing file where the common functions between tee-supplicant and libteec can go?

o Added more comments o Renamed following APIs - ree_rcv_params() -> ree_service_rcv() - ree_snd_params() -> ree_service_snd() Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@intel.com>

o dll signature is added in the header file Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@intel.com>

divneil changed the title ~~tee-supplicant: add support for accessing custom REE service~~ [RFC] tee-supplicant: add support for accessing custom REE service Sep 12, 2019

divneil mentioned this pull request Sep 12, 2019

[RFC] hello_world: add demo code for using custom ree service linaro-swg/optee_examples#63

Closed

tee-supplicant: add support for accessing custom REE service

c90d310

Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@intel.com>

jforissier reviewed Sep 13, 2019

View reviewed changes

etienne-lms reviewed Sep 13, 2019

View reviewed changes

divneil commented Sep 18, 2019

View reviewed changes

divneil-intel added 2 commits September 20, 2019 00:31

tee-supplicant: refactored the ree service solution

1b24dc4

o Added more comments o Renamed following APIs - ree_rcv_params() -> ree_service_rcv() - ree_snd_params() -> ree_service_snd() Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@intel.com>

ree_service: added DLL signature

10783ba

o dll signature is added in the header file Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@intel.com>

divneil closed this Oct 23, 2019

Uh oh!

Conversation

divneil commented Sep 12, 2019

Uh oh!

divneil commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Sep 13, 2019 • 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!

etienne-lms 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!

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!

Choose a reason for hiding this comment

Uh oh!

divneil commented Sep 12, 2019 •

edited

Loading

jforissier Sep 13, 2019 •

edited

Loading