Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions libteec/CMakeLists.txt
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ set (CFG_TEE_CLIENT_LOG_FILE "/data/tee/teec.log" CACHE STRING "Location of libt
set (SRC
src/tee_client_api.c
src/teec_trace.c
src/ree_service_api.c

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetical order preferred, please

)

if (CFG_TEE_BENCHMARK)
Expand Down
3 changes: 2 additions & 1 deletion libteec/Makefile
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ LIB_MAJ_MIN := $(LIB_NAME).$(MAJOR_VERSION).$(MINOR_VERSION)
LIB_MAJ_MIN_P := $(LIB_NAME).$(MAJOR_VERSION).$(MINOR_VERSION).$(PATCH_VERSION)

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetical order preferred, please

ifeq ($(CFG_TEE_BENCHMARK),y)
TEEC_SRCS += teec_benchmark.c
endif
Expand Down
255 changes: 255 additions & 0 deletions libteec/src/ree_service_api.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,255 @@

#include <sys/types.h>
#include <sys/ipc.h>
#include <sys/msg.h>
#include <stdlib.h>
#include <errno.h>

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetical order preferred, please


#ifndef __aligned
#define __aligned(x) __attribute__((__aligned__(x)))
#endif
#include <linux/tee.h>

struct service {
int msgqid;
void *buf;
size_t buf_sz;
};

static bool is_param_type_value(uint64_t param_type)
{
if (param_type == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT ||
param_type == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT ||
param_type == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT)
return true;
return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

static bool is_param_type_memref(uint64_t param_type)
{
if (param_type == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT ||
param_type == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT ||
param_type == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT)
return true;
return false;
}

/**
* uuid_to_str() - convert uuid structure to string
*
* Example uuid: 2aa2685c-fba3-44be-a218-fbdafebd639a
* Convert the structure to the string form as above
*/
TEEC_Result uuid_to_str(REEC_UUID *uuid, char *uuid_str, size_t size)
{
uint32_t i, idx;

if (!uuid || !uuid_str)
return TEEC_ERROR_BAD_PARAMETERS;

/* Convert to the uuid string */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);

snprintf(uuid_str, size, "%08x-", uuid->timeLow);
idx = strlen(uuid_str);

snprintf(uuid_str + idx, size - idx, "%04x-", uuid->timeMid);
idx = strlen(uuid_str);

snprintf(uuid_str + idx, size - idx,
"%04x-", uuid->timeHiAndVersion);
idx = strlen(uuid_str);

snprintf(uuid_str + idx, size,
"%02x%02x-", uuid->clockSeqAndNode[0],
uuid->clockSeqAndNode[1]);
idx = strlen(uuid_str);

for (i = 2; i < 8; i++) {
snprintf(uuid_str + idx, size - idx,
"%02x", uuid->clockSeqAndNode[i]);
idx = strlen(uuid_str);
}

return TEEC_SUCCESS;
}

TEEC_Result ree_service_init(REEC_UUID *uuid, void **service)
{
int ret = -1;
size_t size;
FILE *fp = NULL;
char filename[64];
key_t msgqkey = 0;
TEEC_Result result;
char uuid_str[48];

result = uuid_to_str(uuid, uuid_str, sizeof(uuid_str));
if (result != TEEC_SUCCESS)
return result;

struct service *s = malloc(sizeof(struct service));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: prefer declaration at function entry.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad miss.

if (!s)
return -ENOMEM;

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

goto err;
}

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should fclose(fp); here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed error handling all over.

goto err;
}

if (fclose(fp)) {
printf("Failed to commit data to storage\n");
result = TEEC_ERROR_GENERIC;
goto err;
}

/* Create a message queue and wait for the msg */
msgqkey = ftok(filename, 'O');
if (msgqkey == -1) {
printf("Failed to create a msg queue key (%d: %s)\n",
errno, strerror(errno));
result = TEEC_ERROR_GENERIC;
goto err;
}

s->msgqid = msgget(msgqkey, 0600 | IPC_CREAT);
if (s->msgqid == -1) {
printf("Failed to get the msg queue\n");
result = TEEC_ERROR_GENERIC;
goto err;
}

*service = s;

return 0;

err:
if (s)
free(s);

return ret;
}

void ree_service_exit(void *service)
{
struct service *s = service;

if (!s)
return;

if (s->msgqid != -1) {
if (msgctl(s->msgqid, IPC_RMID, NULL) == -1)
printf("Failed to delete msgq, try using ipcrm\n");
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


free(s);
}

TEEC_Result ree_rcv_params(void *service, size_t *num_params,
struct tee_params *params)
{
int ret, idx = 0;
char *buf = NULL, *ptr;
struct service *s = service;
long msg_size[2] = {0};
size_t size, attr_sz, value_sz, mtype_sz = sizeof(long);

if (!s || !num_params || !params)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add || (num_params > TEE_PARAM_NUM)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return TEEC_ERROR_BAD_PARAMETERS;

attr_sz = sizeof(params->attr);
value_sz = sizeof(params->u.value);

/* The first message will tell the size of buffer */
ret = msgrcv(s->msgqid, &msg_size,
sizeof(msg_size[1]), OPTEE_MRC_MSG_SEND, 0);
if (ret == -1) {
printf("Failed to get the size of buffer\n");
goto err;
}
size = msg_size[1];
s->buf_sz = size;

buf = calloc(size, 1);
if (!buf) {
printf("Out of memory to receive message\n");
goto err;
}

/* The second message will retrive full contents */
ret = msgrcv(s->msgqid, buf, size - mtype_sz, OPTEE_MRC_MSG_SEND, 0);
if (ret == -1) {
printf("Failed to receive msg\n");
goto err;
}

/* Real params start from here: buf + mtype_sz */
for (ptr = buf + mtype_sz; ptr < buf + size - sizeof(TEEC_Result);) {

if (is_param_type_value(*(long *)ptr)) {

memcpy(&params[idx].attr, ptr, attr_sz);
ptr += attr_sz;

memcpy(&params[idx].u.value, ptr, value_sz);
ptr += value_sz;

} else if (is_param_type_memref(*(long *)ptr)) {

memcpy(&params[idx].attr, ptr, attr_sz);
ptr += attr_sz;

params[idx].u.memref.size = *(size_t *)ptr;
params[idx].u.memref.buffer = ptr +
sizeof(params[idx].u.memref.size);
ptr = (char *)params[idx].u.memref.buffer +
params[idx].u.memref.size;
}
idx++;
if (idx == 4)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TEE_PARAM_NUM

break;
}

s->buf = buf;
*num_params = idx;
return 0;

err:
if (buf)
free(buf);
return TEEC_ERROR_GENERIC;
}

TEEC_Result ree_snd_params(void *service, size_t num_params,
struct tee_params *params, int32_t error)
{
struct service *s = service;
size_t mtype_sz = sizeof(long);

(void)num_params;
(void)params;
(void)error;

*(TEEC_Result *)((uint8_t *)s->buf + s->buf_sz - sizeof(TEEC_Result)) = error;

*((long *)s->buf) = OPTEE_MRC_MSG_RCV;
if (msgsnd(s->msgqid, s->buf, s->buf_sz - mtype_sz, 0) == -1)
printf("Failed to send the response\n");

return 0;
}
76 changes: 76 additions & 0 deletions public/ree_service_api.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#ifndef __REE_SERVICE_H__
#define __REE_SERVICE_H__

/*
* Attributes for struct tee_ioctl_param, selects field in the union
*/
#define TEE_PARAM_ATTR_TYPE_NONE 0 /* parameter not used */

/*
* These defines value parameters (struct tee_ioctl_param_value)
*/
#define TEE_PARAM_ATTR_TYPE_VALUE_INPUT 1
#define TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT 2
#define TEE_PARAM_ATTR_TYPE_VALUE_INOUT 3 /* input and output */

/*
* These defines shared memory reference parameters (struct
* tee_ioctl_param_memref)
*/
#define TEE_PARAM_ATTR_TYPE_MEMREF_INPUT 5
#define TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT 6
#define TEE_PARAM_ATTR_TYPE_MEMREF_INOUT 7 /* input and output */
struct tee_param_memref {
void *buffer;
uint64_t size;
};

struct tee_param_value {
uint64_t a;
uint64_t b;
uint64_t c;
};

struct tee_params {
uint64_t attr;
union {
struct tee_param_memref memref;
struct tee_param_value value;
} u;
};

TEEC_Result uuid_to_str(REEC_UUID *uuid, char *uuid_str, size_t size);
TEEC_Result ree_service_init(REEC_UUID *uuid, void **service);
void ree_service_exit(void *service);
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);

@jforissier jforissier Sep 13, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sure.

  1. 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.
  2. 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.


/*
* Define protocol for messages with .cmd == OPTEE_MSG_RPC_CMD_GENERIC
*/

/*
* Open REE Service
*
* [in] param[0].u.value.a OPTEE_MRC_GENERIC_OPEN
* [in] param[0].u.value.b TA instance id
* [out] param[1].u.value.c service handle
*/
#define OPTEE_MRC_GENERIC_SERVICE_START 3

/*
* Close REE Service
*
* [in] param[0].u.value.a OPTEE_MRC_GENERIC_CLOSE
* [in] param[0].u.value.b TA instance id
*/
#define OPTEE_MRC_GENERIC_SERVICE_STOP 4

/* mtype for message queue message exchange */
#define OPTEE_MRC_MSG_SEND 1 /* send params to service */
#define OPTEE_MRC_MSG_RCV 2 /* receive params from service */


#endif
2 changes: 2 additions & 0 deletions public/tee_client_api.h
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ typedef struct {
uint8_t clockSeqAndNode[8];
} TEEC_UUID;

typedef TEEC_UUID REEC_UUID;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* struct TEEC_SharedMemory - Memory to transfer data between a client
* application and trusted code.
Expand Down
3 changes: 3 additions & 0 deletions tee-supplicant/CMakeLists.txt
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ set (SRC
src/tee_supp_fs.c
src/tee_supplicant.c
src/teec_ta_load.c
src/tee_service.c
src/tee_service_handle.c

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

if (CFG_GP_SOCKETS)
Expand Down Expand Up @@ -86,6 +88,7 @@ target_include_directories(${PROJECT_NAME} PRIVATE src)

target_link_libraries (${PROJECT_NAME}
PRIVATE teec
PRIVATE dl
PRIVATE optee-client-headers)

################################################################################
Expand Down
6 changes: 4 additions & 2 deletions tee-supplicant/Makefile
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ TEES_SRCS := tee_supplicant.c \
teec_ta_load.c \
tee_supp_fs.c \
rpmb.c \
handle.c
handle.c \
tee_service.c \
tee_service_handle.c


ifeq ($(CFG_GP_SOCKETS),y)
Expand Down Expand Up @@ -55,7 +57,7 @@ ifeq ($(CFG_TA_TEST_PATH),y)
TEES_CFLAGS += -DCFG_TA_TEST_PATH=1
endif
TEES_FILE := $(OUT_DIR)/$(PACKAGE_NAME)
TEES_LFLAGS := $(LDFLAGS) -L$(OUT_DIR)/../libteec -lteec
TEES_LFLAGS := $(LDFLAGS) -L$(OUT_DIR)/../libteec -lteec -ldl

ifeq ($(CFG_TA_GPROF_SUPPORT),y)
TEES_CFLAGS += -DCFG_TA_GPROF_SUPPORT
Expand Down
Loading