tee-supplicant: add a framework for loadable plugins by anisyanka · Pull Request #239 · OP-TEE/optee_client

anisyanka · 2020-12-08T22:04:15Z

This framework makes the supplicant a bit more flexible
in terms of providing services. Any external TEE services
can be designed as a tee-supplicant plugin.
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;

To create a plugin developers should implement the
'struct plugin_method' and a bind function.
See public/tee_plugin_method.h file.
As an example this patch implements a 'syslog' plugin
and its Makefile. The plugin helps to log any
information from TEE to system journal.

The plugin framework is based on new RPC call - 'OPTEE_MSG_RPC_CMD_PLUGIN'.
This RPC is an unified interface between TEE and any plugins.
Every plugin has own name based on UUID. TEE has access to
plugins only by the name (UUID).

Every plugin should be placed into "/usr/lib/tee-supplicant/plugins/".
The supplicant opens and binds all plugins in the directory during
startup process using libdl. After this any requests to plugins
from TEE will be processed in the common RPC handler.

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

anisyanka · 2020-12-08T22:19:01Z

The PR relates to:
optee backend: OP-TEE/optee_os#4248
example of using: linaro-swg/optee_examples#79

jenswikl · 2020-12-09T17:16:05Z

Why doesn't tee-supplicant read from the ringbuffer directly?
Assuming that this relates to OP-TEE/optee_os#4230
I have a feeling that this will not be very suitable for the logging framework since we may want to use all kinds of tricks for efficiency.

anisyanka · 2020-12-10T10:41:37Z

Why doesn't tee-supplicant read from the ringbuffer directly?
Assuming that this relates to OP-TEE/optee_os#4230
I have a feeling that this will not be very suitable for the logging framework since we may want to use all kinds of tricks for efficiency.

Sorry, if I confused you. I have not started to code the logging framework discussed in OP-TEE/optee_os#4230 yet.

This PR does not apply to the logger, because it is only about plugins.
The plugins are needed just to do the tee-supplicant a bit more flexible in terms of providing services.
In our system there are several OS specific services, which are needed from optee code or TA code.
For example os specific logging mechanisms (not the syslog) and entropy gathering service.
Of course, we can add new RPCs to optee and the supplicant and always modify its code for working with our services,
but, seems, it looks overkill. I think the other optee users may have the same experience.
That's why we proposed to use plugins like an universal framework to add new specific feature to the supplicant.

As an example I added a plugin named syslog (file tee-supplicant/plugins/syslog/syslog_plugin.c).
We can consider the 'syslog' plugin as a proof-of-concept that the plugin framework works.
Also It maybe useful if you want to have a simple logger from TAs code already now,
but this plugin is not what we discussed, when we talked about the logger :)

jenswikl · 2020-12-11T09:41:35Z

+};
+
+/* The bind function must save plugin methods into the passed argument */
+#define IMPLEMENT_PLUGIN_METHOD_BIND_FN(bind_func)                             \


Why do we need a "bind" function when we have an init function too?
Couldn't we just as well do something like:

struct plugin_method { const char *name; /* short friendly name of the plugin */ TEEC_UUID uuid; int (*init)(void); int (*invoke)(int cmd, int sub_cmd, void *data, size_t len); };

And then in the plugin just declare

struct plugin_method plugin_method = { ... };

It a bit more direct and also removes the dependency on the file name.

jenswikl · 2020-12-11T09:52:17Z

+
+struct plugin_method {
+	int (*init)(void);
+	int (*invoke)(int cmd, int sub_cmd, void *data, size_t len);


unsigned int instead of just int?

anisyanka · 2020-12-16T10:58:57Z

All comments have been addressed

jenswikl · 2020-12-16T11:10:51Z

+	if (!p)
+		return TEEC_ERROR_ITEM_NOT_FOUND;
+
+	if (!p->method->invoke)


This "can't happen" since it was tested in load_plugin() above. An assert() should be enough.

jenswikl · 2020-12-16T11:11:55Z

+struct plugin_method {
+	const char *name; /* short friendly name of the plugin */
+	TEEC_UUID uuid;
+	int (*init)(void);


Any reason why we're not returning a TEEC_Result instead here and for invoke() below?

I think we can replace int to TEEC_Result

jenswikl · 2020-12-16T11:14:06Z

+				if (res != 0) {
+					free(p);
+					EMSG("init the <%s> plugin failed with orig = %d",
+					     p->method->name, res);


Use after free of p.

jenswikl · 2020-12-16T11:57:14Z

+ * Invoke a tee-supplicant plugin.
+ *
+ * [in]     param[0].u.value.a	OPTEE_INVOKE_PLUGIN
+ * [in]     param[0].u.value.b	uuid.d1


We need to define byte order of the UUID. Consider the case when Secure world is little endian but Normal world is Big endian. These value words will then be swapped (in the driver, not supported now by the way) before transmission.

I think this should to be defined as in https://developer.arm.com/documentation/den0028/c/ section "5.3 Unique Identification format".

anisyanka · 2020-12-23T08:09:30Z

All comments have been addressed

jenswikl · 2021-01-04T08:15:44Z


+static void uuid_from_octets(TEEC_UUID *d, const uint8_t s[TEE_IOCTL_UUID_LEN])
+{
+	d->timeLow = (s[0] << 24) | (s[1] << 16) | (s[2] << 8) | s[3];


Does work (will the uint8_t automatically be promoted to unsigned int here)?
It seems more robust to either use a helper macro like SHIFT_U32() in https://github.com/OP-TEE/optee_os/blob/b68aca61f70193c8715a49748d13c77afe43aa1c/lib/libutils/ext/include/util.h#L117
or just cast each s[x] to uint32_t.

I agree.
It seems we need common util.h file in optee-client project.
We can put into one all helpers which already exists in the tee_supplicant.c file (e.g. uuid_from_octets()), in the libcktee (file local_utils.h) and also SHIFT_U32(). But it should be done in separate MR.

So far I did casting each s[x] to uint32_t.

jenswikl · 2021-01-04T08:20:43Z

+ * Copyright (c) 2020, Open Mobile Platform LLC
+ */
+
+#ifndef __TEE_PLUGIN_METHOD_H


Please drop the __ prefix. Those are reserved for libc and friends in user space.
We're using it in some files already, but let's not make matters worse.

jenswikl · 2021-01-04T08:24:23Z

+
+	dir = opendir(TEE_PLUGIN_LOAD_PATH);
+	if (!dir) {
+		EMSG("could not open directory %s", TEE_PLUGIN_LOAD_PATH);


I suppose this isn't an error, please use IMSG() instead.

jenswikl · 2021-01-04T08:27:34Z

+/* This structure describes one plugin for the supplicant */
+struct plugin {
+	void *handle;
+	struct plugin_method *method; /* implemented by plugins' authors */


Implemented in the plugin

jenswikl · 2021-01-04T08:28:37Z

 	}

+	if (plugin_load_all() != 0) {
+		EMSG("failed to load user's tee plugins");


Failed to load plugins.

jenswikl · 2021-01-04T08:33:15Z

+ * Copyright (c) 2020, Open Mobile Platform LLC
+ */
+
+#ifndef __PLUGIN_H


Please drop the __ prefix.

anisyanka · 2021-01-10T10:33:03Z

All comments have been addressed

jenswikl · 2021-01-15T13:11:41Z

+		    TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT)
+		return TEEC_ERROR_BAD_PARAMETERS;
+
+	uuid_words[0] = le32toh(params[0].b);


I'm sorry I was wrong about this swapping to make it little endian. I Recently got clarifications from Arm on this.
See https://www.spinics.net/lists/arm-kernel/msg868040.html
So please drop the le32toh() and we should be good.

Thank you for the clarification

jenswikl · 2021-01-15T13:14:41Z

+#include <linux/tee.h>
+
+/* internal possible returned values */
+enum {


Please give a name to this enum so that we can use the correct type when it's used.

anisyanka · 2021-01-20T09:46:33Z

All comments have been addressed

jenswikl · 2021-01-22T09:08:57Z

-	mkdir -p $(DESTDIR)$(SBINDIR) $(DESTDIR)$(LIBDIR) $(DESTDIR)$(INCLUDEDIR)
+	mkdir -p $(DESTDIR)$(SBINDIR) $(DESTDIR)$(LIBDIR) $(DESTDIR)$(INCLUDEDIR) $(DESTDIR)$(PLUGINDIR)
+	mkdir -p $(MKDIR)
+	cp config.mk $(MKDIR)


When building with buildroot for ARMv7 $(MKDIR) seems to become host/arm-buildroot-linux-gnueabihf/sysroot/mk. This is far too likely to clash with other packages. I'd prefer something like:

cp config.mk $(DESTDIR)/optee_client_config.mk

jenswikl · 2021-01-22T09:18:39Z

 	cp libckteec/include/*.h $(DESTDIR)$(INCLUDEDIR)
 	cp -a ${O}/libckteec/libckteec.so* $(DESTDIR)$(LIBDIR)
 	cp -a ${O}/libckteec/libckteec.a $(DESTDIR)$(LIBDIR)
+	$(eval SUPP_PLUGINS := $(shell find ./tee-supplicant/plugins -name *.plugin))


We do we need this when we don't have any plugins in this git?

I think with these lines the plugin framework looks complete and ready to using.
I'd prefer keep ones here in case if someone will add own plugins in own fork.
In this case they won't have to edit the Makefile to build the plugins.

And also, these lines don't generate errors during building, if there are no plugins.

I hope we can agree that this is dead code here in upstream.

What happens downstream we can't control, perhaps using a separate git for plugins is the preferred solution. We're demonstrating that in linaro-swg/optee_examples#79 so it seems at bit strange to promote another approach here.

You're right.
I'll drop these lines

anisyanka · 2021-01-25T12:13:50Z

All comments have been addressed.

jenswikl · 2021-01-25T14:17:02Z

+TEES_LFLAGS += -Wl,-rpath=$(CFG_TEE_PLUGIN_LOAD_PATH)

-tee-supplicant: $(TEES_FILE)
+tee-supplicant: $(TEES_FILE) plugins


I don't suppose we need the plugins target any longer. Please remove it here and elsewhere.

jenswikl · 2021-01-25T14:17:50Z

@@ -0,0 +1,11 @@
+# Add new plugin dirs here


This file together with the tee-supplicant/plugins directory can be removed.

jenswikl · 2021-01-25T14:19:32Z

 	PRIVATE teec
-	PRIVATE optee-client-headers)
+	PRIVATE optee-client-headers
+	PRIVATE dl)


Is this how the -ldl parameter get added when linking?
How is the -Wl,-rpath=$(CFG_TEE_PLUGIN_LOAD_PATH) parameter added?

Is this how the -ldl parameter get added when linking?

Yes, it is.

How is the -Wl,-rpath=$(CFG_TEE_PLUGIN_LOAD_PATH) parameter added?

I've used CMAKE_INSTALL_RPATH. It is a variable embedded in cmake.
If I understood cmake specification right, the variable is needed to set a run-time search path.

jenswikl · 2021-01-25T17:36:51Z

Please squash in the fixup commits

jenswikl · 2021-01-29T13:46:44Z

I'm sorry I didn't see the update before. It seems that github sometimes doesn't notify of a force-push. The safest is to add a comment once a force-push has been performed. Just a plain "Update" is good enough.

jenswikl

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

This framework makes the supplicant a bit more flexible in terms of providing services. Any external TEE services can be designed as a tee-supplicant plugin. 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; To create a plugin developers should implement the 'struct plugin_method'. See public/tee_plugin_method.h file. The plugin framework is based on new RPC call - 'OPTEE_MSG_RPC_CMD_PLUGIN'. This is an unified interface between TEE and any plugins. Every plugin has own name based on UUID. TEE has access to plugins only by this UUID. Every plugin should be placed into "/usr/lib/tee-supplicant/plugins/". See 'CFG_TEE_PLUGIN_LOAD_PATH' definition in config.mk. The supplicant opens and binds all plugins in the directory during startup process using libdl. After this any requests to plugins from TEE will be processed in the common RPC handler. Signed-off-by: Aleksandr Anisimov <a.anisimov@omprussia.ru> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

anisyanka · 2021-01-29T16:59:45Z

Updated

ruchi393 · 2021-02-05T05:54:50Z

@anisyanka , can you please add documentation related with this feature by raising a PR in https://github.com/OP-TEE/optee_docs.

anisyanka · 2021-02-05T08:45:23Z

@ruchi393, yes, good idea, thanks!

anisyanka mentioned this pull request Dec 8, 2020

Loadable plugins in tee-supplicant #219

Closed

This was referenced Dec 8, 2020

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

Merged

core: add new PRC and PTA for loadable plugins framework OP-TEE/optee_os#4248

Merged

jenswikl reviewed Dec 11, 2020

View reviewed changes

jenswikl reviewed Dec 16, 2020

View reviewed changes

jenswikl reviewed Jan 4, 2021

View reviewed changes

jenswikl reviewed Jan 15, 2021

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

jenswikl reviewed Jan 22, 2021

View reviewed changes

anisyanka force-pushed the loadable-plugins branch from 8fbca3c to e931b3a Compare January 25, 2021 12:11

jenswikl reviewed Jan 25, 2021

View reviewed changes

anisyanka force-pushed the loadable-plugins branch from 1a9c5a6 to 917f227 Compare January 25, 2021 17:50

anisyanka requested a review from jenswikl January 29, 2021 13:32

jenswikl reviewed Jan 29, 2021

View reviewed changes

anisyanka force-pushed the loadable-plugins branch from 917f227 to 9c859a2 Compare January 29, 2021 16:58

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

anisyanka deleted the loadable-plugins branch February 4, 2021 15:19

Uh oh!

Conversation

anisyanka commented Dec 8, 2020

Uh oh!

anisyanka commented Dec 8, 2020

Uh oh!

jenswikl commented Dec 9, 2020

Uh oh!

anisyanka commented Dec 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anisyanka commented Dec 16, 2020

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!

anisyanka commented Dec 23, 2020

Uh oh!

jenswikl Jan 4, 2021 • 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!

anisyanka Jan 10, 2021 • 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!

anisyanka commented Jan 10, 2021

Uh oh!

jenswikl Jan 15, 2021 • 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!

anisyanka commented Jan 20, 2021

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!

jenswikl Jan 25, 2021 • 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!

anisyanka commented Jan 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jenswikl Jan 4, 2021 •

edited

Loading

anisyanka Jan 10, 2021 •

edited

Loading

jenswikl Jan 15, 2021 •

edited

Loading

jenswikl Jan 25, 2021 •

edited

Loading

anisyanka Jan 25, 2021 •

edited

Loading