core: add new PRC and PTA for loadable plugins framework #4248

anisyanka · 2020-12-08T22:05:02Z

Any external TEE services can be designed as a tee-supplicant plugin.
The plugins will be loaded by the supplicant during startup process
using libdl.
It makes it easy to:

add new features in the supplicant that aren't needed in upstream,
e.g. Rich OS specific services;
sync upstream version with own fork;

These patches add a new RPC - 'OPTEE_RPC_CMD_PLUGIN' as an unified
interface between OP-TEE and any plugins and a new PTA as an unified
interface between any user's TAs and any plugins.

Kernel code can use RPC directly to call for execution of some command in plugins.
TAs can use the plugin-pta.

Every plugin has own name based on UUID.
OP-TEE has access to plugins by it.

See definition of protocol for the command 'OPTEE_RPC_CMD_PLUGIN'
in 'core/include/optee_rpc_cmd.h' file.

Signed-off-by: Aleksandr Anisimov a.anisimov@omprussia.ru

jforissier

Hi @anisyanka,

Thanks for the patches. Please see my comments below.

jforissier · 2020-12-09T08:19:34Z

core/include/optee_rpc_cmd.h

@@ -139,6 +139,11 @@
 */
 #define OPTEE_RPC_CMD_FTRACE		11

+/*
+ * Plugin command, see definition of protocol below


tee-supplicant plugin command ...

jforissier · 2020-12-09T08:33:40Z

core/include/optee_rpc_cmd.h

+/*
+ * Plugin command, see definition of protocol below
+ */
+#define OPTEE_RPC_CMD_PLUGIN		12


OPTEE_RPC_CMD_SUPP_PLUGIN

jforissier · 2020-12-09T08:34:31Z

core/include/optee_rpc_cmd.h

+ * [in]     value[2].a	sub_cmd for plugin
+ * [in]     memref[3]	buffer holding data for plugin
+ */
+#define OPTEE_INVOKE_PLUGIN	0


For consistency: OPTEE_RPC_SUPP_PLUGIN_INVOKE

jforissier · 2020-12-09T08:36:22Z

core/include/optee_rpc_cmd.h

+ * [in]     value[1].b	uuid.d4
+ * [in]     value[1].c	cmd for plugin
+ * [in]     value[2].a	sub_cmd for plugin
+ * [in]     memref[3]	buffer holding data for plugin


Any chance one would need to return data back to secure world? ([in/out]).

I think such cases are possible and we should provide the [in/out] ability, thanks

jforissier · 2020-12-09T08:41:13Z

core/include/tee/uuid.h

+ * Returns TEE_SUCCESS, when successfully converting a string or an error code,
+ * if it has a wrong format or NULL.
+ */
+TEE_Result tee_uuid_from_string(TEE_UUID *dst, const char *str, int len);


This duplicates tee_uuid_from_str() from lib/libutee/tee_uuid_from_str.c. If we keep string UUIDs (which I'm not convinced we should), then the implementation could be moved to libutils I think.

wow, I will try to use the tee_uuid_from_str() from libutee :)
In this case I will drop own implementation.

As I said even better would be to not use character strings at all and use a TEE_UUID instead, unless I am missing something?

TAs work with plugins by its names and that's why the TAs pass the string with name to TEE_InvokeTACommand(), like here. This string is passed to the plugin-pta.
We can use tee_uuid_from_str() inside the PTA before calling tee_invoke_plugin_rpc().
If we will not use character strings at all in optee, we will need convert the strings in TA, when we want to call plugins.
I don't mind, but seems it looks not convinient for TA developes. What do you think?

You could do:

#define SYSLOG_PLUGIN_UUID { 0x96bcf744, 0x4f72, 0x4866, \ { 0xbf, 0x1d, 0x86, 0x34, 0xfd, 0x9c, 0x65, 0xe5 } }

...instead of:

#define SYSLOG_PLUGIN_NAME "96bcf744-4f72-4866-bf1d-8634fd9c65e5"

That's how TAs open other TAs or PTAs for example. The only place you may need to convert from binary UUIDs to a string is in tee-supplicant when looking for a plugin (and even in this case it may not be necessary, I mean assuming tee-supplicant enumerates and loads all the plugins on startup, it would convert from plugin name to UUID instead, or find the UUID in the plugin).

OK, got it, thanks!

jforissier · 2020-12-09T08:42:08Z

core/include/tee/tee_plugin_rpc.h

+#include <tee_api_types.h>
+
+struct tee_plugin_rpc_operation {
+	const char *name; /* string "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */


Why not use a TEE_UUID?

jforissier · 2020-12-09T08:43:17Z

core/tee/sub.mk

@@ -45,3 +45,4 @@ endif #CFG_WITH_USER_TA,y

 srcs-y += uuid.c
 srcs-y += tee_ta_enc_manager.c
+srcs-y += tee_plugin_rpc.c


tee_supp_plugin_rpc.c

jforissier · 2020-12-09T08:47:41Z

core/include/tee/tee_plugin_rpc.h

+#include <stdint.h>
+#include <tee_api_types.h>
+
+struct tee_plugin_rpc_operation {


"supplicant" is missing here, struct supp_plugin_rpc_op maybe?
And filename could be supp_plugin_rpc.h.

jforissier · 2020-12-09T08:50:42Z

core/tee/tee_plugin_rpc.c

+
+	res = tee_uuid_from_string(&u, op->name, strlen(op->name));
+	if (res) {
+		EMSG("can't convert string <%s> to UUID", op->name);


You wouldn't need to care about this if you used TEE_UUID.

jforissier · 2020-12-09T08:52:17Z

core/pta/sub.mk

@@ -10,5 +10,6 @@ endif
 srcs-$(CFG_WITH_STATS) += stats.c
 srcs-$(CFG_SYSTEM_PTA) += system.c
 srcs-$(CFG_NXP_SE05X) += scp03.c
+srcs-$(CFG_PLUGIN_PTA) += plugin.c


CFG_SUPP_PLUGIN_PTA += supp_plugin.c

etienne-lms · 2020-12-09T09:11:01Z

Isn't this providing the same kind of services as proposed by the OCALLs P-Rs (#3673 and related)?

anisyanka · 2020-12-09T14:39:02Z

Isn't this providing the same kind of services as proposed by the OCALLs P-Rs (#3673 and related)?

Thanks for the link. I looked those PRs. The goals of our PRs are similar, but there are important differences:

OCALLs is about an interaction TA with a particular CA, but plugins can be common for several TAs without opening a session.
The tee-supplicant loads all plugins using libdl from special directory during startup and after that all plugins are alive.
For example see the syslog plugin (file tee-supplicant/plugins/syslog/syslog_plugin.c in tee-supplicant: add a framework for loadable plugins optee_client#239). With the help of it any TAs can write something to system log.
The proposed plugins don't require changes in Linux kernel and in libteec API. That's why plugins look more easy, but they still have the same capabilities.

jenswi-linaro

Wouldn't it make sense to add this to the system PTA instead of defining yet another PTA? System PTA is about providing system services to user TAs so it seems like a good match.

jenswi-linaro · 2020-12-11T08:17:25Z

core/tee/tee_plugin_rpc.c

+		goto out;
+	}
+
+	memcpy(&plug_uuid, &u, sizeof(TEE_UUID));


I believe tee_uuid_to_octets() would be a better choice in order to get the correct byte order.

jenswi-linaro · 2020-12-11T08:18:43Z

core/tee/tee_plugin_rpc.c

+		uint32_t d2;
+		uint32_t d3;
+		uint32_t d4;
+	} plug_uuid;


It seems like uint32_t plug_uuid[4] = { }; would be just as useful and a bit simpler.

jenswi-linaro · 2020-12-11T08:40:30Z

core/tee/tee_plugin_rpc.c

+
+	memcpy(&plug_uuid, &u, sizeof(TEE_UUID));
+
+	struct thread_param params[THREAD_RPC_MAX_NUM_PARAMS] = {


Declarations of variables at the start of the block please.

jenswi-linaro · 2020-12-11T08:44:37Z

lib/libutee/include/pta_plugin.h

+ * [in]     value[1].b       sub_command for plugin
+ * [in]     memref[2]        additional data for plugin
+ */
+#define PTA_PLUGIN_INVOKE 0


Wouldn't it be nice with a wrapper in libutee, something like:

TEE_Result optee_invoke_supp_plugin(const TEE_UUID *uuid, uint32_t cmd, uint32_t sub_cmd, void *buf, size_t len);

jenswi-linaro · 2020-12-11T08:46:33Z

lib/libutee/include/pta_plugin.h

+ * [in]     memref[0].size   length of the uuid string including '\0' symbol
+ * [in]     value[1].a       command for plugin
+ * [in]     value[1].b       sub_command for plugin
+ * [in]     memref[2]        additional data for plugin


Shouldn't this be in and out?

anisyanka · 2020-12-15T07:30:29Z

Wouldn't it make sense to add this to the system PTA instead of defining yet another PTA? System PTA is about providing system services to user TAs so it seems like a good match.

Sounds good. I will move core/pta/supp_plugin.c functionality to core/pta/system and add a new API like #4248 (comment) to lib/libutee/tee_system_pta.c

anisyanka · 2020-12-16T10:59:20Z

All comments have been addressed

etienne-lms

Minor comments

etienne-lms · 2020-12-16T11:30:49Z

core/include/tee/tee_supp_plugin_rpc.h

+ */
+
+#ifndef TEE_PLUGIN_RPC_H
+#define TEE_PLUGIN_RPC_H


TEE_SUPP_PLUGIN_RPC_H

etienne-lms · 2020-12-16T11:31:25Z

core/pta/system.c

+static TEE_Result system_supp_plugin_invoke(uint32_t param_types,
+					    TEE_Param params[TEE_NUM_PARAMS])
+{
+	struct tee_supp_plugin_rpc_op op;


etienne-lms · 2020-12-16T11:32:55Z

core/tee/tee_supp_plugin_rpc.c

+#include <tee/tee_supp_plugin_rpc.h>
+#include <tee/uuid.h>
+#include <trace.h>
+#include <mm/mobj.h>


move before line 8

etienne-lms · 2020-12-16T11:33:15Z

core/tee/tee_supp_plugin_rpc.c

+{
+	TEE_Result res = TEE_ERROR_GENERIC;
+	struct thread_param params[THREAD_RPC_MAX_NUM_PARAMS];
+	uint32_t plugin_uuid[4];


etienne-lms · 2020-12-16T11:33:25Z

core/tee/tee_supp_plugin_rpc.c

+	uint32_t plugin_uuid[4];
+	void *va = NULL;
+	struct mobj *mobj = NULL;
+	uint32_t outlen;


etienne-lms · 2020-12-16T11:37:47Z

lib/libutee/tee_system_pta.c

+	res = invoke_system_pta(PTA_SYSTEM_SUPP_PLUGIN_INVOKE, param_types,
+				params);
+	if (res)
+		EMSG("Invoke tee-supplicant's plugin failed");


I would make it a DMSG():

DMSG("Invoke tee-supplicant's plugin failed: %#"PRIx32, res);

Could you please explain why you want to use DMSG() instead EMSG()?
In this case we print the error message, that's why it seems EMSG is more suitable.

core/pta/system.c

jenswi-linaro · 2020-12-16T12:36:12Z

lib/libutee/include/pta_system.h

+/*
+ * Invoke a tee-supplicant's plugin
+ *
+ * [in]     memref[0]        uuid(octets) of the plugin


Specify that it's a TEE_UUID to be clear.

jenswi-linaro · 2020-12-16T12:38:14Z

lib/libutee/include/tee_internal_api_extensions.h

+ * @cmd:        command for the plugin
+ * @sub_cmd:    subcommand for the plugin
+ * @buf:        data [for/from] the plugin [in/out]
+ * @len:        length of the buf


Should this be an out parameter too, in case the size is smaller when returning some result?

I think yes.
We can replace size_t len to size_t *len and use one pointer to input and output length.

jenswi-linaro · 2020-12-22T07:28:06Z

core/tee/tee_supp_plugin_rpc.c

+#include <trace.h>
+#include <mm/mobj.h>
+
+TEE_Result tee_invoke_supp_plugin_rpc(struct tee_supp_plugin_rpc_op *op)


Why is this function taking a struct tee_supp_plugin_rpc_op only used for this purpose instead of separate parameters?

I think we can use separate parameters here. Ok, I will drop tee_supp_plugin_rpc_op

anisyanka · 2020-12-23T08:19:13Z

All comments have been addressed

jenswi-linaro · 2020-12-23T08:15:13Z

core/pta/system.c

+					  params[1].value.b, /* sub_cmd */
+					  params[2].memref.buffer, /* data */
+					  params[2].memref.size, /* in len */
+					  (size_t *)&params[3].value.a


This cast isn't portable.

Yes, you are right. I will fix it

I'm sorry, I should have been more clear. Please use a temporary helper variable instead of changing the type in the argument of tee_invoke_supp_plugin_rpc().

It seems you see outdated changes.
In previous commits I changed outlen type for tee_invoke_supp_plugin_rpc() from size_t * to uint32_t * and removed type conversion in core/pta/system.c:863. See the last fixup commit: 3a46040#diff-80ee4f614b595d036997a8c22e8034f37acf85250c5829593e630cab71a9bb4aR863.

I can return back size_t * and add temporary helper variable, if it will be better.

I can return back size_t * and add temporary helper variable, if it will be better.

Please do so.

jenswi-linaro · 2020-12-23T08:48:43Z

core/tee/tee_supp_plugin_rpc.c

+		memcpy(va, buf, len);
+	}
+
+	plugin_uuid = (uint32_t *)uuid;


I believe you need to do something like:

uint32_t uuid_words[4] = { }; tee_uuid_to_octets(uuid_words, uuid);

Secure world is always in little endian so there's no need for any more byte swapping.

Later when decoding this in tee-supplicant you'd do something like:

TEEC_UUID uuid = { }; uint32_t uuid_words[4] = { }; uuid_words[0] = from_le32(params[0].b); ... tee_uuid_from_octets(&uuid, (void *)uuid_words);

This should work for both a big and little endian normal world. Other parts in the chain (driver) are missing so a big endian normal world wouldn't work for other reasons, but we shouldn't make it harder than necessary in case we'd ever fix that.

Do it make sense?

Yes, it make sense. I added le32toh() to tee-supplicant when decoding uuid_words[].

jenswi-linaro · 2020-12-23T08:55:50Z

core/include/optee_rpc_cmd.h

+ * [out]    value[2].b  length of the outbuf (memref[3]), if out is needed.
+ * [in/out] memref[3]	buffer holding data for plugin
+ *
+ * Serialized bytes of the TEE_UUID:


Might be more clear to say: "UUID serialized into octets".

This is not to be confused with the in memory representation of TEE_UUID.

anisyanka · 2020-12-24T13:30:57Z

All comments have been addressed

jenswi-linaro · 2021-01-22T09:33:55Z

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Any external TEE services can be designed as a tee-supplicant plugin. The plugins will be loaded by the supplicant during startup process using libdl. It makes it easy to: - add new features in the supplicant that aren't needed in upstream, e.g. Rich OS specific services; - sync upstream version with own fork; This patch adds a new RPC - 'OPTEE_RPC_CMD_SUPP_PLUGIN' as an unified interface between OP-TEE and any plugins. Kernel code can use it to call for execution of some command in plugins. Every plugin has own name based on UUID. OP-TEE has access to plugins by it. See definition of protocol for the plugin RPC command in 'core/include/optee_rpc_cmd.h' file. Signed-off-by: Aleksandr Anisimov <a.anisimov@omprussia.ru> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

This patch adds a new API to libutee to interact with tee-supplicant plugins from TEE userspace. Every user TA can use 'tee_invoke_supp_plugin()' to send any commands to a plugin. The commands are predefined by the plugin developer. See the https://github.com/linaro-swg/optee_examples repo for an example of using plugins. Signed-off-by: Aleksandr Anisimov <a.anisimov@omprussia.ru> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

anisyanka · 2021-01-29T17:03:27Z

The commits reworded.
Thank you for the review!

jforissier · 2021-02-04T09:14:01Z

@anisyanka thanks for your contribution.

This was referenced Dec 8, 2020

Loadable plugins in tee-supplicant OP-TEE/optee_client#219

Closed

tee-supplicant: add a framework for loadable plugins OP-TEE/optee_client#239

Merged

plugins: example of using supplicant syslog plugin from TA code linaro-swg/optee_examples#79

Merged

jforissier reviewed Dec 9, 2020

View reviewed changes

jenswi-linaro reviewed Dec 11, 2020

View reviewed changes

etienne-lms reviewed Dec 16, 2020

View reviewed changes

jenswi-linaro reviewed Dec 16, 2020

View reviewed changes

jenswi-linaro reviewed Dec 22, 2020

View reviewed changes

jenswi-linaro reviewed Dec 23, 2020

View reviewed changes

anisyanka mentioned this pull request Jan 20, 2021

regression: add tests for the supplicant plugin framework OP-TEE/optee_test#482

Merged

Aleksandr Anisimov added 2 commits January 25, 2021 12:15

anisyanka force-pushed the plugin-pta branch from 95a4906 to 1dd4fb0 Compare January 25, 2021 12:14

jforissier merged commit e4ad5cc into OP-TEE:master Feb 4, 2021

anisyanka deleted the plugin-pta branch February 4, 2021 15:19


		memcpy(&plug_uuid, &u, sizeof(TEE_UUID));

		struct thread_param params[THREAD_RPC_MAX_NUM_PARAMS] = {

core: add new PRC and PTA for loadable plugins framework #4248

core: add new PRC and PTA for loadable plugins framework #4248

Conversation

anisyanka commented Dec 8, 2020

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

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 Dec 9, 2020

anisyanka commented Dec 9, 2020

jenswi-linaro 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

anisyanka commented Dec 15, 2020

anisyanka commented Dec 16, 2020

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

anisyanka Dec 17, 2020 • edited

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

anisyanka commented Dec 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anisyanka Jan 10, 2021 • edited

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

anisyanka commented Dec 24, 2020

jenswi-linaro commented Jan 22, 2021

anisyanka commented Jan 29, 2021

jforissier commented Feb 4, 2021

anisyanka Dec 17, 2020 •

edited

anisyanka Jan 10, 2021 •

edited