From: Javier González javier@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(-)
From: Javier González javier@javigon.com
Add tee operations that are available to kernel submodules.
TODO: Discuss parameters when sending a command to TEE
Signed-off-by: Javier González javier@javigon.com --- 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(-)
diff --git a/drivers/sec-hw/tee.c b/drivers/sec-hw/tee.c index df4a359..55731ae 100644 --- a/drivers/sec-hw/tee.c +++ b/drivers/sec-hw/tee.c @@ -19,7 +19,142 @@ #include <linux/uaccess.h> #include "tee_private.h"
-static int tee_open(struct inode *inode, struct file *filp) +static LIST_HEAD(tee_dev_list); +static DEFINE_SPINLOCK(driver_lock); +static DECLARE_BITMAP(dev_mask, TEE_NUM_DEVICES); + +static struct tee_device *tee_dev_get(u32 dev_id) +{ + struct tee_device *pos, *chip = NULL; + + rcu_read_lock(); + list_for_each_entry_rcu(pos, &tee_dev_list, list_dev) { + if (dev_id != TEE_ANY_NUM && dev_id != pos->dev_id) + continue; + + chip = pos; + break; + } + + rcu_read_unlock(); + return chip; +} + +static int __tee_open(struct tee_device *teedev, struct tee_filp *teefilp) +{ + int ret; + + ret = teedev->desc->ops->open(teefilp); + if (ret) { + dev_err(teedev->dev, "tee: open session failed - device: %s\n", + teedev->name); + return ret; + } + + tee_module_put(teedev); + dev_dbg(teedev->dev, "tee: open session succeeded\n"); + return ret; +} + +static void __tee_release(struct tee_device *teedev, struct tee_filp *teefilp) +{ + teedev->desc->ops->release(teefilp); + dev_dbg(teedev->dev, "tee: close session succeeded\n"); +} + +static long __tee_send_cmd(struct tee_device *teedev, struct tee_filp *teefilp, + struct tee_cmd *cmd, struct tee_parameter_list *params) +{ + /* long ret; */ + + /* TODO: Discuss command parameters */ + /* ret = teedev->desc->ops->cmd(teefilp, cmd, params); */ + /* if (ret) { */ + /* dev_err(teedev->dev, "tee: send command failed - device: %s\n", */ + /* teedev->name); */ + /* return ret; */ + /* } */ + + dev_dbg(teedev->dev, "tee: send command succeeded\n"); + /* return ret; */ + return 0; +} + +/* + * TEE in-kernel operations + */ +int tee_open(u32 dev_id, struct tee_filp *teefilp) +{ + struct tee_device *teedev; + struct tee_filp *new_teefilp; + int ret; + + teedev = tee_dev_get(dev_id); + if (!teedev) { + dev_err(teedev->dev, "tee: could not find tee device id: %d\n", + teedev->dev_id); + } + + new_teefilp = kzalloc(sizeof(*teefilp), GFP_KERNEL); + if (!new_teefilp) + return -ENOMEM; + + /* Releasedby client when closing tee session */ + teefilp = new_teefilp; + teefilp->teedev = teedev; + + mutex_lock(&teedev->mutex); + ret = __tee_open(teedev, teefilp); + mutex_unlock(&teedev->mutex); + + if (ret) + kfree(teefilp); + return ret; +} +EXPORT_SYMBOL_GPL(tee_open); + +int tee_release(u32 dev_id, struct tee_filp *teefilp) +{ + struct tee_device *teedev; + + teedev = tee_dev_get(dev_id); + if (!teedev) { + dev_err(teedev->dev, "tee: could not find tee device id: %d\n", + teedev->dev_id); + } + + mutex_lock(&teedev->mutex); + __tee_release(teedev, teefilp); + mutex_unlock(&teedev->mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(tee_release); + +long tee_send_cmd(u32 dev_id, struct tee_filp *teefilp, struct tee_cmd *cmd, + struct tee_parameter_list *params) +{ + struct tee_device *teedev; + long ret = 0; + + teedev = tee_dev_get(dev_id); + if (!teedev) { + dev_err(teedev->dev, "tee: could not find tee device id: %d\n", + teedev->dev_id); + } + + mutex_lock(&teedev->mutex); + ret = __tee_send_cmd(teedev, teefilp, cmd, params); + mutex_unlock(&teedev->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(tee_send_cmd); + +/* + * ioctl operations + */ +static int tee_open_ioctl(struct inode *inode, struct file *filp) { int ret; struct tee_device *teedev; @@ -32,20 +167,28 @@ static int tee_open(struct inode *inode, struct file *filp)
teefilp->teedev = teedev; filp->private_data = teefilp; - ret = teedev->desc->ops->open(teefilp); + + mutex_lock(&teedev->mutex); + ret = __tee_open(teedev, teefilp); + mutex_unlock(&teedev->mutex); + if (ret) kfree(teefilp); return ret; }
-static int tee_release(struct inode *inode, struct file *filp) +static int tee_release_ioctl(struct inode *inode, struct file *filp) { struct tee_filp *teefilp = filp->private_data; + struct tee_device *teedev = teefilp->teedev;
/* Free all shm:s related to this teefilp */ tee_shm_free_by_teefilp(teefilp);
- teefilp->teedev->desc->ops->release(teefilp); + mutex_lock(&teedev->mutex); + __tee_release(teefilp->teedev, teefilp); + mutex_unlock(&teedev->mutex); + return 0; }
@@ -153,11 +296,10 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) } }
- static const struct file_operations tee_fops = { .owner = THIS_MODULE, - .open = tee_open, - .release = tee_release, + .open = tee_open_ioctl, + .release = tee_release_ioctl, .unlocked_ioctl = tee_ioctl };
@@ -187,6 +329,15 @@ struct tee_device *tee_register(const struct tee_desc *teedesc, snprintf(teedev->name, sizeof(teedev->name), "tee%d", atomic_inc_return(&device_no));
+ teedev->dev_id = find_first_zero_bit(dev_mask, TEE_NUM_DEVICES); + + if (teedev->dev_id >= TEE_NUM_DEVICES) { + dev_err(teedev->dev, "no available tee device numbers\n"); + goto err; + } + + set_bit(teedev->dev_id, dev_mask); + teedev->miscdev.parent = dev; teedev->miscdev.minor = MISC_DYNAMIC_MINOR; teedev->miscdev.name = teedev->name; @@ -202,6 +353,10 @@ struct tee_device *tee_register(const struct tee_desc *teedesc, INIT_LIST_HEAD(&teedev->list_shm); mutex_init(&teedev->mutex);
+ spin_lock(&driver_lock); + list_add_rcu(&teedev->list_dev, &tee_dev_list); + spin_unlock(&driver_lock); + dev_set_drvdata(teedev->miscdev.this_device, teedev);
dev_info(dev, "register misc device "%s" (minor=%d)\n", @@ -222,6 +377,12 @@ void tee_unregister(struct tee_device *teedev) dev_info(teedev->dev, "unregister misc device "%s" (minor=%d)\n", dev_name(teedev->miscdev.this_device), teedev->miscdev.minor); misc_deregister(&teedev->miscdev); + + spin_lock(&driver_lock); + list_del_rcu(&teedev->list_dev); + spin_unlock(&driver_lock); + synchronize_rcu(); + /* TODO finish this function */ } EXPORT_SYMBOL_GPL(tee_unregister); diff --git a/drivers/sec-hw/tee_private.h b/drivers/sec-hw/tee_private.h index 7b1668f..52da1b9 100644 --- a/drivers/sec-hw/tee_private.h +++ b/drivers/sec-hw/tee_private.h @@ -10,16 +10,25 @@ * GNU General Public License for more details. * */ +#include <linux/module.h> + #ifndef TEE_PRIVATE_H #define TEE_PRIVATE_H
#define TEE_MAX_DEV_NAME_LEN 32 + +enum tee_const { + TEE_NUM_DEVICES = 8, +}; + struct tee_device { char name[TEE_MAX_DEV_NAME_LEN]; + u32 dev_id; const struct tee_desc *desc; struct device *dev; struct miscdevice miscdev; struct list_head list_shm; + struct list_head list_dev; struct mutex mutex; void *driver_data; }; @@ -35,6 +44,11 @@ struct tee_shm { u32 flags; };
+static inline void tee_module_put(struct tee_device *teedev) +{ + module_put(teedev->dev->driver->owner); +} + int tee_shm_fd(struct tee_shm *shm); void tee_shm_free_by_teefilp(struct tee_filp *teefilp);
diff --git a/include/linux/sec-hw/tee.h b/include/linux/sec-hw/tee.h index f1af46b..616f11f 100644 --- a/include/linux/sec-hw/tee.h +++ b/include/linux/sec-hw/tee.h @@ -23,6 +23,15 @@ * space */
+/** + * struct tee_filp - driver specific file pointer data + * @teedev: pointer to this drivers struct tee_device + * @filp_data: driver specific file pointer data, managed by the driver + */ +struct tee_filp { + struct tee_device *teedev; + void *filp_data; +};
/* Helpers to make the ioctl defines */ #define TEE_IOC_MAGIC 0xa4 @@ -37,6 +46,10 @@ */ #define TEE_SUBSYS_VERSION 1
+/* + * Default device + */ +#define TEE_ANY_NUM 0XFFF
/* Flags relating to shared memory */ #define TEE_IOCTL_SHM_MAPPED 0x1 /* memory mapped in normal world */ @@ -84,6 +97,83 @@ struct tee_ioctl_cmd_data { #define TEE_IOC_CMD _TEE_IOR(1, struct tee_ioctl_cmd_data)
/** + * struct tee_cmd - Command sent to the TEE + * @cmd: command identifier as defined in drivers/sec-hw/trustzone/cmd_id.h or + * in each specific TEE cmd_id.h + * + * Command that is interpreted by the TEE and triggers a trusted service to + * execute. + * TODO: Agree on a minimal list of common trusted services + */ +struct tee_cmd { + u32 cmd; +}; + +/** + * enum tee_param_type - type of the trustzone parameter + * @tee_UINT8: equivalent to uint8_t in the TEE + * @tee_UINT32: equivalent to uint32_t in the TEE + * @tee_GENERIC: parameter sent through shared memory. It is the TEEs + * responsibility to know the type of the parameter. This is used for specific + * commands that are known to the client and the TEE + */ +enum tee_param_type { +tee_UINT8 = 0x0, +tee_UINT32, +tee_GENERIC +}; + +/** + *enum tee_param_purpose - purpose of the trustzone parameter + * @TEE_PARAM_IN: input parameter + * @TEE_PARAM_OUT: output parameter + * @TEE_PARAM_INOUT: input and Output parameter + * + * Parameters sent to the TEE must be identified so that the TEE knows how to + * treat them. + */ +enum tee_param_purpose { +TEE_PARAM_IN = 0x0, +TEE_PARAM_OUT, +TEE_PARAM_INOUT +}; + +/** + * struct tee_parameter - parameter to be sent to the TEE + * @ll_list: list node + * @type: parameter type as in tee_param_type + * @inout: parameter purpose as in tee_param_purpose + * @value: parameter value. Type is used to cast parameter in TEE + * @size: parameter size + * + * The type and number of parameters sent to the TEE might vary from TEE + * framework to TEE framework. This structure allows to hide TEE-specific + * parameter definition and delegates this responsability entirely to the TEE. + * Some specific TEE APIs such as Global Platform's API limit the numer of + * parameters to 4, the layer implementing such APIs are responsible to + * introduce this policy. + */ +struct tee_parameter { + struct llist_node ll_list; + uint8_t type; + uint8_t inout; + void *value; + uint32_t size; +}; + +/** + * struct tee_parameter_list - list of parameters to be sent to the TEE + * @param_list: parameter list head + * @params: parameter + * @n_params: number of parameters + */ +struct tee_parameter_list { + struct llist_head param_list; + struct tee_parameter *params; + u8 n_params; +}; + +/** * struct tee_shm_alloc_data - Shared memory allocate argument * @size: [in/out] Size of shared memory to allocate * @flags: [in/out] Flags to/from allocation. @@ -167,7 +257,8 @@ struct tee_ioctl_mem_share_data { #define TEE_IOC_MEM_UNSHARE _TEE_IOW(4, struct tee_ioctl_mem_share_data)
/* - * Five syscalls are used when communicating with the generic TEE driver. + * Five syscalls are used when communicating with the generic TEE driver from + * user sapce: * open(): opens the device associated with the driver * ioctl(): as described above operating on the file descripto from open() * close(): two cases @@ -177,4 +268,9 @@ struct tee_ioctl_mem_share_data { * munmap(): unmaps previously shared memory */
+extern int tee_open(u32, struct tee_filp*); +extern int tee_release(u32, struct tee_filp*); +extern long tee_send_cmd(u32, struct tee_filp*, struct tee_cmd*, + struct tee_parameter_list*); + #endif /*__TEE_H*/ diff --git a/include/linux/sec-hw/tee_drv.h b/include/linux/sec-hw/tee_drv.h index 8693363..c275849 100644 --- a/include/linux/sec-hw/tee_drv.h +++ b/include/linux/sec-hw/tee_drv.h @@ -33,17 +33,6 @@ struct tee_device; struct tee_shm;
- -/** - * struct tee_filp - driver specific file pointer data - * @teedev: pointer to this drivers struct tee_device - * @filp_data: driver specific file pointer data, managed by the driver - */ -struct tee_filp { - struct tee_device *teedev; - void *filp_data; -}; - /** * struct tee_driver_ops - driver operations vtable * @get_version: returns version of driver
Hi Javier,
On Tue, Mar 31, 2015 at 5:34 PM, javigon.napster@gmail.com wrote:
From: Javier González javier@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.
That would be inconvenient for at least OP-TEE. In the current state where marshaling the parameters in user space, updating some pointers and shared memory references in the driver and then passing it on to secure world. What you're proposing would be to first marshal once in user space, and then once more in kernel space using two different ways for formatting the parameters.
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.
In the OP-TEE case the tee_filp corresponds to a TEE_Context, if we call it tee_session in the kernel it will be confusing as we store the TEE sessions inside the context.
The TEE subsystem is very thin and doesn't provide much, except some helpers to create a device and manage shared memory. But it solves one other problem common to all tee drivers, it provides a place in the kernel tree where to put the driver. I think it's an advantage to make the TEE subsystem in small steps. Currently the the TEE subsystem is a bit more than 1100 lines of code and not very complicated.
I think that what's currently on github is very close to what we need for the first step (I have a couple of small patches that I'd like to get in also, but nothing dramatic). I would prefer we didn't add more generic features to the subsystem and instead focused on getting what we have in shape to upstream.
- 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.
Are you proposing storing it under /drivers/tee instead of /drivers/sec-hw? I'm in favor of that since tee describes better what we're doing. We are communicating with software running in the secure environment, not interfacing with some secure crypto device etc.
Finally, regarding the process: is sending patches, discussing, and then applying to github a process you all fell comfortable with? Suggestions are welcome.
Fine with me. I think it would be good if we did all the merging on github via pull requests, that way it's very clear what will be merged.
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
Tee-dev mailing list Tee-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/tee-dev
Hi Jens,
On 01 Apr 2015, at 09:20, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi Javier,
On Tue, Mar 31, 2015 at 5:34 PM, javigon.napster@gmail.com wrote:
From: Javier González javier@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.
That would be inconvenient for at least OP-TEE. In the current state where marshaling the parameters in user space, updating some pointers and shared memory references in the driver and then passing it on to secure world. What you're proposing would be to first marshal once in user space, and then once more in kernel space using two different ways for formatting the parameters.
Yes. We could maintain two different functions for it, so that user space communication through ioctl only needs to be formatted once. I do not know the internals of OP-TEE, but this works nice in OTZ; when a command and parameters reach the specific driver, they are formatted so that the TEE can understand it while the interdace is the same for the kernel.
- 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.
In the OP-TEE case the tee_filp corresponds to a TEE_Context, if we call it tee_session in the kernel it will be confusing as we store the TEE sessions inside the context.
Ok. Can we call it tee_context then, so that it describes it better when not using ioctl?
The TEE subsystem is very thin and doesn't provide much, except some helpers to create a device and manage shared memory. But it solves one other problem common to all tee drivers, it provides a place in the kernel tree where to put the driver. I think it's an advantage to make the TEE subsystem in small steps. Currently the the TEE subsystem is a bit more than 1100 lines of code and not very complicated.
Agreed.
I think that what's currently on github is very close to what we need for the first step (I have a couple of small patches that I'd like to get in also, but nothing dramatic). I would prefer we didn't add more generic features to the subsystem and instead focused on getting what we have in shape to upstream.
Ok. I would still like to add the in-kernel functionality. I think that it is a big part of the contribution providing the framework for in-kernel TEE operations. It would also help maintaining an agnostic but not opaque interface. We can look at trusted services later on.
- 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.
Are you proposing storing it under /drivers/tee instead of /drivers/sec-hw? I'm in favor of that since tee describes better what we're doing. We are communicating with software running in the secure environment, not interfacing with some secure crypto device etc.
My initial though was a trustzone driver, therefore the sec-hw idea. Given the move towards tee-agnostic I am now between /drivers/tee and /drivers/sec-hw/tee - since hardware is still necessary to support the TEE. But I am more and more open to having /drivers/tee :)
Finally, regarding the process: is sending patches, discussing, and then applying to github a process you all fell comfortable with? Suggestions are welcome.
Fine with me. I think it would be good if we did all the merging on github via pull requests, that way it's very clear what will be merged.
Sounds good. But I assume we maintain RFC in patch mailing list?
Best, Javier.
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
Tee-dev mailing list Tee-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/tee-dev
On Wed, Apr 01, 2015 at 09:47:26AM +0200, javigon wrote:
Hi Jens,
On 01 Apr 2015, at 09:20, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi Javier,
On Tue, Mar 31, 2015 at 5:34 PM, javigon.napster@gmail.com wrote:
From: Javier González javier@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.
That would be inconvenient for at least OP-TEE. In the current state where marshaling the parameters in user space, updating some pointers and shared memory references in the driver and then passing it on to secure world. What you're proposing would be to first marshal once in user space, and then once more in kernel space using two different ways for formatting the parameters.
Yes. We could maintain two different functions for it, so that user space communication through ioctl only needs to be formatted once. I do not know the internals of OP-TEE, but this works nice in OTZ; when a command and parameters reach the specific driver, they are formatted so that the TEE can understand it while the interdace is the same for the kernel.
OK, where should the formatting be done? In a new tee_cmd_kernel() callback?
- 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.
In the OP-TEE case the tee_filp corresponds to a TEE_Context, if we call it tee_session in the kernel it will be confusing as we store the TEE sessions inside the context.
Ok. Can we call it tee_context then, so that it describes it better when not using ioctl?
OK
The TEE subsystem is very thin and doesn't provide much, except some helpers to create a device and manage shared memory. But it solves one other problem common to all tee drivers, it provides a place in the kernel tree where to put the driver. I think it's an advantage to make the TEE subsystem in small steps. Currently the the TEE subsystem is a bit more than 1100 lines of code and not very complicated.
Agreed.
I think that what's currently on github is very close to what we need for the first step (I have a couple of small patches that I'd like to get in also, but nothing dramatic). I would prefer we didn't add more generic features to the subsystem and instead focused on getting what we have in shape to upstream.
Ok. I would still like to add the in-kernel functionality. I think that it is a big part of the contribution providing the framework for in-kernel TEE operations. It would also help maintaining an agnostic but not opaque interface. We can look at trusted services later on.
OK
- 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.
Are you proposing storing it under /drivers/tee instead of /drivers/sec-hw? I'm in favor of that since tee describes better what we're doing. We are communicating with software running in the secure environment, not interfacing with some secure crypto device etc.
My initial though was a trustzone driver, therefore the sec-hw idea. Given the move towards tee-agnostic I am now between /drivers/tee and /drivers/sec-hw/tee - since hardware is still necessary to support the TEE. But I am more and more open to having /drivers/tee :)
:-)
Finally, regarding the process: is sending patches, discussing, and then applying to github a process you all fell comfortable with? Suggestions are welcome.
Fine with me. I think it would be good if we did all the merging on github via pull requests, that way it's very clear what will be merged.
Sounds good. But I assume we maintain RFC in patch mailing list?
Sure.
-- Regards, Jens
Hi,
On 01 Apr 2015, at 11:35, Jens Wiklander jens.wiklander@linaro.org wrote:
On Wed, Apr 01, 2015 at 09:47:26AM +0200, javigon wrote:
Hi Jens,
On 01 Apr 2015, at 09:20, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi Javier,
On Tue, Mar 31, 2015 at 5:34 PM, javigon.napster@gmail.com wrote:
From: Javier González javier@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.
That would be inconvenient for at least OP-TEE. In the current state where marshaling the parameters in user space, updating some pointers and shared memory references in the driver and then passing it on to secure world. What you're proposing would be to first marshal once in user space, and then once more in kernel space using two different ways for formatting the parameters.
Yes. We could maintain two different functions for it, so that user space communication through ioctl only needs to be formatted once. I do not know the internals of OP-TEE, but this works nice in OTZ; when a command and parameters reach the specific driver, they are formatted so that the TEE can understand it while the interdace is the same for the kernel.
OK, where should the formatting be done? In a new tee_cmd_kernel() callback?
Sounds good to me. Here we parse the command and add the parameter list.
- 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.
In the OP-TEE case the tee_filp corresponds to a TEE_Context, if we call it tee_session in the kernel it will be confusing as we store the TEE sessions inside the context.
Ok. Can we call it tee_context then, so that it describes it better when not using ioctl?
OK
The TEE subsystem is very thin and doesn't provide much, except some helpers to create a device and manage shared memory. But it solves one other problem common to all tee drivers, it provides a place in the kernel tree where to put the driver. I think it's an advantage to make the TEE subsystem in small steps. Currently the the TEE subsystem is a bit more than 1100 lines of code and not very complicated.
Agreed.
I think that what's currently on github is very close to what we need for the first step (I have a couple of small patches that I'd like to get in also, but nothing dramatic). I would prefer we didn't add more generic features to the subsystem and instead focused on getting what we have in shape to upstream.
Ok. I would still like to add the in-kernel functionality. I think that it is a big part of the contribution providing the framework for in-kernel TEE operations. It would also help maintaining an agnostic but not opaque interface. We can look at trusted services later on.
OK
- 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.
Are you proposing storing it under /drivers/tee instead of /drivers/sec-hw? I'm in favor of that since tee describes better what we're doing. We are communicating with software running in the secure environment, not interfacing with some secure crypto device etc.
My initial though was a trustzone driver, therefore the sec-hw idea. Given the move towards tee-agnostic I am now between /drivers/tee and /drivers/sec-hw/tee - since hardware is still necessary to support the TEE. But I am more and more open to having /drivers/tee :)
:-)
If the rest agree, let’s do it then :)
Finally, regarding the process: is sending patches, discussing, and then applying to github a process you all fell comfortable with? Suggestions are welcome.
Fine with me. I think it would be good if we did all the merging on github via pull requests, that way it's very clear what will be merged.
Sounds good. But I assume we maintain RFC in patch mailing list?
Sure.
-- Regards, Jens
Best, Javier
-
javigon -
javigon.napster＠gmail.com
-
Jens Wiklander