Hi,
There's been a massive resistance to the opaque TEE_IOC_CMD interface on
the kernel mailing lists. The last is from Mark Rutland:
https://lkml.org/lkml/2015/6/5/244
Jason Gunthorpe made some quite hard remarks in
https://lkml.org/lkml/2015/4/20/513
There hasn't been that many different people (3 or something)
criticizing the interface, but everyone has more or less asked us to
reconsider.
I think it's time to take one step back and reevaluate the consequences
of having a well defined user space API.
Consider dropping TEE_IOC_CMD and replace it with:
struct tee_open_session_arg {
__u8 uuid[UUID_LEN];
__u8 clnt_uuid[UUID_LEN];
__u32 clnt_login;
__u32 cookie; /* some unique value set by user space */
__u32 session;
__u32 ret;
__u32 ret_origin;
__u32 num_params;
};
TEE_IOC_OPEN_SESSION
struct tee_invoke_command_arg {
__u32 func;
__u32 cookie; /* some unique value set by user space */
__u32 session;
__u32 ret;
__u32 ret_origin;
__u32 num_params;
};
TEE_IOC_INVOKE_COMMAND
struct tee_cancel_command_arg {
__u32 cookie; /* matches the command or open session to cancel */
__u32 session;
__u32 ret;
__u32 ret_origin;
};
TEE_IOC_CANCEL_COMMAND
struct tee_close_session_arg {
__u32 session;
__u32 ret;
__u32 ret_origin;
};
TEE_IOC_CLOSE_SESSION
The tee_open_session_arg and tee_invoke_command_arg structs would be
followed by a struct msg_param as defined in optee_msg.h in patch v3
https://lkml.org/lkml/2015/5/15/48 .
TEE_IOC_VERSION would be changed to:
#define TEE_IMPL_ID_OPTEE 1
#define TEE_OPTEE_CAP_TZ (1 << 0)
struct tee_ioctl_version_data {
__u32 impl_id;
__u32 capabilities;
};
or equivalent.
Advantages:
1. User space gets a well defined interface
2. It will be easier to create a uniform API for the coming kernel internal
API
3. The subsystem could offload more from the driver.
Disadvantages:
1. Risk that we'll need to extend the API if a field is missing for some
TEE.
I'd like some feedback on providing a well defined user space API. Note
that the user space API shouldn't enforce a certain protocol between
normal and secure world.
--
Thanks,
Jens
Hi,
Here's my latest patches for the generic TEE subsystem. There's two
patches with incompatible driver API changes which makes the OP-TEE
driver fail to compile. I'm fixing that in
"optee: incompatible tee subsystem api change" as a separate commit to
make it easy to squash the different patches later when we send it to
the kernel mailing list.
While this review is ongoing the patches are also available at
https://github.com/jenswi-linaro/linux/tree/tee-dev3
I'll make a pull request of this when this review is done.
Jens Wiklander (7):
tee: bugfix refcount
tee: fix tee_shm_va2pa() and tee_shm_pa2va()
tee: shared memory from CMA or res_mem
tee: rename struct tee_filp
optee: incompatible tee subsystem api change
tee: update module reference counter
tee: bugfix error handling of tee_shm_alloc()
drivers/sec-hw/Makefile | 3 +-
drivers/sec-hw/optee/Kconfig | 3 +-
drivers/sec-hw/optee/Makefile | 3 +-
drivers/sec-hw/optee/core.c | 222 +++++++++++++++++++++++++++++++++++++++
drivers/sec-hw/optee/optee.c | 201 -----------------------------------
drivers/sec-hw/tee.c | 76 ++++++++------
drivers/sec-hw/tee_private.h | 51 ++++++---
drivers/sec-hw/tee_shm.c | 204 +++++++++++++++++-------------------
drivers/sec-hw/tee_shm_pool.c | 232 +++++++++++++++++++++++++++++++++++++++++
include/linux/sec-hw/tee_drv.h | 75 ++++++++++---
10 files changed, 697 insertions(+), 373 deletions(-)
create mode 100644 drivers/sec-hw/optee/core.c
delete mode 100644 drivers/sec-hw/optee/optee.c
create mode 100644 drivers/sec-hw/tee_shm_pool.c
--
1.9.1
Hi,
I think we should move the generic TEE subsystem to /drivers/tee and
/include/linux/tee since we're only dealing with TEE.
What do you think should we move from sec-hw to tee?
Regards,
Jens
Hi,
Here's some small fixes to the generic TEE subsystem.
I've created a pull request of the same patches at
https://github.com/TrustZoneGenericDriver/linux/pull/1
Jens Wiklander (2):
tee: bugfix and export tee_shm_fd()
tee: bugfix tee_shm_alloc()
drivers/sec-hw/tee.c | 9 ++++++++-
drivers/sec-hw/tee_private.h | 2 --
drivers/sec-hw/tee_shm.c | 7 ++++++-
include/linux/sec-hw/tee_drv.h | 7 +++++++
4 files changed, 21 insertions(+), 4 deletions(-)
--
1.9.1
From: Javier González <javier(a)javigon.com>
Hi,
Here's the proposal I described the other day. The goal is to provide
support for kernel submodules. I encountered some challenges that I
would like to discuss with you:
- Command and parameters: In the patchset Jens sent, all ommunication
with the TEE is opaque. This is good for user space but not for kernel
submodules. I propose adding a tee_cmd and tee_parameters. The value
is opaque and can be flourished by the TEE if necessary.
- Command list: If we want kernel submodules to use the TEE as they use
TPM we need a list of commands that all (most) TEEs would support. We
need to have this discussion and maybe bring more parties to it.
Probably Global Platform's use cases are a good place to start.
- Session: I miss the concept of a session. The responsability is very
similar to tee_filp. I would suggest to change the name to tee_session.
I believe that it makes it more clear.
- Position: I like sec-hw :) But we need to bring at least another piece
of secure hardware to this location in order to motivate a new
submodule. TPM is the most obvious. We would then need to move all
into /drivers/sec-hw/?? I assume your do not like trustzone since it
is very specific for some of you - is tee good? I did not want to
send a patch without discussing the naming first.
Finally, regarding the process: is sending patches, discussing, and then
applying to github a process you all fell comfortable with? Suggestions
are welcome.
Best,
Javier
Javier González (1):
tee: add tee operations for kernel submodules
drivers/sec-hw/tee.c | 175 +++++++++++++++++++++++++++++++++++++++--
drivers/sec-hw/tee_private.h | 14 ++++
include/linux/sec-hw/tee.h | 98 ++++++++++++++++++++++-
include/linux/sec-hw/tee_drv.h | 11 ---
4 files changed, 279 insertions(+), 19 deletions(-)
--
1.9.1
Hi,
In this patch I've tried to summarize the recent discussion. I've defined
the needed ioctls and a brief description of how the other relevant
syscalls are used.
Regards,
Jens
Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org>
---
Documentation/ioctl/ioctl-number.txt | 1 +
include/linux/sechw/tee.h | 154 +++++++++++++++++++++++++++++++++++
2 files changed, 155 insertions(+)
create mode 100644 include/linux/sechw/tee.h
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 8136e1f..a04c139 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -301,6 +301,7 @@ Code Seq#(hex) Include File Comments
0xA3 80-8F Port ACL in development:
<mailto:tlewis@mindspring.com>
0xA3 90-9F linux/dtlk.h
+0xA4 00-1F linux/sechw/tee.h Generic TEE driver
0xAB 00-1F linux/nbd.h
0xAC 00-1F linux/raw.h
0xAD 00 Netfilter device in development:
diff --git a/include/linux/sechw/tee.h b/include/linux/sechw/tee.h
new file mode 100644
index 0000000..0c44d5d
--- /dev/null
+++ b/include/linux/sechw/tee.h
@@ -0,0 +1,154 @@
+/*
+ * Copyright (c) 2015, Linaro Limited
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __TEE_H
+#define __TEE_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/*
+ * This file describes the API provided by the generic TEE driver to user
+ * space
+ */
+
+#define TEE_GENDRV_MAJOR_VERSION 1
+#define TEE_GENDRV_MINOR_VERSION 0
+
+/**
+ * struct tee_version - TEE versions
+ * @gendrv_major_version: Generic TEE driver major version
+ * @gendrv_minor_version: Generic TEE driver minor version
+ * @specdrv_major_version: Specific TEE driver major version
+ * @specdrv_minor_version: Specific TEE driver minor version
+ * @tee_api_major_version: Specific TEE API major version
+ * @tee_api_minor_version: Specific TEE API minor version
+ * @tee_os_major_version: Secure OS major version
+ * @tee_os_minor_version: Secure OS minor version
+ * @tee_api_uuid: Specific TEE API uuid
+ * @tee_os_uuid: Secure OS uuid
+ *
+ * Identifies the generic TEE driver, the specific TEE driver, which API
+ * is used to communicate with the Secure OS and the Secure OS itself.
+ *
+ * Unused fields are zeroed.
+ */
+struct tee_version {
+ uint32_t gendrv_major_version;
+ uint32_t gendrv_minor_version;
+ uint32_t specdrv_major_version;
+ uint32_t specdrv_minor_version;
+ uint32_t tee_api_major_version;
+ uint32_t tee_api_minor_version;
+ uint32_t tee_os_major_version;
+ uint32_t tee_os_minor_version;
+ uint8_t tee_api_uuid[16];
+ uint8_t tee_os_uuid[16];
+};
+
+/**
+ * struct tee_cmd_data - Opaque command argument
+ * @buf_ptr: A __user pointer to a command buffer
+ * @buf_len: Length of the buffer above
+ *
+ * Opaque command data which is passed on to the specific driver. The command
+ * buffer doesn't have to reside in shared memory.
+ */
+struct tee_cmd_data {
+ uint64_t buf_ptr;
+ uint64_t buf_len;
+};
+
+/**
+ * struct tee_shm_alloc_data - Shared memory allocate argument
+ * @size: Size of shared memory to allocate
+ * @flags: Flags to/from allocation, currently zero
+ * @fd: File descriptor of the shared memory
+ */
+struct tee_shm_alloc_data {
+ uint64_t size;
+ uint32_t flags;
+ int32_t fd;
+};
+
+/**
+ * struct tee_mem_share_data - share user space memory with Secure OS
+ * @ptr: A __user pointer to memory to share
+ * @size: Size of the memory to share
+ * @flags: Flags to/from sharing, currently set to zero by caller
+ * @pad: Padding, set to zero by caller
+ */
+struct tee_mem_share_data {
+ uint64_t ptr;
+ uint64_t size;
+ uint32_t flags;
+ uint32_t pad;
+};
+
+#define TEE_IOC_MAGIC 0xa4
+#define TEE_IOC_BASE 0
+
+#define _TEE_IOR(nr, size) _IOR(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr), size)
+#define _TEE_IOWR(nr, size) _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr), size)
+
+/**
+ * TEE_IOC_VERSION - query version of drivers and secure OS
+ *
+ * Takes a tee_version struct and returns with the version numbers filled in.
+ */
+#define TEE_IOC_VERSION _TEE_IOR(0, struct tee_version)
+
+/**
+ * TEE_IOC_CMD - pass a command to the specific TEE driver
+ *
+ * Takes tee_cmd_data struct which is passed to the specific TEE driver.
+ */
+#define TEE_IOC_CMD _TEE_IOR(1, struct tee_cmd_data)
+
+/**
+ * TEE_IOC_SHM_ALLOC - allocate shared memory
+ *
+ * Allocates shared memory between the user space process and secure OS.
+ * The returned file descriptor is used to map the shared memory into user
+ * space. The shared memory is freed when the descriptor is closed and the
+ * memory is unmapped.
+ */
+#define TEE_IOC_SHM_ALLOC _TEE_IOWR(2, struct tee_shm_alloc_data)
+
+/**
+ * TEE_IOC_MEM_SHARE - share a portion of user space memory with secure OS
+ *
+ * Shares a portion of user space memory with secure OS.
+ */
+#define TEE_IOC_MEM_SHARE _TEE_IOWR(3, struct tee_mem_share_data)
+
+/**
+ * TEE_IOC_MEM_UNSHARE - unshares a portion shared user space memory
+ *
+ * Unshares a portion of previously shared user space memory.
+ */
+#define TEE_IOC_MEM_UNSHARE _TEE_IOWR(4, struct tee_mem_share_data)
+
+/*
+ * Five syscalls are used when communicating with the generic TEE driver.
+ * open(): opens the device associated with the driver
+ * ioctl(): as described above operating on the file descripto from open()
+ * close(): two cases
+ * - closes the device file descriptor
+ * - closes a file descriptor connected to allocated shared memory
+ * mmap(): maps shared memory into user space
+ * munmap(): unmaps previously shared memory
+ */
+
+#endif /*__TEE_H*/
--
1.9.1
So to get started with the IOCTL's needed between TEE clients in user
space and the generic driver. Have the different use cases in back of
your mind when thinking about this.
Here are three IOC's needed. What else?
IOCTL's:
TEE_IOC_CMD
TEE_IOC_SHM_ALLOC
TEE_IOC_SHM_FREE
--
Regards,
Joakim B
