* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-17 7:50 ` Jens Wiklander
0 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-17 7:50 UTC (permalink / raw)
To: linux-arm-kernel
Initial patch for generic TEE subsystem.
This subsystem provides:
* Registration/un-registration of TEE drivers.
* Shared memory between normal world and secure world.
* Ioctl interface for interaction with user space.
A TEE (Trusted Execution Environment) driver is a driver that interfaces
with a trusted OS running in some secure environment, for example,
TrustZone on ARM cpus, or a separate secure co-processor etc.
To avoid putting unnecessary restrictions on the TEE driver and the
trusted OS the TEE_IOC_CMD passes an opaque buffer to the TEE driver to
facilitate a communication channel between user space and the trusted
OS.
The TEE subsystem can serve a TEE driver for a Global Platform compliant
TEE, but it's not limited to only Global Platform TEEs.
This patch builds on other similar implementations trying to solve
the same problem:
* "optee_linuxdriver" by among others
Jean-michel DELORME<jean-michel.delorme@st.com> and
Emmanuel MICHEL <emmanuel.michel@st.com>
* "Generic TrustZone Driver" by Javier Gonz?lez <javier@javigon.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
Documentation/ioctl/ioctl-number.txt | 1 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/tee/Kconfig | 8 +
drivers/tee/Makefile | 3 +
drivers/tee/tee.c | 253 +++++++++++++++++++++++++++
drivers/tee/tee_private.h | 64 +++++++
drivers/tee/tee_shm.c | 330 +++++++++++++++++++++++++++++++++++
drivers/tee/tee_shm_pool.c | 246 ++++++++++++++++++++++++++
include/linux/tee/tee.h | 180 +++++++++++++++++++
include/linux/tee/tee_drv.h | 271 ++++++++++++++++++++++++++++
11 files changed, 1359 insertions(+)
create mode 100644 drivers/tee/Kconfig
create mode 100644 drivers/tee/Makefile
create mode 100644 drivers/tee/tee.c
create mode 100644 drivers/tee/tee_private.h
create mode 100644 drivers/tee/tee_shm.c
create mode 100644 drivers/tee/tee_shm_pool.c
create mode 100644 include/linux/tee/tee.h
create mode 100644 include/linux/tee/tee_drv.h
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 8136e1f..6e9bd04 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/sec-hw/tee.h Generic TEE subsystem
0xAB 00-1F linux/nbd.h
0xAC 00-1F linux/raw.h
0xAD 00 Netfilter device in development:
diff --git a/drivers/Kconfig b/drivers/Kconfig
index c0cc96b..7510f69 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -182,4 +182,6 @@ source "drivers/thunderbolt/Kconfig"
source "drivers/android/Kconfig"
+source "drivers/tee/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 527a6da..0d24e70 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/
obj-$(CONFIG_THUNDERBOLT) += thunderbolt/
obj-$(CONFIG_CORESIGHT) += coresight/
obj-$(CONFIG_ANDROID) += android/
+obj-$(CONFIG_TEE) += tee/
diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
new file mode 100644
index 0000000..64a8cd7
--- /dev/null
+++ b/drivers/tee/Kconfig
@@ -0,0 +1,8 @@
+# Generic Trusted Execution Environment Configuration
+config TEE
+ bool "Trusted Execution Environment support"
+ default n
+ select DMA_SHARED_BUFFER
+ help
+ This implements a generic interface towards a Trusted Execution
+ Environment (TEE).
diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
new file mode 100644
index 0000000..60d2dab
--- /dev/null
+++ b/drivers/tee/Makefile
@@ -0,0 +1,3 @@
+obj-y += tee.o
+obj-y += tee_shm.o
+obj-y += tee_shm_pool.o
diff --git a/drivers/tee/tee.c b/drivers/tee/tee.c
new file mode 100644
index 0000000..23a6e75
--- /dev/null
+++ b/drivers/tee/tee.c
@@ -0,0 +1,253 @@
+/*
+ * 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.
+ *
+ */
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/tee/tee_drv.h>
+#include "tee_private.h"
+
+static int tee_open(struct inode *inode, struct file *filp)
+{
+ int ret;
+ struct tee_device *teedev;
+ struct tee_context *ctx;
+
+ teedev = container_of(filp->private_data, struct tee_device, miscdev);
+ if (!try_module_get(teedev->desc->owner))
+ return -EINVAL;
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->teedev = teedev;
+ filp->private_data = ctx;
+ ret = teedev->desc->ops->open(ctx);
+ if (ret) {
+ kfree(ctx);
+ module_put(teedev->desc->owner);
+ }
+ return ret;
+}
+
+static int tee_release(struct inode *inode, struct file *filp)
+{
+ struct tee_context *ctx = filp->private_data;
+ struct tee_device *teedev = ctx->teedev;
+
+ ctx->teedev->desc->ops->release(ctx);
+ kfree(ctx);
+ module_put(teedev->desc->owner);
+ return 0;
+}
+
+static long tee_ioctl_version(struct tee_context *ctx,
+ struct tee_ioctl_version __user *uvers)
+{
+ struct tee_ioctl_version vers;
+
+ memset(&vers, 0, sizeof(vers));
+ vers.gen_version = TEE_SUBSYS_VERSION;
+ ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
+ vers.uuid);
+
+ return copy_to_user(uvers, &vers, sizeof(vers));
+}
+
+static long tee_ioctl_cmd(struct tee_context *ctx,
+ struct tee_ioctl_cmd_data __user *ucmd)
+{
+ long ret;
+ struct tee_ioctl_cmd_data cmd;
+ void __user *buf_ptr;
+
+ ret = copy_from_user(&cmd, ucmd, sizeof(cmd));
+ if (ret)
+ return ret;
+
+ buf_ptr = (void __user *)(uintptr_t)cmd.buf_ptr;
+ return ctx->teedev->desc->ops->cmd(ctx, buf_ptr, cmd.buf_len);
+}
+
+static long tee_ioctl_shm_alloc(struct tee_context *ctx,
+ struct tee_ioctl_shm_alloc_data __user *udata)
+{
+ long ret;
+ struct tee_ioctl_shm_alloc_data data;
+ struct tee_shm *shm;
+
+ if (copy_from_user(&data, udata, sizeof(data)))
+ return -EFAULT;
+
+ /* Currently no input flags are supported */
+ if (data.flags)
+ return -EINVAL;
+
+ data.fd = -1;
+
+ shm = tee_shm_alloc(ctx->teedev, data.size,
+ TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+ if (IS_ERR(shm))
+ return PTR_ERR(shm);
+
+ ret = ctx->teedev->desc->ops->shm_share(shm);
+ if (ret)
+ goto err;
+
+ data.flags = shm->flags;
+ data.size = shm->size;
+ data.fd = tee_shm_get_fd(shm);
+ if (data.fd < 0) {
+ ret = data.fd;
+ goto err;
+ }
+
+ if (copy_to_user(udata, &data, sizeof(data))) {
+ ret = -EFAULT;
+ goto err;
+ }
+ /*
+ * When user space closes the file descriptor the shared memory
+ * should be freed
+ */
+ tee_shm_put(shm);
+ return 0;
+err:
+ if (data.fd >= 0)
+ tee_shm_put_fd(data.fd);
+ tee_shm_free(shm);
+ return ret;
+}
+
+static long tee_ioctl_mem_share(struct tee_context *ctx,
+ struct tee_ioctl_mem_share_data __user *udata)
+{
+ /* Not supported yet */
+ return -ENOSYS;
+}
+
+static long tee_ioctl_mem_unshare(struct tee_context *ctx,
+ struct tee_ioctl_mem_share_data __user *udata)
+{
+ /* Not supported yet */
+ return -ENOSYS;
+}
+
+static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ struct tee_context *ctx = filp->private_data;
+ void __user *uarg = (void __user *)arg;
+
+ switch (cmd) {
+ case TEE_IOC_VERSION:
+ return tee_ioctl_version(ctx, uarg);
+ case TEE_IOC_CMD:
+ return tee_ioctl_cmd(ctx, uarg);
+ case TEE_IOC_SHM_ALLOC:
+ return tee_ioctl_shm_alloc(ctx, uarg);
+ case TEE_IOC_MEM_SHARE:
+ return tee_ioctl_mem_share(ctx, uarg);
+ case TEE_IOC_MEM_UNSHARE:
+ return tee_ioctl_mem_unshare(ctx, uarg);
+ default:
+ return -EINVAL;
+ }
+}
+
+
+static const struct file_operations tee_fops = {
+ .owner = THIS_MODULE,
+ .open = tee_open,
+ .release = tee_release,
+ .unlocked_ioctl = tee_ioctl
+};
+
+struct tee_device *tee_register(const struct tee_desc *teedesc,
+ struct device *dev, struct tee_shm_pool *pool,
+ void *driver_data)
+{
+ static atomic_t device_no = ATOMIC_INIT(-1);
+ static atomic_t priv_device_no = ATOMIC_INIT(-1);
+ struct tee_device *teedev;
+ void *ret;
+ int rc;
+
+ if (!teedesc || !teedesc->name || !dev || !pool) {
+ ret = ERR_PTR(-EINVAL);
+ goto err;
+ }
+
+ teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
+ if (!teedev) {
+ ret = ERR_PTR(-ENOMEM);
+ goto err;
+ }
+
+ teedev->dev = dev;
+ teedev->desc = teedesc;
+ teedev->pool = pool;
+ teedev->driver_data = driver_data;
+
+ if (teedesc->flags & TEE_DESC_PRIVILEGED)
+ snprintf(teedev->name, sizeof(teedev->name),
+ "teepriv%d", atomic_inc_return(&priv_device_no));
+ else
+ snprintf(teedev->name, sizeof(teedev->name),
+ "tee%d", atomic_inc_return(&device_no));
+
+ teedev->miscdev.parent = dev;
+ teedev->miscdev.minor = MISC_DYNAMIC_MINOR;
+ teedev->miscdev.name = teedev->name;
+ teedev->miscdev.fops = &tee_fops;
+
+ rc = misc_register(&teedev->miscdev);
+ if (rc) {
+ dev_err(dev, "misc_register() failed name=\"%s\"\n",
+ teedev->name);
+ ret = ERR_PTR(rc);
+ goto err;
+ }
+
+ INIT_LIST_HEAD(&teedev->list_shm);
+
+ dev_set_drvdata(teedev->miscdev.this_device, teedev);
+
+ dev_dbg(dev, "register misc device \"%s\" (minor=%d)\n",
+ dev_name(teedev->miscdev.this_device), teedev->miscdev.minor);
+
+ return teedev;
+err:
+ dev_err(dev, "could not register %s driver\n",
+ teedesc->flags & TEE_DESC_PRIVILEGED ? "privileged" : "client");
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_register);
+
+void tee_unregister(struct tee_device *teedev)
+{
+ if (!teedev)
+ return;
+
+ dev_dbg(teedev->dev, "unregister misc device \"%s\" (minor=%d)\n",
+ dev_name(teedev->miscdev.this_device), teedev->miscdev.minor);
+ misc_deregister(&teedev->miscdev);
+}
+EXPORT_SYMBOL_GPL(tee_unregister);
+
+void *tee_get_drvdata(struct tee_device *teedev)
+{
+ return teedev->driver_data;
+}
+EXPORT_SYMBOL_GPL(tee_get_drvdata);
diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h
new file mode 100644
index 0000000..2199634
--- /dev/null
+++ b/drivers/tee/tee_private.h
@@ -0,0 +1,64 @@
+/*
+ * 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_PRIVATE_H
+#define TEE_PRIVATE_H
+
+struct tee_device;
+
+struct tee_shm {
+ struct list_head list_node;
+ struct tee_device *teedev;
+ phys_addr_t paddr;
+ void *kaddr;
+ size_t size;
+ struct dma_buf *dmabuf;
+ struct page *pages;
+ u32 flags;
+};
+
+struct tee_shm_pool_mgr;
+struct tee_shm_pool_mgr_ops {
+ int (*alloc)(struct tee_shm_pool_mgr *poolmgr, struct tee_shm *shm,
+ size_t size);
+ void (*free)(struct tee_shm_pool_mgr *poolmgr, struct tee_shm *shm);
+};
+
+struct tee_shm_pool_mgr {
+ const struct tee_shm_pool_mgr_ops *ops;
+ void *private_data;
+};
+
+struct tee_shm_pool {
+ struct tee_shm_pool_mgr private_mgr;
+ struct tee_shm_pool_mgr dma_buf_mgr;
+ void (*destroy)(struct tee_shm_pool *pool);
+ void *private_data;
+};
+
+#define TEE_MAX_DEV_NAME_LEN 32
+struct tee_device {
+ char name[TEE_MAX_DEV_NAME_LEN];
+ const struct tee_desc *desc;
+ struct device *dev;
+ struct miscdevice miscdev;
+
+ void *driver_data;
+
+ struct list_head list_shm;
+ struct tee_shm_pool *pool;
+};
+
+int tee_shm_init(void);
+
+#endif /*TEE_PRIVATE_H*/
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
new file mode 100644
index 0000000..38359ad
--- /dev/null
+++ b/drivers/tee/tee_shm.c
@@ -0,0 +1,330 @@
+/*
+ * 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.
+ *
+ */
+#include <linux/device.h>
+#include <linux/fdtable.h>
+#include <linux/sched.h>
+#include <linux/dma-buf.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/tee/tee_drv.h>
+#include "tee_private.h"
+
+/* Mutex for all shm objects and lists */
+static DEFINE_MUTEX(teeshm_mutex);
+
+static void tee_shm_release(struct tee_shm *shm);
+
+static struct sg_table *tee_shm_op_map_dma_buf(struct dma_buf_attachment
+ *attach, enum dma_data_direction dir)
+{
+ return NULL;
+}
+
+static void tee_shm_op_unmap_dma_buf(struct dma_buf_attachment *attach,
+ struct sg_table *table, enum dma_data_direction dir)
+{
+}
+
+static void tee_shm_op_release(struct dma_buf *dmabuf)
+{
+ struct tee_shm *shm = dmabuf->priv;
+
+ tee_shm_release(shm);
+}
+
+static void *tee_shm_op_kmap_atomic(struct dma_buf *dmabuf,
+ unsigned long pgnum)
+{
+ return NULL;
+}
+
+static void *tee_shm_op_kmap(struct dma_buf *dmabuf, unsigned long pgnum)
+{
+ return NULL;
+}
+
+static int tee_shm_op_mmap(struct dma_buf *dmabuf,
+ struct vm_area_struct *vma)
+{
+ struct tee_shm *shm = dmabuf->priv;
+ size_t size = vma->vm_end - vma->vm_start;
+
+ return remap_pfn_range(vma, vma->vm_start, shm->paddr >> PAGE_SHIFT,
+ size, vma->vm_page_prot);
+}
+
+static struct dma_buf_ops tee_shm_dma_buf_ops = {
+ .map_dma_buf = tee_shm_op_map_dma_buf,
+ .unmap_dma_buf = tee_shm_op_unmap_dma_buf,
+ .release = tee_shm_op_release,
+ .kmap_atomic = tee_shm_op_kmap_atomic,
+ .kmap = tee_shm_op_kmap,
+ .mmap = tee_shm_op_mmap,
+};
+
+struct tee_shm *tee_shm_alloc(struct tee_device *teedev, size_t size,
+ u32 flags)
+{
+ struct tee_shm_pool_mgr *poolm = NULL;
+ struct tee_shm *shm;
+ void *ret;
+ int rc;
+
+ if (!(flags & TEE_SHM_MAPPED)) {
+ dev_err(teedev->dev, "only mapped allocations supported\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ if ((flags & ~(TEE_SHM_MAPPED|TEE_SHM_DMA_BUF))) {
+ dev_err(teedev->dev, "invalid shm flags 0x%x", flags);
+ return ERR_PTR(-EINVAL);
+ }
+
+ shm = kzalloc(sizeof(struct tee_shm), GFP_KERNEL);
+ if (!shm)
+ return ERR_PTR(-ENOMEM);
+
+ if (!try_module_get(teedev->desc->owner)) {
+ ret = ERR_PTR(-EINVAL);
+ goto err;
+ }
+
+ shm->flags = flags;
+ shm->teedev = teedev;
+ if (flags & TEE_SHM_DMA_BUF)
+ poolm = &teedev->pool->dma_buf_mgr;
+ else
+ poolm = &teedev->pool->private_mgr;
+
+ rc = poolm->ops->alloc(poolm, shm, size);
+ if (rc) {
+ ret = ERR_PTR(rc);
+ goto err;
+ }
+
+ mutex_lock(&teeshm_mutex);
+ list_add_tail(&shm->list_node, &teedev->list_shm);
+ mutex_unlock(&teeshm_mutex);
+
+ if (flags & TEE_SHM_DMA_BUF) {
+ shm->dmabuf = dma_buf_export(shm, &tee_shm_dma_buf_ops,
+ shm->size, O_RDWR, NULL);
+ if (IS_ERR(shm->dmabuf)) {
+ ret = ERR_CAST(shm->dmabuf);
+ goto err;
+ }
+
+ /*
+ * Only call share on dma_buf shm:s, as the driver private
+ * shm:s always originates from the driver itself.
+ */
+ rc = teedev->desc->ops->shm_share(shm);
+ if (rc) {
+ dma_buf_put(shm->dmabuf);
+ return ERR_PTR(rc);
+ }
+ shm->flags |= __TEE_SHM_SHARED;
+ }
+
+ return shm;
+err:
+ if (poolm && shm->kaddr)
+ poolm->ops->free(poolm, shm);
+ kfree(shm);
+ module_put(teedev->desc->owner);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_shm_alloc);
+
+int tee_shm_get_fd(struct tee_shm *shm)
+{
+ u32 req_flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF;
+ int fd;
+
+ if ((shm->flags & req_flags) != req_flags)
+ return -EINVAL;
+
+ fd = dma_buf_fd(shm->dmabuf, O_CLOEXEC);
+ if (fd >= 0)
+ get_dma_buf(shm->dmabuf);
+ return fd;
+}
+EXPORT_SYMBOL_GPL(tee_shm_get_fd);
+
+int tee_shm_put_fd(int fd)
+{
+ return __close_fd(current->files, fd);
+}
+EXPORT_SYMBOL_GPL(tee_shm_put_fd);
+
+
+static void tee_shm_release(struct tee_shm *shm)
+{
+ struct tee_device *teedev = shm->teedev;
+ struct tee_shm_pool_mgr *poolm;
+
+ mutex_lock(&teeshm_mutex);
+ list_del(&shm->list_node);
+ mutex_unlock(&teeshm_mutex);
+
+ if (shm->flags & TEE_SHM_DMA_BUF)
+ poolm = &teedev->pool->dma_buf_mgr;
+ else
+ poolm = &teedev->pool->private_mgr;
+
+ if (shm->flags & __TEE_SHM_SHARED)
+ teedev->desc->ops->shm_unshare(shm);
+ poolm->ops->free(poolm, shm);
+ kfree(shm);
+
+ module_put(teedev->desc->owner);
+}
+
+void tee_shm_free(struct tee_shm *shm)
+{
+
+ /*
+ * dma_buf_put() decreases the dmabuf reference counter and will
+ * call tee_shm_release() when the last reference is gone.
+ *
+ * In the case of anonymous memory we call tee_shm_release directly
+ * instead at it doesn't have a reference counter.
+ */
+ if (shm->flags & TEE_SHM_DMA_BUF)
+ dma_buf_put(shm->dmabuf);
+ else
+ tee_shm_release(shm);
+}
+EXPORT_SYMBOL_GPL(tee_shm_free);
+
+static bool cmp_key_va(struct tee_shm *shm, uintptr_t va)
+{
+ uintptr_t shm_va = (uintptr_t)shm->kaddr;
+
+ return (va >= shm_va) && (va < (shm_va + shm->size));
+}
+
+static bool cmp_key_pa(struct tee_shm *shm, uintptr_t pa)
+{
+ return (pa >= shm->paddr) && (pa < (shm->paddr + shm->size));
+}
+
+static struct tee_shm *tee_shm_find_by_key(struct tee_device *teedev, u32 flags,
+ bool (*cmp)(struct tee_shm *shm, uintptr_t key),
+ uintptr_t key)
+{
+ struct tee_shm *ret = NULL;
+ struct tee_shm *shm;
+
+ mutex_lock(&teeshm_mutex);
+ list_for_each_entry(shm, &teedev->list_shm, list_node) {
+ if (cmp(shm, key)) {
+ ret = shm;
+ break;
+ }
+ }
+ mutex_unlock(&teeshm_mutex);
+
+ return ret;
+}
+
+struct tee_shm *tee_shm_find_by_va(struct tee_device *teedev, u32 flags,
+ void *va)
+{
+ return tee_shm_find_by_key(teedev, flags, cmp_key_va, (uintptr_t)va);
+}
+EXPORT_SYMBOL_GPL(tee_shm_find_by_va);
+
+struct tee_shm *tee_shm_find_by_pa(struct tee_device *teedev, u32 flags,
+ phys_addr_t pa)
+{
+ return tee_shm_find_by_key(teedev, flags, cmp_key_pa, pa);
+}
+EXPORT_SYMBOL_GPL(tee_shm_find_by_pa);
+
+int tee_shm_va2pa(struct tee_shm *shm, void *va, phys_addr_t *pa)
+{
+ /* Check that we're in the range of the shm */
+ if ((char *)va < (char *)shm->kaddr)
+ return -EINVAL;
+ if ((char *)va >= ((char *)shm->kaddr + shm->size))
+ return -EINVAL;
+
+ return tee_shm_get_pa(shm, (u_long)va - (u_long)shm->kaddr, pa);
+}
+EXPORT_SYMBOL_GPL(tee_shm_va2pa);
+
+int tee_shm_pa2va(struct tee_shm *shm, phys_addr_t pa, void **va)
+{
+ /* Check that we're in the range of the shm */
+ if (pa < shm->paddr)
+ return -EINVAL;
+ if (pa >= (shm->paddr + shm->size))
+ return -EINVAL;
+
+ if (va) {
+ void *v = tee_shm_get_va(shm, pa - shm->paddr);
+
+ if (IS_ERR(v))
+ return PTR_ERR(v);
+ *va = v;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tee_shm_pa2va);
+
+void *tee_shm_get_va(struct tee_shm *shm, size_t offs)
+{
+ if (offs >= shm->size)
+ return ERR_PTR(-EINVAL);
+ return (char *)shm->kaddr + offs;
+}
+EXPORT_SYMBOL_GPL(tee_shm_get_va);
+
+int tee_shm_get_pa(struct tee_shm *shm, size_t offs, phys_addr_t *pa)
+{
+ if (offs >= shm->size)
+ return -EINVAL;
+ if (pa)
+ *pa = shm->paddr + offs;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tee_shm_get_pa);
+
+static bool is_shm_dma_buf(struct dma_buf *dmabuf)
+{
+ return dmabuf->ops == &tee_shm_dma_buf_ops;
+}
+
+struct tee_shm *tee_shm_get_from_fd(int fd)
+{
+ struct dma_buf *dmabuf = dma_buf_get(fd);
+
+ if (IS_ERR(dmabuf))
+ return ERR_CAST(dmabuf);
+
+ if (!is_shm_dma_buf(dmabuf)) {
+ dma_buf_put(dmabuf);
+ return ERR_PTR(-EINVAL);
+ }
+ return dmabuf->priv;
+}
+EXPORT_SYMBOL_GPL(tee_shm_get_from_fd);
+
+void tee_shm_put(struct tee_shm *shm)
+{
+ if (shm->flags & TEE_SHM_DMA_BUF)
+ dma_buf_put(shm->dmabuf);
+}
+EXPORT_SYMBOL_GPL(tee_shm_put);
diff --git a/drivers/tee/tee_shm_pool.c b/drivers/tee/tee_shm_pool.c
new file mode 100644
index 0000000..c1d2092
--- /dev/null
+++ b/drivers/tee/tee_shm_pool.c
@@ -0,0 +1,246 @@
+/*
+ * 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.
+ *
+ */
+#include <linux/device.h>
+#include <linux/dma-buf.h>
+#include <linux/slab.h>
+#include <linux/genalloc.h>
+#ifdef CONFIG_CMA
+#include <linux/cma.h>
+#include <linux/dma-contiguous.h>
+#endif
+#include <linux/tee/tee_drv.h>
+#include "tee_private.h"
+
+#define SHM_POOL_NUM_PRIV_PAGES 1
+
+static int pool_op_gen_alloc(struct tee_shm_pool_mgr *poolm,
+ struct tee_shm *shm, size_t size)
+{
+ unsigned long va;
+ struct gen_pool *genpool = poolm->private_data;
+ size_t s = roundup(size, 1 << genpool->min_alloc_order);
+
+ va = gen_pool_alloc(genpool, s);
+ if (!va)
+ return -ENOMEM;
+ shm->kaddr = (void *)va;
+ shm->paddr = gen_pool_virt_to_phys(genpool, va);
+ shm->size = s;
+ return 0;
+}
+
+static void pool_op_gen_free(struct tee_shm_pool_mgr *poolm,
+ struct tee_shm *shm)
+{
+ gen_pool_free(poolm->private_data, (unsigned long)shm->kaddr,
+ shm->size);
+ shm->kaddr = NULL;
+}
+
+static const struct tee_shm_pool_mgr_ops pool_ops_generic = {
+ .alloc = pool_op_gen_alloc,
+ .free = pool_op_gen_free,
+};
+
+#ifdef CONFIG_CMA
+static int pool_op_cma_alloc(struct tee_shm_pool_mgr *poolm,
+ struct tee_shm *shm, size_t size)
+{
+ unsigned long order = get_order(PAGE_SIZE);
+ size_t count;
+ struct page *pages;
+
+ /*
+ * It's not valid to call this function with size = 0, but if size
+ * is 0 we'll get a very large number and the allocation will fail.
+ */
+ count = ((size - 1) >> PAGE_SHIFT) + 1;
+ pages = cma_alloc(poolm->private_data, count, order);
+ if (!pages)
+ return -ENOMEM;
+ shm->kaddr = page_address(pages);
+ shm->pages = pages;
+ shm->paddr = virt_to_phys(shm->kaddr);
+ shm->size = count << PAGE_SHIFT;
+ return 0;
+}
+
+static void pool_op_cma_free(struct tee_shm_pool_mgr *poolm,
+ struct tee_shm *shm)
+{
+ size_t count;
+
+ count = shm->size >> PAGE_SHIFT;
+ cma_release(poolm->private_data, shm->pages, count);
+ shm->kaddr = NULL;
+}
+
+static const struct tee_shm_pool_mgr_ops pool_ops_cma = {
+ .alloc = pool_op_cma_alloc,
+ .free = pool_op_cma_free,
+};
+
+static void pool_cma_destroy(struct tee_shm_pool *pool)
+{
+ gen_pool_destroy(pool->private_mgr.private_data);
+ cma_release(pool->dma_buf_mgr.private_data, pool->private_data,
+ SHM_POOL_NUM_PRIV_PAGES);
+}
+
+struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
+ phys_addr_t *paddr, size_t *size)
+{
+ struct cma *cma = dev_get_cma_area(dev);
+ struct tee_shm_pool *pool;
+ struct page *page = NULL;
+ size_t order = get_order(PAGE_SIZE);
+ struct gen_pool *genpool = NULL;
+ void *va;
+ int ret;
+
+ pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+ if (!pool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ page = cma_alloc(cma, SHM_POOL_NUM_PRIV_PAGES, order);
+ if (!page) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ genpool = gen_pool_create(get_order(8), -1);
+ if (!genpool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ gen_pool_set_algo(genpool, gen_pool_best_fit, NULL);
+
+ va = page_address(page);
+ ret = gen_pool_add_virt(genpool, (u_long)va, virt_to_phys(va),
+ SHM_POOL_NUM_PRIV_PAGES * PAGE_SIZE, -1);
+ if (ret)
+ goto err;
+
+ pool->private_data = page;
+ pool->private_mgr.private_data = genpool;
+ pool->private_mgr.ops = &pool_ops_generic;
+ pool->dma_buf_mgr.private_data = cma;
+ pool->dma_buf_mgr.ops = &pool_ops_cma;
+ pool->destroy = pool_cma_destroy;
+
+ *paddr = cma_get_base(cma);
+ *vaddr = (u_long)phys_to_virt(*paddr);
+ *size = cma_get_size(cma);
+ return pool;
+err:
+ dev_err(dev, "can't allocate memory for CMA shared memory pool\n");
+ if (genpool)
+ gen_pool_destroy(genpool);
+ if (page)
+ cma_release(cma, page, SHM_POOL_NUM_PRIV_PAGES);
+ kfree(pool);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(tee_shm_pool_alloc_cma);
+#endif
+
+static void pool_res_mem_destroy(struct tee_shm_pool *pool)
+{
+ gen_pool_destroy(pool->private_mgr.private_data);
+ gen_pool_destroy(pool->dma_buf_mgr.private_data);
+}
+
+struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
+ u_long vaddr, phys_addr_t paddr, size_t size)
+{
+ size_t page_mask = PAGE_SIZE - 1;
+ size_t priv_size = PAGE_SIZE * SHM_POOL_NUM_PRIV_PAGES;
+ struct tee_shm_pool *pool = NULL;
+ struct gen_pool *genpool = NULL;
+ int ret;
+
+ /*
+ * Start and end must be page aligned
+ */
+ if ((vaddr & page_mask) || (paddr & page_mask) || (size & page_mask)) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ /*
+ * Wouldn't make sense to have less than twice the number of
+ * private pages, in practice the size has to be much larger, but
+ * this is the absolute minimum.
+ */
+ if (size < priv_size * 2) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+ if (!pool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ /*
+ * Create the pool for driver private shared memory
+ */
+ genpool = gen_pool_create(3 /* 8 byte aligned */, -1);
+ if (!genpool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ gen_pool_set_algo(genpool, gen_pool_best_fit, NULL);
+ ret = gen_pool_add_virt(genpool, vaddr, paddr, priv_size, -1);
+ if (ret)
+ goto err;
+ pool->private_mgr.private_data = genpool;
+ pool->private_mgr.ops = &pool_ops_generic;
+
+ /*
+ * Create the pool for dma_buf shared memory
+ */
+ genpool = gen_pool_create(PAGE_SHIFT, -1);
+ gen_pool_set_algo(genpool, gen_pool_best_fit, NULL);
+ if (!genpool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ ret = gen_pool_add_virt(genpool, vaddr + priv_size, paddr + priv_size,
+ size - priv_size, -1);
+ if (ret)
+ goto err;
+ pool->dma_buf_mgr.private_data = genpool;
+ pool->dma_buf_mgr.ops = &pool_ops_generic;
+ pool->destroy = pool_res_mem_destroy;
+ return pool;
+err:
+ dev_err(dev, "can't allocate memory for res_mem shared memory pool\n");
+ if (pool && pool->private_mgr.private_data)
+ gen_pool_destroy(pool->private_mgr.private_data);
+ if (genpool)
+ gen_pool_destroy(genpool);
+ kfree(pool);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(tee_shm_pool_alloc_res_mem);
+
+void tee_shm_pool_free(struct tee_shm_pool *pool)
+{
+ pool->destroy(pool);
+ kfree(pool);
+}
+EXPORT_SYMBOL_GPL(tee_shm_pool_free);
diff --git a/include/linux/tee/tee.h b/include/linux/tee/tee.h
new file mode 100644
index 0000000..f1af46b
--- /dev/null
+++ b/include/linux/tee/tee.h
@@ -0,0 +1,180 @@
+/*
+ * 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
+ */
+
+
+/* Helpers to make the ioctl defines */
+#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)
+#define _TEE_IOW(nr, size) _IOW(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr), size)
+
+/*
+ * Version of the generic TEE subsystem, if it doesn't match what's
+ * returned by TEE_IOC_VERSION this header is not in sync with the kernel.
+ */
+#define TEE_SUBSYS_VERSION 1
+
+
+/* Flags relating to shared memory */
+#define TEE_IOCTL_SHM_MAPPED 0x1 /* memory mapped in normal world */
+#define TEE_IOCTL_SHM_DMA_BUF 0x2 /* dma-buf handle on shared memory */
+
+/**
+ * struct tee_version - TEE versions
+ * @gen_version: [out] Generic TEE driver version
+ * @spec_version: [out] Specific TEE driver version
+ * @uuid: [out] Specific TEE driver uuid, zero if not used
+ *
+ * Identifies the generic TEE driver, and the specific TEE driver.
+ * Used as argument for TEE_IOC_VERSION below.
+ */
+struct tee_ioctl_version {
+ uint32_t gen_version;
+ uint32_t spec_version;
+ uint8_t uuid[16];
+};
+/**
+ * TEE_IOC_VERSION - query version of drivers
+ *
+ * Takes a tee_version struct and returns with the version numbers filled in.
+ */
+#define TEE_IOC_VERSION _TEE_IOR(0, struct tee_ioctl_version)
+
+/**
+ * struct tee_cmd_data - Opaque command argument
+ * @buf_ptr: [in] A __user pointer to a command buffer
+ * @buf_len: [in] 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.
+ * Used as argument for TEE_IOC_CMD below.
+ */
+struct tee_ioctl_cmd_data {
+ uint64_t buf_ptr;
+ uint64_t buf_len;
+};
+/**
+ * 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_ioctl_cmd_data)
+
+/**
+ * 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.
+ * @fd: [out] dma_buf file descriptor of the shared memory
+ *
+ * The flags field should currently be zero as input. Updated by the call
+ * with actual flags as defined by TEE_IOCTL_SHM_* above.
+ * This structure is used as argument for TEE_IOC_SHM_ALLOC below.
+ */
+struct tee_ioctl_shm_alloc_data {
+ uint64_t size;
+ uint32_t flags;
+ int32_t fd;
+};
+/**
+ * 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_ioctl_shm_alloc_data)
+
+/**
+ * struct tee_mem_buf - share user space memory with Secure OS
+ * @ptr: A __user pointer to memory to share
+ * @size: Size of the memory to share
+ * Used in 'struct tee_mem_share_data' below.
+ */
+struct tee_ioctl_mem_buf {
+ uint64_t ptr;
+ uint64_t size;
+};
+
+/**
+ * struct tee_mem_dma_buf - share foreign dma_buf memory
+ * @fd: dma_buf file descriptor
+ * @pad: padding, set to zero by caller
+ * Used in 'struct tee_mem_share_data' below.
+ */
+struct tee_ioctl_mem_dma_buf {
+ int32_t fd;
+ uint32_t pad;
+};
+
+/**
+ * struct tee_mem_share_data - share memory with Secure OS
+ * @buf: [in] share user space memory
+ * @dma_buf: [in] share foreign dma_buf memory
+ * @flags: [in/out] Flags to/from sharing.
+ * @pad: [in/out] Padding, set to zero by caller
+ *
+ * The bits in @flags are defined by TEE_IOCTL_SHM_* above, undefined bits
+ * should be seto to zero as input. If TEE_IOCTL_SHM_DMA_BUF is set in the
+ * flags field use the dma_buf field, else the buf field in the union.
+ *
+ * Used as argument for TEE_IOC_MEM_SHARE and TEE_IOC_MEM_UNSHARE below.
+ */
+struct tee_ioctl_mem_share_data {
+ union {
+ struct tee_ioctl_mem_buf buf;
+ struct tee_ioctl_mem_dma_buf dma_buf;
+ };
+ uint32_t flags;
+ uint32_t pad;
+};
+
+/**
+ * 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_ioctl_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_IOW(4, struct tee_ioctl_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*/
diff --git a/include/linux/tee/tee_drv.h b/include/linux/tee/tee_drv.h
new file mode 100644
index 0000000..8309fb4
--- /dev/null
+++ b/include/linux/tee/tee_drv.h
@@ -0,0 +1,271 @@
+/*
+ * 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_DRV_H
+#define __TEE_DRV_H
+
+#include <linux/types.h>
+#include <linux/list.h>
+#include <linux/miscdevice.h>
+#include <linux/tee/tee.h>
+
+/*
+ * The file describes the API provided by the generic TEE driver to the
+ * specific TEE driver.
+ */
+
+#define TEE_SHM_MAPPED 0x1 /* Memory mapped by the kernel */
+#define TEE_SHM_DMA_BUF 0x2 /* Memory with dma-buf handle */
+#define __TEE_SHM_SHARED 0x4 /* Has been shared with secure world */
+
+#define TEE_UUID_SIZE 16
+
+struct tee_device;
+struct tee_shm;
+struct tee_shm_pool;
+
+/**
+ * struct tee_context - driver specific context on file pointer data
+ * @teedev: pointer to this drivers struct tee_device
+ * @data: driver specific context data, managed by the driver
+ */
+struct tee_context {
+ struct tee_device *teedev;
+ void *data;
+};
+
+/**
+ * struct tee_driver_ops - driver operations vtable
+ * @get_version: returns version of driver
+ * @open: called when the device file is opened
+ * @release: release this open file
+ * @cmd: process a command from user space
+ * @shm_share: share some memory with Secure OS
+ * @shm_unshare: unshare some memory with Secure OS
+ */
+struct tee_driver_ops {
+ void (*get_version)(struct tee_context *ctx, u32 *version, u8 *uuid);
+ int (*open)(struct tee_context *ctx);
+ void (*release)(struct tee_context *ctx);
+ int (*cmd)(struct tee_context *ctx, void __user *buf, size_t len);
+ int (*shm_share)(struct tee_shm *shm);
+ void (*shm_unshare)(struct tee_shm *shm);
+};
+
+/**
+ * struct tee_desc - Describes the TEE driver to the subsystem
+ * @name: name of driver
+ * @ops: driver operations vtable
+ * @owner: module providing the driver
+ * @flags: Extra properties of driver, defined by TEE_DESC_* below
+ */
+#define TEE_DESC_PRIVILEGED 0x1
+struct tee_desc {
+ const char *name;
+ const struct tee_driver_ops *ops;
+ struct module *owner;
+ u32 flags;
+};
+
+
+/**
+ * tee_register() - Register a specific TEE driver
+ * @teedesc: Descriptor for this driver
+ * @dev: Parent device for this driver
+ * @driver_data: Private driver data for this driver
+ *
+ * Once the specific driver has been probed it registers in the generic
+ * driver with this function.
+ *
+ * @returns a pointer to a 'struct tee_device' or an ERR_PTR on failure
+ */
+struct tee_device *tee_register(const struct tee_desc *teedesc,
+ struct device *dev, struct tee_shm_pool *pool,
+ void *driver_data);
+
+/**
+ * tee_unregister() - Unregister a specific TEE driver
+ * @teedev: Driver to unregister
+ */
+void tee_unregister(struct tee_device *teedev);
+
+/**
+ * tee_shm_pool_alloc_cma() - Create a shared memory pool based on device default CMA area
+ * @dev: Device to get default CMA area from
+ * @vaddr: Returned virtual address of start of CMA area
+ * @paddr: Returned physical address of start of CMA area
+ * @size: Returned size of CMA area
+ * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
+ */
+#ifdef CONFIG_CMA
+struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
+ phys_addr_t *paddr, size_t *size);
+#else
+struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
+ phys_addr_t *paddr, size_t *size)
+{
+ return ERR_PTR(-ENOENT);
+}
+#endif
+
+/**
+ * tee_shm_pool_alloc_res_mem() - Create a shared memory pool a reserved memory range
+ * @dev: Device allocating the pool
+ * @vaddr: Virtual address of start of pool
+ * @paddr: Physical address of start of pool
+ * @size: Size in bytes of the pool
+ *
+ * Start of pool will be rounded up to the nearest page, end of pool will
+ * be rounded down to the nearest page.
+ *
+ * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
+ */
+struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
+ u_long vaddr, phys_addr_t paddr, size_t size);
+
+/**
+ * tee_shm_pool_free() - Free a shared memory pool
+ * @pool: The shared memory pool to free
+ *
+ * The must be no remaining shared memory allocated from this pool when
+ * this function is called.
+ */
+void tee_shm_pool_free(struct tee_shm_pool *pool);
+
+/**
+ * tee_get_drvdata() - Return driver_data pointer
+ * @returns the driver_data pointer supplied to tee_register().
+ */
+void *tee_get_drvdata(struct tee_device *teedev);
+
+/**
+ * tee_shm_alloc() - Allocate shared memory
+ * @teedev: Driver that allocates the shared memory
+ * @size: Requested size of shared memory
+ * @flags: Flags setting properties for the requested shared memory.
+ *
+ * Memory allocated as global shared memory is automatically freed when the
+ * TEE file pointer is closed. The @flags field uses the bits defined by
+ * TEE_SHM_* above. TEE_SHM_MAPPED must currently always be set. If
+ * TEE_SHM_DMA_BUF global shared memory will be allocated and associated
+ * with a dma-buf handle, else driver private memory.
+ *
+ * @returns a pointer to 'struct tee_shm'
+ */
+struct tee_shm *tee_shm_alloc(struct tee_device *teedev, size_t size,
+ u32 flags);
+
+/**
+ * tee_shm_free() - Free shared memory
+ * @shm: Handle to shared memory to free
+ */
+void tee_shm_free(struct tee_shm *shm);
+
+/**
+ * tee_shm_find_by_va() - Find a shared memory handle by a virtual address
+ * @teedev: The device that owns the shared memory
+ * @flags: Select which type of shared memory to locate, if
+ * TEE_SHM_DMA_BUF global shared memory else driver private
+ * shared memory.
+ * @va: Virtual address covered by the shared memory
+ * @returns a Handle to shared memory
+ */
+struct tee_shm *tee_shm_find_by_va(struct tee_device *teedev, u32 flags,
+ void *va);
+/**
+ * tee_shm_find_by_pa() - Find a shared memory handle by a physical address
+ * @teedev: The device that owns the shared memory
+ * @flags: Select which type of shared memory to locate, if
+ * TEE_SHM_DMA_BUF global shared memory else driver private
+ * shared memory.
+ * @pa: Physical address covered by the shared memory
+ * @returns a Handle to shared memory
+ */
+struct tee_shm *tee_shm_find_by_pa(struct tee_device *teedev, u32 flags,
+ phys_addr_t pa);
+
+/**
+ * tee_shm_va2pa() - Get physical address of a virtual address
+ * @shm: Shared memory handle
+ * @va: Virtual address to tranlsate
+ * @pa: Returned physical address
+ * @returns 0 on success and < 0 on failure
+ */
+int tee_shm_va2pa(struct tee_shm *shm, void *va, phys_addr_t *pa);
+
+/**
+ * tee_shm_pa2va() - Get virtual address of a physical address
+ * @shm: Shared memory handle
+ * @pa: Physical address to tranlsate
+ * @va: Returned virtual address
+ * @returns 0 on success and < 0 on failure
+ */
+int tee_shm_pa2va(struct tee_shm *shm, phys_addr_t pa, void **va);
+
+/**
+ * tee_shm_get_size() - Get size of a shared memory
+ * @returns the size of the shared memory
+ */
+size_t tee_shm_get_size(struct tee_shm *shm);
+
+/**
+ * tee_shm_get_va() - Get virtual address of a shared memory plus an offset
+ * @shm: Shared memory handle
+ * @offs: Offset from start of this shared memory
+ * @returns virtual address of the shared memory + offs if offs is within
+ * the bounds of this shared memory, else an ERR_PTR
+ */
+void *tee_shm_get_va(struct tee_shm *shm, size_t offs);
+
+/**
+ * tee_shm_get_pa() - Get physical address of a shared memory plus an offset
+ * @shm: Shared memory handle
+ * @offs: Offset from start of this shared memory
+ * @pa: Physical address to return
+ * @returns 0 if offs is within the bounds of this shared memory, else an
+ * error code.
+ */
+int tee_shm_get_pa(struct tee_shm *shm, size_t offs, phys_addr_t *pa);
+
+/**
+ * tee_shm_get_from_fd() - Get a shared memory handle from a file descriptor
+ * @fd: A user space file descriptor
+ *
+ * This function increases the reference counter on the shared memory and
+ * returns a handle.
+ * @returns handle to shared memory
+ */
+struct tee_shm *tee_shm_get_from_fd(int fd);
+
+/**
+ * tee_shm_put() - Decrease reference count on a shared memory handle
+ * @shm: Shared memory handle
+ */
+void tee_shm_put(struct tee_shm *shm);
+
+/**
+ * tee_shm_get_fd() - Increase reference count and return file descriptor
+ * @shm: Shared memory handle
+ * @returns user space file descriptor to shared memory
+ */
+int tee_shm_get_fd(struct tee_shm *shm);
+
+/**
+ * tee_shm_put_fd() - Decrease reference count and close file descriptor
+ * @fd: File descriptor to close
+ * @returns < 0 on failure
+ */
+int tee_shm_put_fd(int fd);
+
+#endif /*__TEE_DRV_H*/
--
1.9.1
^ permalink raw reply related [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-17 7:50 ` Jens Wiklander
0 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-17 7:50 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Arnd Bergmann,
Greg Kroah-Hartman, javier-5MUHepqpBA1BDgjK7y7TUQ
Cc: Herbert Xu, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
valentin.manea-hv44wF8Li93QT0dZR+AlfA,
jean-michel.delorme-qxv4g6HH51o, emmanuel.michel-qxv4g6HH51o,
Jens Wiklander
Initial patch for generic TEE subsystem.
This subsystem provides:
* Registration/un-registration of TEE drivers.
* Shared memory between normal world and secure world.
* Ioctl interface for interaction with user space.
A TEE (Trusted Execution Environment) driver is a driver that interfaces
with a trusted OS running in some secure environment, for example,
TrustZone on ARM cpus, or a separate secure co-processor etc.
To avoid putting unnecessary restrictions on the TEE driver and the
trusted OS the TEE_IOC_CMD passes an opaque buffer to the TEE driver to
facilitate a communication channel between user space and the trusted
OS.
The TEE subsystem can serve a TEE driver for a Global Platform compliant
TEE, but it's not limited to only Global Platform TEEs.
This patch builds on other similar implementations trying to solve
the same problem:
* "optee_linuxdriver" by among others
Jean-michel DELORME<jean-michel.delorme-qxv4g6HH51o@public.gmane.org> and
Emmanuel MICHEL <emmanuel.michel-qxv4g6HH51o@public.gmane.org>
* "Generic TrustZone Driver" by Javier González <javier-5MUHepqpBA2IwRZHo2/mJg@public.gmane.orgm>
Signed-off-by: Jens Wiklander <jens.wiklander-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
Documentation/ioctl/ioctl-number.txt | 1 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/tee/Kconfig | 8 +
drivers/tee/Makefile | 3 +
drivers/tee/tee.c | 253 +++++++++++++++++++++++++++
drivers/tee/tee_private.h | 64 +++++++
drivers/tee/tee_shm.c | 330 +++++++++++++++++++++++++++++++++++
drivers/tee/tee_shm_pool.c | 246 ++++++++++++++++++++++++++
include/linux/tee/tee.h | 180 +++++++++++++++++++
include/linux/tee/tee_drv.h | 271 ++++++++++++++++++++++++++++
11 files changed, 1359 insertions(+)
create mode 100644 drivers/tee/Kconfig
create mode 100644 drivers/tee/Makefile
create mode 100644 drivers/tee/tee.c
create mode 100644 drivers/tee/tee_private.h
create mode 100644 drivers/tee/tee_shm.c
create mode 100644 drivers/tee/tee_shm_pool.c
create mode 100644 include/linux/tee/tee.h
create mode 100644 include/linux/tee/tee_drv.h
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 8136e1f..6e9bd04 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-mn4gwa5WIIQysxA8WJXlww@public.gmane.org>
0xA3 90-9F linux/dtlk.h
+0xA4 00-1F linux/sec-hw/tee.h Generic TEE subsystem
0xAB 00-1F linux/nbd.h
0xAC 00-1F linux/raw.h
0xAD 00 Netfilter device in development:
diff --git a/drivers/Kconfig b/drivers/Kconfig
index c0cc96b..7510f69 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -182,4 +182,6 @@ source "drivers/thunderbolt/Kconfig"
source "drivers/android/Kconfig"
+source "drivers/tee/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 527a6da..0d24e70 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/
obj-$(CONFIG_THUNDERBOLT) += thunderbolt/
obj-$(CONFIG_CORESIGHT) += coresight/
obj-$(CONFIG_ANDROID) += android/
+obj-$(CONFIG_TEE) += tee/
diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
new file mode 100644
index 0000000..64a8cd7
--- /dev/null
+++ b/drivers/tee/Kconfig
@@ -0,0 +1,8 @@
+# Generic Trusted Execution Environment Configuration
+config TEE
+ bool "Trusted Execution Environment support"
+ default n
+ select DMA_SHARED_BUFFER
+ help
+ This implements a generic interface towards a Trusted Execution
+ Environment (TEE).
diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
new file mode 100644
index 0000000..60d2dab
--- /dev/null
+++ b/drivers/tee/Makefile
@@ -0,0 +1,3 @@
+obj-y += tee.o
+obj-y += tee_shm.o
+obj-y += tee_shm_pool.o
diff --git a/drivers/tee/tee.c b/drivers/tee/tee.c
new file mode 100644
index 0000000..23a6e75
--- /dev/null
+++ b/drivers/tee/tee.c
@@ -0,0 +1,253 @@
+/*
+ * 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.
+ *
+ */
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/tee/tee_drv.h>
+#include "tee_private.h"
+
+static int tee_open(struct inode *inode, struct file *filp)
+{
+ int ret;
+ struct tee_device *teedev;
+ struct tee_context *ctx;
+
+ teedev = container_of(filp->private_data, struct tee_device, miscdev);
+ if (!try_module_get(teedev->desc->owner))
+ return -EINVAL;
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->teedev = teedev;
+ filp->private_data = ctx;
+ ret = teedev->desc->ops->open(ctx);
+ if (ret) {
+ kfree(ctx);
+ module_put(teedev->desc->owner);
+ }
+ return ret;
+}
+
+static int tee_release(struct inode *inode, struct file *filp)
+{
+ struct tee_context *ctx = filp->private_data;
+ struct tee_device *teedev = ctx->teedev;
+
+ ctx->teedev->desc->ops->release(ctx);
+ kfree(ctx);
+ module_put(teedev->desc->owner);
+ return 0;
+}
+
+static long tee_ioctl_version(struct tee_context *ctx,
+ struct tee_ioctl_version __user *uvers)
+{
+ struct tee_ioctl_version vers;
+
+ memset(&vers, 0, sizeof(vers));
+ vers.gen_version = TEE_SUBSYS_VERSION;
+ ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
+ vers.uuid);
+
+ return copy_to_user(uvers, &vers, sizeof(vers));
+}
+
+static long tee_ioctl_cmd(struct tee_context *ctx,
+ struct tee_ioctl_cmd_data __user *ucmd)
+{
+ long ret;
+ struct tee_ioctl_cmd_data cmd;
+ void __user *buf_ptr;
+
+ ret = copy_from_user(&cmd, ucmd, sizeof(cmd));
+ if (ret)
+ return ret;
+
+ buf_ptr = (void __user *)(uintptr_t)cmd.buf_ptr;
+ return ctx->teedev->desc->ops->cmd(ctx, buf_ptr, cmd.buf_len);
+}
+
+static long tee_ioctl_shm_alloc(struct tee_context *ctx,
+ struct tee_ioctl_shm_alloc_data __user *udata)
+{
+ long ret;
+ struct tee_ioctl_shm_alloc_data data;
+ struct tee_shm *shm;
+
+ if (copy_from_user(&data, udata, sizeof(data)))
+ return -EFAULT;
+
+ /* Currently no input flags are supported */
+ if (data.flags)
+ return -EINVAL;
+
+ data.fd = -1;
+
+ shm = tee_shm_alloc(ctx->teedev, data.size,
+ TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+ if (IS_ERR(shm))
+ return PTR_ERR(shm);
+
+ ret = ctx->teedev->desc->ops->shm_share(shm);
+ if (ret)
+ goto err;
+
+ data.flags = shm->flags;
+ data.size = shm->size;
+ data.fd = tee_shm_get_fd(shm);
+ if (data.fd < 0) {
+ ret = data.fd;
+ goto err;
+ }
+
+ if (copy_to_user(udata, &data, sizeof(data))) {
+ ret = -EFAULT;
+ goto err;
+ }
+ /*
+ * When user space closes the file descriptor the shared memory
+ * should be freed
+ */
+ tee_shm_put(shm);
+ return 0;
+err:
+ if (data.fd >= 0)
+ tee_shm_put_fd(data.fd);
+ tee_shm_free(shm);
+ return ret;
+}
+
+static long tee_ioctl_mem_share(struct tee_context *ctx,
+ struct tee_ioctl_mem_share_data __user *udata)
+{
+ /* Not supported yet */
+ return -ENOSYS;
+}
+
+static long tee_ioctl_mem_unshare(struct tee_context *ctx,
+ struct tee_ioctl_mem_share_data __user *udata)
+{
+ /* Not supported yet */
+ return -ENOSYS;
+}
+
+static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ struct tee_context *ctx = filp->private_data;
+ void __user *uarg = (void __user *)arg;
+
+ switch (cmd) {
+ case TEE_IOC_VERSION:
+ return tee_ioctl_version(ctx, uarg);
+ case TEE_IOC_CMD:
+ return tee_ioctl_cmd(ctx, uarg);
+ case TEE_IOC_SHM_ALLOC:
+ return tee_ioctl_shm_alloc(ctx, uarg);
+ case TEE_IOC_MEM_SHARE:
+ return tee_ioctl_mem_share(ctx, uarg);
+ case TEE_IOC_MEM_UNSHARE:
+ return tee_ioctl_mem_unshare(ctx, uarg);
+ default:
+ return -EINVAL;
+ }
+}
+
+
+static const struct file_operations tee_fops = {
+ .owner = THIS_MODULE,
+ .open = tee_open,
+ .release = tee_release,
+ .unlocked_ioctl = tee_ioctl
+};
+
+struct tee_device *tee_register(const struct tee_desc *teedesc,
+ struct device *dev, struct tee_shm_pool *pool,
+ void *driver_data)
+{
+ static atomic_t device_no = ATOMIC_INIT(-1);
+ static atomic_t priv_device_no = ATOMIC_INIT(-1);
+ struct tee_device *teedev;
+ void *ret;
+ int rc;
+
+ if (!teedesc || !teedesc->name || !dev || !pool) {
+ ret = ERR_PTR(-EINVAL);
+ goto err;
+ }
+
+ teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
+ if (!teedev) {
+ ret = ERR_PTR(-ENOMEM);
+ goto err;
+ }
+
+ teedev->dev = dev;
+ teedev->desc = teedesc;
+ teedev->pool = pool;
+ teedev->driver_data = driver_data;
+
+ if (teedesc->flags & TEE_DESC_PRIVILEGED)
+ snprintf(teedev->name, sizeof(teedev->name),
+ "teepriv%d", atomic_inc_return(&priv_device_no));
+ else
+ snprintf(teedev->name, sizeof(teedev->name),
+ "tee%d", atomic_inc_return(&device_no));
+
+ teedev->miscdev.parent = dev;
+ teedev->miscdev.minor = MISC_DYNAMIC_MINOR;
+ teedev->miscdev.name = teedev->name;
+ teedev->miscdev.fops = &tee_fops;
+
+ rc = misc_register(&teedev->miscdev);
+ if (rc) {
+ dev_err(dev, "misc_register() failed name=\"%s\"\n",
+ teedev->name);
+ ret = ERR_PTR(rc);
+ goto err;
+ }
+
+ INIT_LIST_HEAD(&teedev->list_shm);
+
+ dev_set_drvdata(teedev->miscdev.this_device, teedev);
+
+ dev_dbg(dev, "register misc device \"%s\" (minor=%d)\n",
+ dev_name(teedev->miscdev.this_device), teedev->miscdev.minor);
+
+ return teedev;
+err:
+ dev_err(dev, "could not register %s driver\n",
+ teedesc->flags & TEE_DESC_PRIVILEGED ? "privileged" : "client");
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_register);
+
+void tee_unregister(struct tee_device *teedev)
+{
+ if (!teedev)
+ return;
+
+ dev_dbg(teedev->dev, "unregister misc device \"%s\" (minor=%d)\n",
+ dev_name(teedev->miscdev.this_device), teedev->miscdev.minor);
+ misc_deregister(&teedev->miscdev);
+}
+EXPORT_SYMBOL_GPL(tee_unregister);
+
+void *tee_get_drvdata(struct tee_device *teedev)
+{
+ return teedev->driver_data;
+}
+EXPORT_SYMBOL_GPL(tee_get_drvdata);
diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h
new file mode 100644
index 0000000..2199634
--- /dev/null
+++ b/drivers/tee/tee_private.h
@@ -0,0 +1,64 @@
+/*
+ * 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_PRIVATE_H
+#define TEE_PRIVATE_H
+
+struct tee_device;
+
+struct tee_shm {
+ struct list_head list_node;
+ struct tee_device *teedev;
+ phys_addr_t paddr;
+ void *kaddr;
+ size_t size;
+ struct dma_buf *dmabuf;
+ struct page *pages;
+ u32 flags;
+};
+
+struct tee_shm_pool_mgr;
+struct tee_shm_pool_mgr_ops {
+ int (*alloc)(struct tee_shm_pool_mgr *poolmgr, struct tee_shm *shm,
+ size_t size);
+ void (*free)(struct tee_shm_pool_mgr *poolmgr, struct tee_shm *shm);
+};
+
+struct tee_shm_pool_mgr {
+ const struct tee_shm_pool_mgr_ops *ops;
+ void *private_data;
+};
+
+struct tee_shm_pool {
+ struct tee_shm_pool_mgr private_mgr;
+ struct tee_shm_pool_mgr dma_buf_mgr;
+ void (*destroy)(struct tee_shm_pool *pool);
+ void *private_data;
+};
+
+#define TEE_MAX_DEV_NAME_LEN 32
+struct tee_device {
+ char name[TEE_MAX_DEV_NAME_LEN];
+ const struct tee_desc *desc;
+ struct device *dev;
+ struct miscdevice miscdev;
+
+ void *driver_data;
+
+ struct list_head list_shm;
+ struct tee_shm_pool *pool;
+};
+
+int tee_shm_init(void);
+
+#endif /*TEE_PRIVATE_H*/
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
new file mode 100644
index 0000000..38359ad
--- /dev/null
+++ b/drivers/tee/tee_shm.c
@@ -0,0 +1,330 @@
+/*
+ * 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.
+ *
+ */
+#include <linux/device.h>
+#include <linux/fdtable.h>
+#include <linux/sched.h>
+#include <linux/dma-buf.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/tee/tee_drv.h>
+#include "tee_private.h"
+
+/* Mutex for all shm objects and lists */
+static DEFINE_MUTEX(teeshm_mutex);
+
+static void tee_shm_release(struct tee_shm *shm);
+
+static struct sg_table *tee_shm_op_map_dma_buf(struct dma_buf_attachment
+ *attach, enum dma_data_direction dir)
+{
+ return NULL;
+}
+
+static void tee_shm_op_unmap_dma_buf(struct dma_buf_attachment *attach,
+ struct sg_table *table, enum dma_data_direction dir)
+{
+}
+
+static void tee_shm_op_release(struct dma_buf *dmabuf)
+{
+ struct tee_shm *shm = dmabuf->priv;
+
+ tee_shm_release(shm);
+}
+
+static void *tee_shm_op_kmap_atomic(struct dma_buf *dmabuf,
+ unsigned long pgnum)
+{
+ return NULL;
+}
+
+static void *tee_shm_op_kmap(struct dma_buf *dmabuf, unsigned long pgnum)
+{
+ return NULL;
+}
+
+static int tee_shm_op_mmap(struct dma_buf *dmabuf,
+ struct vm_area_struct *vma)
+{
+ struct tee_shm *shm = dmabuf->priv;
+ size_t size = vma->vm_end - vma->vm_start;
+
+ return remap_pfn_range(vma, vma->vm_start, shm->paddr >> PAGE_SHIFT,
+ size, vma->vm_page_prot);
+}
+
+static struct dma_buf_ops tee_shm_dma_buf_ops = {
+ .map_dma_buf = tee_shm_op_map_dma_buf,
+ .unmap_dma_buf = tee_shm_op_unmap_dma_buf,
+ .release = tee_shm_op_release,
+ .kmap_atomic = tee_shm_op_kmap_atomic,
+ .kmap = tee_shm_op_kmap,
+ .mmap = tee_shm_op_mmap,
+};
+
+struct tee_shm *tee_shm_alloc(struct tee_device *teedev, size_t size,
+ u32 flags)
+{
+ struct tee_shm_pool_mgr *poolm = NULL;
+ struct tee_shm *shm;
+ void *ret;
+ int rc;
+
+ if (!(flags & TEE_SHM_MAPPED)) {
+ dev_err(teedev->dev, "only mapped allocations supported\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ if ((flags & ~(TEE_SHM_MAPPED|TEE_SHM_DMA_BUF))) {
+ dev_err(teedev->dev, "invalid shm flags 0x%x", flags);
+ return ERR_PTR(-EINVAL);
+ }
+
+ shm = kzalloc(sizeof(struct tee_shm), GFP_KERNEL);
+ if (!shm)
+ return ERR_PTR(-ENOMEM);
+
+ if (!try_module_get(teedev->desc->owner)) {
+ ret = ERR_PTR(-EINVAL);
+ goto err;
+ }
+
+ shm->flags = flags;
+ shm->teedev = teedev;
+ if (flags & TEE_SHM_DMA_BUF)
+ poolm = &teedev->pool->dma_buf_mgr;
+ else
+ poolm = &teedev->pool->private_mgr;
+
+ rc = poolm->ops->alloc(poolm, shm, size);
+ if (rc) {
+ ret = ERR_PTR(rc);
+ goto err;
+ }
+
+ mutex_lock(&teeshm_mutex);
+ list_add_tail(&shm->list_node, &teedev->list_shm);
+ mutex_unlock(&teeshm_mutex);
+
+ if (flags & TEE_SHM_DMA_BUF) {
+ shm->dmabuf = dma_buf_export(shm, &tee_shm_dma_buf_ops,
+ shm->size, O_RDWR, NULL);
+ if (IS_ERR(shm->dmabuf)) {
+ ret = ERR_CAST(shm->dmabuf);
+ goto err;
+ }
+
+ /*
+ * Only call share on dma_buf shm:s, as the driver private
+ * shm:s always originates from the driver itself.
+ */
+ rc = teedev->desc->ops->shm_share(shm);
+ if (rc) {
+ dma_buf_put(shm->dmabuf);
+ return ERR_PTR(rc);
+ }
+ shm->flags |= __TEE_SHM_SHARED;
+ }
+
+ return shm;
+err:
+ if (poolm && shm->kaddr)
+ poolm->ops->free(poolm, shm);
+ kfree(shm);
+ module_put(teedev->desc->owner);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_shm_alloc);
+
+int tee_shm_get_fd(struct tee_shm *shm)
+{
+ u32 req_flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF;
+ int fd;
+
+ if ((shm->flags & req_flags) != req_flags)
+ return -EINVAL;
+
+ fd = dma_buf_fd(shm->dmabuf, O_CLOEXEC);
+ if (fd >= 0)
+ get_dma_buf(shm->dmabuf);
+ return fd;
+}
+EXPORT_SYMBOL_GPL(tee_shm_get_fd);
+
+int tee_shm_put_fd(int fd)
+{
+ return __close_fd(current->files, fd);
+}
+EXPORT_SYMBOL_GPL(tee_shm_put_fd);
+
+
+static void tee_shm_release(struct tee_shm *shm)
+{
+ struct tee_device *teedev = shm->teedev;
+ struct tee_shm_pool_mgr *poolm;
+
+ mutex_lock(&teeshm_mutex);
+ list_del(&shm->list_node);
+ mutex_unlock(&teeshm_mutex);
+
+ if (shm->flags & TEE_SHM_DMA_BUF)
+ poolm = &teedev->pool->dma_buf_mgr;
+ else
+ poolm = &teedev->pool->private_mgr;
+
+ if (shm->flags & __TEE_SHM_SHARED)
+ teedev->desc->ops->shm_unshare(shm);
+ poolm->ops->free(poolm, shm);
+ kfree(shm);
+
+ module_put(teedev->desc->owner);
+}
+
+void tee_shm_free(struct tee_shm *shm)
+{
+
+ /*
+ * dma_buf_put() decreases the dmabuf reference counter and will
+ * call tee_shm_release() when the last reference is gone.
+ *
+ * In the case of anonymous memory we call tee_shm_release directly
+ * instead at it doesn't have a reference counter.
+ */
+ if (shm->flags & TEE_SHM_DMA_BUF)
+ dma_buf_put(shm->dmabuf);
+ else
+ tee_shm_release(shm);
+}
+EXPORT_SYMBOL_GPL(tee_shm_free);
+
+static bool cmp_key_va(struct tee_shm *shm, uintptr_t va)
+{
+ uintptr_t shm_va = (uintptr_t)shm->kaddr;
+
+ return (va >= shm_va) && (va < (shm_va + shm->size));
+}
+
+static bool cmp_key_pa(struct tee_shm *shm, uintptr_t pa)
+{
+ return (pa >= shm->paddr) && (pa < (shm->paddr + shm->size));
+}
+
+static struct tee_shm *tee_shm_find_by_key(struct tee_device *teedev, u32 flags,
+ bool (*cmp)(struct tee_shm *shm, uintptr_t key),
+ uintptr_t key)
+{
+ struct tee_shm *ret = NULL;
+ struct tee_shm *shm;
+
+ mutex_lock(&teeshm_mutex);
+ list_for_each_entry(shm, &teedev->list_shm, list_node) {
+ if (cmp(shm, key)) {
+ ret = shm;
+ break;
+ }
+ }
+ mutex_unlock(&teeshm_mutex);
+
+ return ret;
+}
+
+struct tee_shm *tee_shm_find_by_va(struct tee_device *teedev, u32 flags,
+ void *va)
+{
+ return tee_shm_find_by_key(teedev, flags, cmp_key_va, (uintptr_t)va);
+}
+EXPORT_SYMBOL_GPL(tee_shm_find_by_va);
+
+struct tee_shm *tee_shm_find_by_pa(struct tee_device *teedev, u32 flags,
+ phys_addr_t pa)
+{
+ return tee_shm_find_by_key(teedev, flags, cmp_key_pa, pa);
+}
+EXPORT_SYMBOL_GPL(tee_shm_find_by_pa);
+
+int tee_shm_va2pa(struct tee_shm *shm, void *va, phys_addr_t *pa)
+{
+ /* Check that we're in the range of the shm */
+ if ((char *)va < (char *)shm->kaddr)
+ return -EINVAL;
+ if ((char *)va >= ((char *)shm->kaddr + shm->size))
+ return -EINVAL;
+
+ return tee_shm_get_pa(shm, (u_long)va - (u_long)shm->kaddr, pa);
+}
+EXPORT_SYMBOL_GPL(tee_shm_va2pa);
+
+int tee_shm_pa2va(struct tee_shm *shm, phys_addr_t pa, void **va)
+{
+ /* Check that we're in the range of the shm */
+ if (pa < shm->paddr)
+ return -EINVAL;
+ if (pa >= (shm->paddr + shm->size))
+ return -EINVAL;
+
+ if (va) {
+ void *v = tee_shm_get_va(shm, pa - shm->paddr);
+
+ if (IS_ERR(v))
+ return PTR_ERR(v);
+ *va = v;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tee_shm_pa2va);
+
+void *tee_shm_get_va(struct tee_shm *shm, size_t offs)
+{
+ if (offs >= shm->size)
+ return ERR_PTR(-EINVAL);
+ return (char *)shm->kaddr + offs;
+}
+EXPORT_SYMBOL_GPL(tee_shm_get_va);
+
+int tee_shm_get_pa(struct tee_shm *shm, size_t offs, phys_addr_t *pa)
+{
+ if (offs >= shm->size)
+ return -EINVAL;
+ if (pa)
+ *pa = shm->paddr + offs;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tee_shm_get_pa);
+
+static bool is_shm_dma_buf(struct dma_buf *dmabuf)
+{
+ return dmabuf->ops == &tee_shm_dma_buf_ops;
+}
+
+struct tee_shm *tee_shm_get_from_fd(int fd)
+{
+ struct dma_buf *dmabuf = dma_buf_get(fd);
+
+ if (IS_ERR(dmabuf))
+ return ERR_CAST(dmabuf);
+
+ if (!is_shm_dma_buf(dmabuf)) {
+ dma_buf_put(dmabuf);
+ return ERR_PTR(-EINVAL);
+ }
+ return dmabuf->priv;
+}
+EXPORT_SYMBOL_GPL(tee_shm_get_from_fd);
+
+void tee_shm_put(struct tee_shm *shm)
+{
+ if (shm->flags & TEE_SHM_DMA_BUF)
+ dma_buf_put(shm->dmabuf);
+}
+EXPORT_SYMBOL_GPL(tee_shm_put);
diff --git a/drivers/tee/tee_shm_pool.c b/drivers/tee/tee_shm_pool.c
new file mode 100644
index 0000000..c1d2092
--- /dev/null
+++ b/drivers/tee/tee_shm_pool.c
@@ -0,0 +1,246 @@
+/*
+ * 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.
+ *
+ */
+#include <linux/device.h>
+#include <linux/dma-buf.h>
+#include <linux/slab.h>
+#include <linux/genalloc.h>
+#ifdef CONFIG_CMA
+#include <linux/cma.h>
+#include <linux/dma-contiguous.h>
+#endif
+#include <linux/tee/tee_drv.h>
+#include "tee_private.h"
+
+#define SHM_POOL_NUM_PRIV_PAGES 1
+
+static int pool_op_gen_alloc(struct tee_shm_pool_mgr *poolm,
+ struct tee_shm *shm, size_t size)
+{
+ unsigned long va;
+ struct gen_pool *genpool = poolm->private_data;
+ size_t s = roundup(size, 1 << genpool->min_alloc_order);
+
+ va = gen_pool_alloc(genpool, s);
+ if (!va)
+ return -ENOMEM;
+ shm->kaddr = (void *)va;
+ shm->paddr = gen_pool_virt_to_phys(genpool, va);
+ shm->size = s;
+ return 0;
+}
+
+static void pool_op_gen_free(struct tee_shm_pool_mgr *poolm,
+ struct tee_shm *shm)
+{
+ gen_pool_free(poolm->private_data, (unsigned long)shm->kaddr,
+ shm->size);
+ shm->kaddr = NULL;
+}
+
+static const struct tee_shm_pool_mgr_ops pool_ops_generic = {
+ .alloc = pool_op_gen_alloc,
+ .free = pool_op_gen_free,
+};
+
+#ifdef CONFIG_CMA
+static int pool_op_cma_alloc(struct tee_shm_pool_mgr *poolm,
+ struct tee_shm *shm, size_t size)
+{
+ unsigned long order = get_order(PAGE_SIZE);
+ size_t count;
+ struct page *pages;
+
+ /*
+ * It's not valid to call this function with size = 0, but if size
+ * is 0 we'll get a very large number and the allocation will fail.
+ */
+ count = ((size - 1) >> PAGE_SHIFT) + 1;
+ pages = cma_alloc(poolm->private_data, count, order);
+ if (!pages)
+ return -ENOMEM;
+ shm->kaddr = page_address(pages);
+ shm->pages = pages;
+ shm->paddr = virt_to_phys(shm->kaddr);
+ shm->size = count << PAGE_SHIFT;
+ return 0;
+}
+
+static void pool_op_cma_free(struct tee_shm_pool_mgr *poolm,
+ struct tee_shm *shm)
+{
+ size_t count;
+
+ count = shm->size >> PAGE_SHIFT;
+ cma_release(poolm->private_data, shm->pages, count);
+ shm->kaddr = NULL;
+}
+
+static const struct tee_shm_pool_mgr_ops pool_ops_cma = {
+ .alloc = pool_op_cma_alloc,
+ .free = pool_op_cma_free,
+};
+
+static void pool_cma_destroy(struct tee_shm_pool *pool)
+{
+ gen_pool_destroy(pool->private_mgr.private_data);
+ cma_release(pool->dma_buf_mgr.private_data, pool->private_data,
+ SHM_POOL_NUM_PRIV_PAGES);
+}
+
+struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
+ phys_addr_t *paddr, size_t *size)
+{
+ struct cma *cma = dev_get_cma_area(dev);
+ struct tee_shm_pool *pool;
+ struct page *page = NULL;
+ size_t order = get_order(PAGE_SIZE);
+ struct gen_pool *genpool = NULL;
+ void *va;
+ int ret;
+
+ pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+ if (!pool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ page = cma_alloc(cma, SHM_POOL_NUM_PRIV_PAGES, order);
+ if (!page) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ genpool = gen_pool_create(get_order(8), -1);
+ if (!genpool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ gen_pool_set_algo(genpool, gen_pool_best_fit, NULL);
+
+ va = page_address(page);
+ ret = gen_pool_add_virt(genpool, (u_long)va, virt_to_phys(va),
+ SHM_POOL_NUM_PRIV_PAGES * PAGE_SIZE, -1);
+ if (ret)
+ goto err;
+
+ pool->private_data = page;
+ pool->private_mgr.private_data = genpool;
+ pool->private_mgr.ops = &pool_ops_generic;
+ pool->dma_buf_mgr.private_data = cma;
+ pool->dma_buf_mgr.ops = &pool_ops_cma;
+ pool->destroy = pool_cma_destroy;
+
+ *paddr = cma_get_base(cma);
+ *vaddr = (u_long)phys_to_virt(*paddr);
+ *size = cma_get_size(cma);
+ return pool;
+err:
+ dev_err(dev, "can't allocate memory for CMA shared memory pool\n");
+ if (genpool)
+ gen_pool_destroy(genpool);
+ if (page)
+ cma_release(cma, page, SHM_POOL_NUM_PRIV_PAGES);
+ kfree(pool);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(tee_shm_pool_alloc_cma);
+#endif
+
+static void pool_res_mem_destroy(struct tee_shm_pool *pool)
+{
+ gen_pool_destroy(pool->private_mgr.private_data);
+ gen_pool_destroy(pool->dma_buf_mgr.private_data);
+}
+
+struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
+ u_long vaddr, phys_addr_t paddr, size_t size)
+{
+ size_t page_mask = PAGE_SIZE - 1;
+ size_t priv_size = PAGE_SIZE * SHM_POOL_NUM_PRIV_PAGES;
+ struct tee_shm_pool *pool = NULL;
+ struct gen_pool *genpool = NULL;
+ int ret;
+
+ /*
+ * Start and end must be page aligned
+ */
+ if ((vaddr & page_mask) || (paddr & page_mask) || (size & page_mask)) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ /*
+ * Wouldn't make sense to have less than twice the number of
+ * private pages, in practice the size has to be much larger, but
+ * this is the absolute minimum.
+ */
+ if (size < priv_size * 2) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+ if (!pool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ /*
+ * Create the pool for driver private shared memory
+ */
+ genpool = gen_pool_create(3 /* 8 byte aligned */, -1);
+ if (!genpool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ gen_pool_set_algo(genpool, gen_pool_best_fit, NULL);
+ ret = gen_pool_add_virt(genpool, vaddr, paddr, priv_size, -1);
+ if (ret)
+ goto err;
+ pool->private_mgr.private_data = genpool;
+ pool->private_mgr.ops = &pool_ops_generic;
+
+ /*
+ * Create the pool for dma_buf shared memory
+ */
+ genpool = gen_pool_create(PAGE_SHIFT, -1);
+ gen_pool_set_algo(genpool, gen_pool_best_fit, NULL);
+ if (!genpool) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ ret = gen_pool_add_virt(genpool, vaddr + priv_size, paddr + priv_size,
+ size - priv_size, -1);
+ if (ret)
+ goto err;
+ pool->dma_buf_mgr.private_data = genpool;
+ pool->dma_buf_mgr.ops = &pool_ops_generic;
+ pool->destroy = pool_res_mem_destroy;
+ return pool;
+err:
+ dev_err(dev, "can't allocate memory for res_mem shared memory pool\n");
+ if (pool && pool->private_mgr.private_data)
+ gen_pool_destroy(pool->private_mgr.private_data);
+ if (genpool)
+ gen_pool_destroy(genpool);
+ kfree(pool);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(tee_shm_pool_alloc_res_mem);
+
+void tee_shm_pool_free(struct tee_shm_pool *pool)
+{
+ pool->destroy(pool);
+ kfree(pool);
+}
+EXPORT_SYMBOL_GPL(tee_shm_pool_free);
diff --git a/include/linux/tee/tee.h b/include/linux/tee/tee.h
new file mode 100644
index 0000000..f1af46b
--- /dev/null
+++ b/include/linux/tee/tee.h
@@ -0,0 +1,180 @@
+/*
+ * 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
+ */
+
+
+/* Helpers to make the ioctl defines */
+#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)
+#define _TEE_IOW(nr, size) _IOW(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr), size)
+
+/*
+ * Version of the generic TEE subsystem, if it doesn't match what's
+ * returned by TEE_IOC_VERSION this header is not in sync with the kernel.
+ */
+#define TEE_SUBSYS_VERSION 1
+
+
+/* Flags relating to shared memory */
+#define TEE_IOCTL_SHM_MAPPED 0x1 /* memory mapped in normal world */
+#define TEE_IOCTL_SHM_DMA_BUF 0x2 /* dma-buf handle on shared memory */
+
+/**
+ * struct tee_version - TEE versions
+ * @gen_version: [out] Generic TEE driver version
+ * @spec_version: [out] Specific TEE driver version
+ * @uuid: [out] Specific TEE driver uuid, zero if not used
+ *
+ * Identifies the generic TEE driver, and the specific TEE driver.
+ * Used as argument for TEE_IOC_VERSION below.
+ */
+struct tee_ioctl_version {
+ uint32_t gen_version;
+ uint32_t spec_version;
+ uint8_t uuid[16];
+};
+/**
+ * TEE_IOC_VERSION - query version of drivers
+ *
+ * Takes a tee_version struct and returns with the version numbers filled in.
+ */
+#define TEE_IOC_VERSION _TEE_IOR(0, struct tee_ioctl_version)
+
+/**
+ * struct tee_cmd_data - Opaque command argument
+ * @buf_ptr: [in] A __user pointer to a command buffer
+ * @buf_len: [in] 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.
+ * Used as argument for TEE_IOC_CMD below.
+ */
+struct tee_ioctl_cmd_data {
+ uint64_t buf_ptr;
+ uint64_t buf_len;
+};
+/**
+ * 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_ioctl_cmd_data)
+
+/**
+ * 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.
+ * @fd: [out] dma_buf file descriptor of the shared memory
+ *
+ * The flags field should currently be zero as input. Updated by the call
+ * with actual flags as defined by TEE_IOCTL_SHM_* above.
+ * This structure is used as argument for TEE_IOC_SHM_ALLOC below.
+ */
+struct tee_ioctl_shm_alloc_data {
+ uint64_t size;
+ uint32_t flags;
+ int32_t fd;
+};
+/**
+ * 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_ioctl_shm_alloc_data)
+
+/**
+ * struct tee_mem_buf - share user space memory with Secure OS
+ * @ptr: A __user pointer to memory to share
+ * @size: Size of the memory to share
+ * Used in 'struct tee_mem_share_data' below.
+ */
+struct tee_ioctl_mem_buf {
+ uint64_t ptr;
+ uint64_t size;
+};
+
+/**
+ * struct tee_mem_dma_buf - share foreign dma_buf memory
+ * @fd: dma_buf file descriptor
+ * @pad: padding, set to zero by caller
+ * Used in 'struct tee_mem_share_data' below.
+ */
+struct tee_ioctl_mem_dma_buf {
+ int32_t fd;
+ uint32_t pad;
+};
+
+/**
+ * struct tee_mem_share_data - share memory with Secure OS
+ * @buf: [in] share user space memory
+ * @dma_buf: [in] share foreign dma_buf memory
+ * @flags: [in/out] Flags to/from sharing.
+ * @pad: [in/out] Padding, set to zero by caller
+ *
+ * The bits in @flags are defined by TEE_IOCTL_SHM_* above, undefined bits
+ * should be seto to zero as input. If TEE_IOCTL_SHM_DMA_BUF is set in the
+ * flags field use the dma_buf field, else the buf field in the union.
+ *
+ * Used as argument for TEE_IOC_MEM_SHARE and TEE_IOC_MEM_UNSHARE below.
+ */
+struct tee_ioctl_mem_share_data {
+ union {
+ struct tee_ioctl_mem_buf buf;
+ struct tee_ioctl_mem_dma_buf dma_buf;
+ };
+ uint32_t flags;
+ uint32_t pad;
+};
+
+/**
+ * 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_ioctl_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_IOW(4, struct tee_ioctl_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*/
diff --git a/include/linux/tee/tee_drv.h b/include/linux/tee/tee_drv.h
new file mode 100644
index 0000000..8309fb4
--- /dev/null
+++ b/include/linux/tee/tee_drv.h
@@ -0,0 +1,271 @@
+/*
+ * 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_DRV_H
+#define __TEE_DRV_H
+
+#include <linux/types.h>
+#include <linux/list.h>
+#include <linux/miscdevice.h>
+#include <linux/tee/tee.h>
+
+/*
+ * The file describes the API provided by the generic TEE driver to the
+ * specific TEE driver.
+ */
+
+#define TEE_SHM_MAPPED 0x1 /* Memory mapped by the kernel */
+#define TEE_SHM_DMA_BUF 0x2 /* Memory with dma-buf handle */
+#define __TEE_SHM_SHARED 0x4 /* Has been shared with secure world */
+
+#define TEE_UUID_SIZE 16
+
+struct tee_device;
+struct tee_shm;
+struct tee_shm_pool;
+
+/**
+ * struct tee_context - driver specific context on file pointer data
+ * @teedev: pointer to this drivers struct tee_device
+ * @data: driver specific context data, managed by the driver
+ */
+struct tee_context {
+ struct tee_device *teedev;
+ void *data;
+};
+
+/**
+ * struct tee_driver_ops - driver operations vtable
+ * @get_version: returns version of driver
+ * @open: called when the device file is opened
+ * @release: release this open file
+ * @cmd: process a command from user space
+ * @shm_share: share some memory with Secure OS
+ * @shm_unshare: unshare some memory with Secure OS
+ */
+struct tee_driver_ops {
+ void (*get_version)(struct tee_context *ctx, u32 *version, u8 *uuid);
+ int (*open)(struct tee_context *ctx);
+ void (*release)(struct tee_context *ctx);
+ int (*cmd)(struct tee_context *ctx, void __user *buf, size_t len);
+ int (*shm_share)(struct tee_shm *shm);
+ void (*shm_unshare)(struct tee_shm *shm);
+};
+
+/**
+ * struct tee_desc - Describes the TEE driver to the subsystem
+ * @name: name of driver
+ * @ops: driver operations vtable
+ * @owner: module providing the driver
+ * @flags: Extra properties of driver, defined by TEE_DESC_* below
+ */
+#define TEE_DESC_PRIVILEGED 0x1
+struct tee_desc {
+ const char *name;
+ const struct tee_driver_ops *ops;
+ struct module *owner;
+ u32 flags;
+};
+
+
+/**
+ * tee_register() - Register a specific TEE driver
+ * @teedesc: Descriptor for this driver
+ * @dev: Parent device for this driver
+ * @driver_data: Private driver data for this driver
+ *
+ * Once the specific driver has been probed it registers in the generic
+ * driver with this function.
+ *
+ * @returns a pointer to a 'struct tee_device' or an ERR_PTR on failure
+ */
+struct tee_device *tee_register(const struct tee_desc *teedesc,
+ struct device *dev, struct tee_shm_pool *pool,
+ void *driver_data);
+
+/**
+ * tee_unregister() - Unregister a specific TEE driver
+ * @teedev: Driver to unregister
+ */
+void tee_unregister(struct tee_device *teedev);
+
+/**
+ * tee_shm_pool_alloc_cma() - Create a shared memory pool based on device default CMA area
+ * @dev: Device to get default CMA area from
+ * @vaddr: Returned virtual address of start of CMA area
+ * @paddr: Returned physical address of start of CMA area
+ * @size: Returned size of CMA area
+ * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
+ */
+#ifdef CONFIG_CMA
+struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
+ phys_addr_t *paddr, size_t *size);
+#else
+struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
+ phys_addr_t *paddr, size_t *size)
+{
+ return ERR_PTR(-ENOENT);
+}
+#endif
+
+/**
+ * tee_shm_pool_alloc_res_mem() - Create a shared memory pool a reserved memory range
+ * @dev: Device allocating the pool
+ * @vaddr: Virtual address of start of pool
+ * @paddr: Physical address of start of pool
+ * @size: Size in bytes of the pool
+ *
+ * Start of pool will be rounded up to the nearest page, end of pool will
+ * be rounded down to the nearest page.
+ *
+ * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
+ */
+struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
+ u_long vaddr, phys_addr_t paddr, size_t size);
+
+/**
+ * tee_shm_pool_free() - Free a shared memory pool
+ * @pool: The shared memory pool to free
+ *
+ * The must be no remaining shared memory allocated from this pool when
+ * this function is called.
+ */
+void tee_shm_pool_free(struct tee_shm_pool *pool);
+
+/**
+ * tee_get_drvdata() - Return driver_data pointer
+ * @returns the driver_data pointer supplied to tee_register().
+ */
+void *tee_get_drvdata(struct tee_device *teedev);
+
+/**
+ * tee_shm_alloc() - Allocate shared memory
+ * @teedev: Driver that allocates the shared memory
+ * @size: Requested size of shared memory
+ * @flags: Flags setting properties for the requested shared memory.
+ *
+ * Memory allocated as global shared memory is automatically freed when the
+ * TEE file pointer is closed. The @flags field uses the bits defined by
+ * TEE_SHM_* above. TEE_SHM_MAPPED must currently always be set. If
+ * TEE_SHM_DMA_BUF global shared memory will be allocated and associated
+ * with a dma-buf handle, else driver private memory.
+ *
+ * @returns a pointer to 'struct tee_shm'
+ */
+struct tee_shm *tee_shm_alloc(struct tee_device *teedev, size_t size,
+ u32 flags);
+
+/**
+ * tee_shm_free() - Free shared memory
+ * @shm: Handle to shared memory to free
+ */
+void tee_shm_free(struct tee_shm *shm);
+
+/**
+ * tee_shm_find_by_va() - Find a shared memory handle by a virtual address
+ * @teedev: The device that owns the shared memory
+ * @flags: Select which type of shared memory to locate, if
+ * TEE_SHM_DMA_BUF global shared memory else driver private
+ * shared memory.
+ * @va: Virtual address covered by the shared memory
+ * @returns a Handle to shared memory
+ */
+struct tee_shm *tee_shm_find_by_va(struct tee_device *teedev, u32 flags,
+ void *va);
+/**
+ * tee_shm_find_by_pa() - Find a shared memory handle by a physical address
+ * @teedev: The device that owns the shared memory
+ * @flags: Select which type of shared memory to locate, if
+ * TEE_SHM_DMA_BUF global shared memory else driver private
+ * shared memory.
+ * @pa: Physical address covered by the shared memory
+ * @returns a Handle to shared memory
+ */
+struct tee_shm *tee_shm_find_by_pa(struct tee_device *teedev, u32 flags,
+ phys_addr_t pa);
+
+/**
+ * tee_shm_va2pa() - Get physical address of a virtual address
+ * @shm: Shared memory handle
+ * @va: Virtual address to tranlsate
+ * @pa: Returned physical address
+ * @returns 0 on success and < 0 on failure
+ */
+int tee_shm_va2pa(struct tee_shm *shm, void *va, phys_addr_t *pa);
+
+/**
+ * tee_shm_pa2va() - Get virtual address of a physical address
+ * @shm: Shared memory handle
+ * @pa: Physical address to tranlsate
+ * @va: Returned virtual address
+ * @returns 0 on success and < 0 on failure
+ */
+int tee_shm_pa2va(struct tee_shm *shm, phys_addr_t pa, void **va);
+
+/**
+ * tee_shm_get_size() - Get size of a shared memory
+ * @returns the size of the shared memory
+ */
+size_t tee_shm_get_size(struct tee_shm *shm);
+
+/**
+ * tee_shm_get_va() - Get virtual address of a shared memory plus an offset
+ * @shm: Shared memory handle
+ * @offs: Offset from start of this shared memory
+ * @returns virtual address of the shared memory + offs if offs is within
+ * the bounds of this shared memory, else an ERR_PTR
+ */
+void *tee_shm_get_va(struct tee_shm *shm, size_t offs);
+
+/**
+ * tee_shm_get_pa() - Get physical address of a shared memory plus an offset
+ * @shm: Shared memory handle
+ * @offs: Offset from start of this shared memory
+ * @pa: Physical address to return
+ * @returns 0 if offs is within the bounds of this shared memory, else an
+ * error code.
+ */
+int tee_shm_get_pa(struct tee_shm *shm, size_t offs, phys_addr_t *pa);
+
+/**
+ * tee_shm_get_from_fd() - Get a shared memory handle from a file descriptor
+ * @fd: A user space file descriptor
+ *
+ * This function increases the reference counter on the shared memory and
+ * returns a handle.
+ * @returns handle to shared memory
+ */
+struct tee_shm *tee_shm_get_from_fd(int fd);
+
+/**
+ * tee_shm_put() - Decrease reference count on a shared memory handle
+ * @shm: Shared memory handle
+ */
+void tee_shm_put(struct tee_shm *shm);
+
+/**
+ * tee_shm_get_fd() - Increase reference count and return file descriptor
+ * @shm: Shared memory handle
+ * @returns user space file descriptor to shared memory
+ */
+int tee_shm_get_fd(struct tee_shm *shm);
+
+/**
+ * tee_shm_put_fd() - Decrease reference count and close file descriptor
+ * @fd: File descriptor to close
+ * @returns < 0 on failure
+ */
+int tee_shm_put_fd(int fd);
+
+#endif /*__TEE_DRV_H*/
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-17 7:50 ` Jens Wiklander
@ 2015-04-17 16:30 ` Jason Gunthorpe
-1 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-17 16:30 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, devicetree, Arnd Bergmann,
Greg Kroah-Hartman, javier, valentin.manea, emmanuel.michel,
Herbert Xu, jean-michel.delorme, tpmdd-devel
On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
[..]
> + rc = misc_register(&teedev->miscdev);
[..]
> +void tee_unregister(struct tee_device *teedev)
> +{
[..]
> + misc_deregister(&teedev->miscdev);
> +}
[..]
>+static int optee_remove(struct platform_device *pdev)
>+{
>+ tee_unregister(optee->teedev);
Isn't that a potential use after free? AFAIK misc_deregister does not
guarentee the miscdev will no longer be accessed after it returns, and
the devm will free it after optee_remove returns.
Memory backing a stuct device needs to be freed via the release
function.
We have been going through this for a while with TPM - it seems like
using misc devices dynamically is not a good idea. Manage your own
struct device directly..
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-17 16:30 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-17 16:30 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
[..]
> + rc = misc_register(&teedev->miscdev);
[..]
> +void tee_unregister(struct tee_device *teedev)
> +{
[..]
> + misc_deregister(&teedev->miscdev);
> +}
[..]
>+static int optee_remove(struct platform_device *pdev)
>+{
>+ tee_unregister(optee->teedev);
Isn't that a potential use after free? AFAIK misc_deregister does not
guarentee the miscdev will no longer be accessed after it returns, and
the devm will free it after optee_remove returns.
Memory backing a stuct device needs to be freed via the release
function.
We have been going through this for a while with TPM - it seems like
using misc devices dynamically is not a good idea. Manage your own
struct device directly..
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 9:01 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 9:01 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> [..]
> > + rc = misc_register(&teedev->miscdev);
> [..]
> > +void tee_unregister(struct tee_device *teedev)
> > +{
> [..]
> > + misc_deregister(&teedev->miscdev);
> > +}
> [..]
> >+static int optee_remove(struct platform_device *pdev)
> >+{
> >+ tee_unregister(optee->teedev);
>
> Isn't that a potential use after free? AFAIK misc_deregister does not
> guarentee the miscdev will no longer be accessed after it returns, and
> the devm will free it after optee_remove returns.
>
> Memory backing a stuct device needs to be freed via the release
> function.
Out of interest, which struct device are you talking about here?
struct tee_device contains two things - a struct device _pointer_ to the
device passed into the registration function, and a miscdev.
A miscdev contains two struct device _pointers_ - a pointer to the parent
device, and a pointer to the char class device. As both of these are
pointers, freeing struct tee_device does not free the memory underlying
any device structure.
What does need to be taken care of is that unbinding the parent device
may cause an already-open user of the userspace interface to dereference
the memory which was freed. Tying this to the lifetime of a struct
device doesn't seem right.
I would suggest adding a kref to struct tee_device and use that to manage
the lifetime of that structure - incrementing the refcount on fops->open
and dropping it at fops->release time, so that the struct is automatically
freed when the last user closes the miscdev after the device has been
unbound. You should probably also have a flag to indicate that the device
is no longer present too to prevent further userspace IO.
It would be nice if miscdev provided help with this...
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 9:01 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 9:01 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> [..]
> > + rc = misc_register(&teedev->miscdev);
> [..]
> > +void tee_unregister(struct tee_device *teedev)
> > +{
> [..]
> > + misc_deregister(&teedev->miscdev);
> > +}
> [..]
> >+static int optee_remove(struct platform_device *pdev)
> >+{
> >+ tee_unregister(optee->teedev);
>
> Isn't that a potential use after free? AFAIK misc_deregister does not
> guarentee the miscdev will no longer be accessed after it returns, and
> the devm will free it after optee_remove returns.
>
> Memory backing a stuct device needs to be freed via the release
> function.
Out of interest, which struct device are you talking about here?
struct tee_device contains two things - a struct device _pointer_ to the
device passed into the registration function, and a miscdev.
A miscdev contains two struct device _pointers_ - a pointer to the parent
device, and a pointer to the char class device. As both of these are
pointers, freeing struct tee_device does not free the memory underlying
any device structure.
What does need to be taken care of is that unbinding the parent device
may cause an already-open user of the userspace interface to dereference
the memory which was freed. Tying this to the lifetime of a struct
device doesn't seem right.
I would suggest adding a kref to struct tee_device and use that to manage
the lifetime of that structure - incrementing the refcount on fops->open
and dropping it at fops->release time, so that the struct is automatically
freed when the last user closes the miscdev after the device has been
unbound. You should probably also have a flag to indicate that the device
is no longer present too to prevent further userspace IO.
It would be nice if miscdev provided help with this...
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 9:01 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 9:01 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jens Wiklander, valentin.manea-hv44wF8Li93QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, javier-5MUHepqpBA1BDgjK7y7TUQ,
emmanuel.michel-qxv4g6HH51o, Herbert Xu, Arnd Bergmann,
Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jean-michel.delorme-qxv4g6HH51o,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> [..]
> > + rc = misc_register(&teedev->miscdev);
> [..]
> > +void tee_unregister(struct tee_device *teedev)
> > +{
> [..]
> > + misc_deregister(&teedev->miscdev);
> > +}
> [..]
> >+static int optee_remove(struct platform_device *pdev)
> >+{
> >+ tee_unregister(optee->teedev);
>
> Isn't that a potential use after free? AFAIK misc_deregister does not
> guarentee the miscdev will no longer be accessed after it returns, and
> the devm will free it after optee_remove returns.
>
> Memory backing a stuct device needs to be freed via the release
> function.
Out of interest, which struct device are you talking about here?
struct tee_device contains two things - a struct device _pointer_ to the
device passed into the registration function, and a miscdev.
A miscdev contains two struct device _pointers_ - a pointer to the parent
device, and a pointer to the char class device. As both of these are
pointers, freeing struct tee_device does not free the memory underlying
any device structure.
What does need to be taken care of is that unbinding the parent device
may cause an already-open user of the userspace interface to dereference
the memory which was freed. Tying this to the lifetime of a struct
device doesn't seem right.
I would suggest adding a kref to struct tee_device and use that to manage
the lifetime of that structure - incrementing the refcount on fops->open
and dropping it at fops->release time, so that the struct is automatically
freed when the last user closes the miscdev after the device has been
unbound. You should probably also have a flag to indicate that the device
is no longer present too to prevent further userspace IO.
It would be nice if miscdev provided help with this...
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 17:29 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-18 17:29 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > [..]
> > > + rc = misc_register(&teedev->miscdev);
> > [..]
> > > +void tee_unregister(struct tee_device *teedev)
> > > +{
> > [..]
> > > + misc_deregister(&teedev->miscdev);
> > > +}
> > [..]
> > >+static int optee_remove(struct platform_device *pdev)
> > >+{
> > >+ tee_unregister(optee->teedev);
> >
> > Isn't that a potential use after free? AFAIK misc_deregister does not
> > guarentee the miscdev will no longer be accessed after it returns, and
> > the devm will free it after optee_remove returns.
> >
> > Memory backing a stuct device needs to be freed via the release
> > function.
>
> Out of interest, which struct device are you talking about here?
Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
refer to the entire thing, struct tee_device, struct misc_device, the
driver allocations, etc.
So, the first issue is the use-after-free via ioctl() touching struct
tee_device that you described.
But then we trundle down to:
+ ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
+ vers.uuid);
If we kref teedev so it is valid then calling a driver call back after
(or during) it's remove function is very likely to blow up.
Also, in TPM we discovered that adding a sysfs file was very ugly
(impossible?) because without the misc_mtx protection that open has,
getting a locked tee_device in the sysfs callback is difficult.
With TPM, we ended up trying lots of options for fixing struct
misc_device in the tpm core, which is handling multiple sub drivers,
and basically gave up. Gave each struct tpm_device an embedded struct
device like Greg suggested here. Then the tpm core is working with the
APIs, not struggling against them.
But this is not a user-space invisible change, so better to do it right
from day 1 ..
We followed rtc as an example of how to create a mid-layer that
exports it's own register function, with char dev and sysfs
components. It seems properly implemented, and has elegant solutions
to these problems (like ops):
- Don't mess with modules, use 'ops' and set 'ops' to null when the
driver removes. The driver core will keep the driver module around
for you bettwen the probe/remove calls. Setting ops = NULL ensures driver
module code cannot be called after remove.
- Use locking for 'ops' to serialize driver callbacks with driver removal
- Embed a struct device/etc in the struct tee_device and use the release
function to deallocate struct tee_device. All callbacks from the
driver/char/sysfs core can now use container_of on something that
is already holds the right kref.
- Consider an alloc/register pattern as we use now in TPM. This has proven
smart for TPM as it allows:
alloc tee_device + init struct device, etc
driver setup
core library helper calls for setup/etc
driver register + char dev publish
It appeared to me this driver was copying TPM's old architecture,
which is very much known to be broken.
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 17:29 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-18 17:29 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > [..]
> > > + rc = misc_register(&teedev->miscdev);
> > [..]
> > > +void tee_unregister(struct tee_device *teedev)
> > > +{
> > [..]
> > > + misc_deregister(&teedev->miscdev);
> > > +}
> > [..]
> > >+static int optee_remove(struct platform_device *pdev)
> > >+{
> > >+ tee_unregister(optee->teedev);
> >
> > Isn't that a potential use after free? AFAIK misc_deregister does not
> > guarentee the miscdev will no longer be accessed after it returns, and
> > the devm will free it after optee_remove returns.
> >
> > Memory backing a stuct device needs to be freed via the release
> > function.
>
> Out of interest, which struct device are you talking about here?
Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
refer to the entire thing, struct tee_device, struct misc_device, the
driver allocations, etc.
So, the first issue is the use-after-free via ioctl() touching struct
tee_device that you described.
But then we trundle down to:
+ ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
+ vers.uuid);
If we kref teedev so it is valid then calling a driver call back after
(or during) it's remove function is very likely to blow up.
Also, in TPM we discovered that adding a sysfs file was very ugly
(impossible?) because without the misc_mtx protection that open has,
getting a locked tee_device in the sysfs callback is difficult.
With TPM, we ended up trying lots of options for fixing struct
misc_device in the tpm core, which is handling multiple sub drivers,
and basically gave up. Gave each struct tpm_device an embedded struct
device like Greg suggested here. Then the tpm core is working with the
APIs, not struggling against them.
But this is not a user-space invisible change, so better to do it right
from day 1 ..
We followed rtc as an example of how to create a mid-layer that
exports it's own register function, with char dev and sysfs
components. It seems properly implemented, and has elegant solutions
to these problems (like ops):
- Don't mess with modules, use 'ops' and set 'ops' to null when the
driver removes. The driver core will keep the driver module around
for you bettwen the probe/remove calls. Setting ops = NULL ensures driver
module code cannot be called after remove.
- Use locking for 'ops' to serialize driver callbacks with driver removal
- Embed a struct device/etc in the struct tee_device and use the release
function to deallocate struct tee_device. All callbacks from the
driver/char/sysfs core can now use container_of on something that
is already holds the right kref.
- Consider an alloc/register pattern as we use now in TPM. This has proven
smart for TPM as it allows:
alloc tee_device + init struct device, etc
driver setup
core library helper calls for setup/etc
driver register + char dev publish
It appeared to me this driver was copying TPM's old architecture,
which is very much known to be broken.
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 17:29 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-18 17:29 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jens Wiklander, valentin.manea-hv44wF8Li93QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, javier-5MUHepqpBA1BDgjK7y7TUQ,
emmanuel.michel-qxv4g6HH51o, Herbert Xu, Arnd Bergmann,
Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jean-michel.delorme-qxv4g6HH51o,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > [..]
> > > + rc = misc_register(&teedev->miscdev);
> > [..]
> > > +void tee_unregister(struct tee_device *teedev)
> > > +{
> > [..]
> > > + misc_deregister(&teedev->miscdev);
> > > +}
> > [..]
> > >+static int optee_remove(struct platform_device *pdev)
> > >+{
> > >+ tee_unregister(optee->teedev);
> >
> > Isn't that a potential use after free? AFAIK misc_deregister does not
> > guarentee the miscdev will no longer be accessed after it returns, and
> > the devm will free it after optee_remove returns.
> >
> > Memory backing a stuct device needs to be freed via the release
> > function.
>
> Out of interest, which struct device are you talking about here?
Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
refer to the entire thing, struct tee_device, struct misc_device, the
driver allocations, etc.
So, the first issue is the use-after-free via ioctl() touching struct
tee_device that you described.
But then we trundle down to:
+ ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
+ vers.uuid);
If we kref teedev so it is valid then calling a driver call back after
(or during) it's remove function is very likely to blow up.
Also, in TPM we discovered that adding a sysfs file was very ugly
(impossible?) because without the misc_mtx protection that open has,
getting a locked tee_device in the sysfs callback is difficult.
With TPM, we ended up trying lots of options for fixing struct
misc_device in the tpm core, which is handling multiple sub drivers,
and basically gave up. Gave each struct tpm_device an embedded struct
device like Greg suggested here. Then the tpm core is working with the
APIs, not struggling against them.
But this is not a user-space invisible change, so better to do it right
from day 1 ..
We followed rtc as an example of how to create a mid-layer that
exports it's own register function, with char dev and sysfs
components. It seems properly implemented, and has elegant solutions
to these problems (like ops):
- Don't mess with modules, use 'ops' and set 'ops' to null when the
driver removes. The driver core will keep the driver module around
for you bettwen the probe/remove calls. Setting ops = NULL ensures driver
module code cannot be called after remove.
- Use locking for 'ops' to serialize driver callbacks with driver removal
- Embed a struct device/etc in the struct tee_device and use the release
function to deallocate struct tee_device. All callbacks from the
driver/char/sysfs core can now use container_of on something that
is already holds the right kref.
- Consider an alloc/register pattern as we use now in TPM. This has proven
smart for TPM as it allows:
alloc tee_device + init struct device, etc
driver setup
core library helper calls for setup/etc
driver register + char dev publish
It appeared to me this driver was copying TPM's old architecture,
which is very much known to be broken.
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 21:57 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 21:57 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 11:29:23AM -0600, Jason Gunthorpe wrote:
> On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > > [..]
> > > > + rc = misc_register(&teedev->miscdev);
> > > [..]
> > > > +void tee_unregister(struct tee_device *teedev)
> > > > +{
> > > [..]
> > > > + misc_deregister(&teedev->miscdev);
> > > > +}
> > > [..]
> > > >+static int optee_remove(struct platform_device *pdev)
> > > >+{
> > > >+ tee_unregister(optee->teedev);
> > >
> > > Isn't that a potential use after free? AFAIK misc_deregister does not
> > > guarentee the miscdev will no longer be accessed after it returns, and
> > > the devm will free it after optee_remove returns.
> > >
> > > Memory backing a stuct device needs to be freed via the release
> > > function.
> >
> > Out of interest, which struct device are you talking about here?
>
> Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
> refer to the entire thing, struct tee_device, struct misc_device, the
> driver allocations, etc.
>
> So, the first issue is the use-after-free via ioctl() touching struct
> tee_device that you described.
>
> But then we trundle down to:
>
> + ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
> + vers.uuid);
>
> If we kref teedev so it is valid then calling a driver call back after
> (or during) it's remove function is very likely to blow up.
Why?
Normally, the file_operations will be associated with the module which,
in this case, called misc_register() - the same module which contains
the remove function.
So, the text for the remove function will still be available. The data
for the remove function will also be available, because we haven't
kref_put()'d the last reference yet.
So, where's the problem?
> Also, in TPM we discovered that adding a sysfs file was very ugly
> (impossible?) because without the misc_mtx protection that open has,
> getting a locked tee_device in the sysfs callback is difficult.
Right - the problem here is that a sysfs file attached to the class
device inside miscdev could be open at the point we want to free the
tee structures - and the lifetime of the miscdev class device is
unrelated to the lifetime of the tee structure.
For a device attribute attached to that class device, the lifetime
of the class device will be controlled by the sysfs file being open.
The problem comes when you want to try to get at some private data
(in this case, the tee private data) from that class device. The class
device has no driver data set. So, people may be tempted to use
dev->parent to grab the parent device's private data - and that's
dangerous, because after device_destroy() in misc_deregister(),
nothing guarantees that the class device's parent pointer remains
valid.
So, attaching attributes to the class device is _really_ a no-go.
The attributes should be attached to the parent device - the device
which is actually being driven.
The second option is to attach them to the struct device for the
device being driven. That has all the standard rules which come
from drivers attaching attributes to the struct device for which
they're driving, so that should be nothing new to anyone.
If that's hard to get right, then we have an error in the design of
the driver model - such stuff should be _easy_ to get right. For
example, the driver model should guarantee that when a driver
attribute is removed from a struct device, its method won't be
called any further (if it doesn't do this, I suspect we have a fair
number of buggy drivers...)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 21:57 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 21:57 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 11:29:23AM -0600, Jason Gunthorpe wrote:
> On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > > [..]
> > > > + rc = misc_register(&teedev->miscdev);
> > > [..]
> > > > +void tee_unregister(struct tee_device *teedev)
> > > > +{
> > > [..]
> > > > + misc_deregister(&teedev->miscdev);
> > > > +}
> > > [..]
> > > >+static int optee_remove(struct platform_device *pdev)
> > > >+{
> > > >+ tee_unregister(optee->teedev);
> > >
> > > Isn't that a potential use after free? AFAIK misc_deregister does not
> > > guarentee the miscdev will no longer be accessed after it returns, and
> > > the devm will free it after optee_remove returns.
> > >
> > > Memory backing a stuct device needs to be freed via the release
> > > function.
> >
> > Out of interest, which struct device are you talking about here?
>
> Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
> refer to the entire thing, struct tee_device, struct misc_device, the
> driver allocations, etc.
>
> So, the first issue is the use-after-free via ioctl() touching struct
> tee_device that you described.
>
> But then we trundle down to:
>
> + ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
> + vers.uuid);
>
> If we kref teedev so it is valid then calling a driver call back after
> (or during) it's remove function is very likely to blow up.
Why?
Normally, the file_operations will be associated with the module which,
in this case, called misc_register() - the same module which contains
the remove function.
So, the text for the remove function will still be available. The data
for the remove function will also be available, because we haven't
kref_put()'d the last reference yet.
So, where's the problem?
> Also, in TPM we discovered that adding a sysfs file was very ugly
> (impossible?) because without the misc_mtx protection that open has,
> getting a locked tee_device in the sysfs callback is difficult.
Right - the problem here is that a sysfs file attached to the class
device inside miscdev could be open at the point we want to free the
tee structures - and the lifetime of the miscdev class device is
unrelated to the lifetime of the tee structure.
For a device attribute attached to that class device, the lifetime
of the class device will be controlled by the sysfs file being open.
The problem comes when you want to try to get at some private data
(in this case, the tee private data) from that class device. The class
device has no driver data set. So, people may be tempted to use
dev->parent to grab the parent device's private data - and that's
dangerous, because after device_destroy() in misc_deregister(),
nothing guarantees that the class device's parent pointer remains
valid.
So, attaching attributes to the class device is _really_ a no-go.
The attributes should be attached to the parent device - the device
which is actually being driven.
The second option is to attach them to the struct device for the
device being driven. That has all the standard rules which come
from drivers attaching attributes to the struct device for which
they're driving, so that should be nothing new to anyone.
If that's hard to get right, then we have an error in the design of
the driver model - such stuff should be _easy_ to get right. For
example, the driver model should guarantee that when a driver
attribute is removed from a struct device, its method won't be
called any further (if it doesn't do this, I suspect we have a fair
number of buggy drivers...)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 21:57 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 21:57 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jens Wiklander, valentin.manea-hv44wF8Li93QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, javier-5MUHepqpBA1BDgjK7y7TUQ,
emmanuel.michel-qxv4g6HH51o, Herbert Xu, Arnd Bergmann,
Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jean-michel.delorme-qxv4g6HH51o,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Sat, Apr 18, 2015 at 11:29:23AM -0600, Jason Gunthorpe wrote:
> On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > > [..]
> > > > + rc = misc_register(&teedev->miscdev);
> > > [..]
> > > > +void tee_unregister(struct tee_device *teedev)
> > > > +{
> > > [..]
> > > > + misc_deregister(&teedev->miscdev);
> > > > +}
> > > [..]
> > > >+static int optee_remove(struct platform_device *pdev)
> > > >+{
> > > >+ tee_unregister(optee->teedev);
> > >
> > > Isn't that a potential use after free? AFAIK misc_deregister does not
> > > guarentee the miscdev will no longer be accessed after it returns, and
> > > the devm will free it after optee_remove returns.
> > >
> > > Memory backing a stuct device needs to be freed via the release
> > > function.
> >
> > Out of interest, which struct device are you talking about here?
>
> Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
> refer to the entire thing, struct tee_device, struct misc_device, the
> driver allocations, etc.
>
> So, the first issue is the use-after-free via ioctl() touching struct
> tee_device that you described.
>
> But then we trundle down to:
>
> + ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
> + vers.uuid);
>
> If we kref teedev so it is valid then calling a driver call back after
> (or during) it's remove function is very likely to blow up.
Why?
Normally, the file_operations will be associated with the module which,
in this case, called misc_register() - the same module which contains
the remove function.
So, the text for the remove function will still be available. The data
for the remove function will also be available, because we haven't
kref_put()'d the last reference yet.
So, where's the problem?
> Also, in TPM we discovered that adding a sysfs file was very ugly
> (impossible?) because without the misc_mtx protection that open has,
> getting a locked tee_device in the sysfs callback is difficult.
Right - the problem here is that a sysfs file attached to the class
device inside miscdev could be open at the point we want to free the
tee structures - and the lifetime of the miscdev class device is
unrelated to the lifetime of the tee structure.
For a device attribute attached to that class device, the lifetime
of the class device will be controlled by the sysfs file being open.
The problem comes when you want to try to get at some private data
(in this case, the tee private data) from that class device. The class
device has no driver data set. So, people may be tempted to use
dev->parent to grab the parent device's private data - and that's
dangerous, because after device_destroy() in misc_deregister(),
nothing guarantees that the class device's parent pointer remains
valid.
So, attaching attributes to the class device is _really_ a no-go.
The attributes should be attached to the parent device - the device
which is actually being driven.
The second option is to attach them to the struct device for the
device being driven. That has all the standard rules which come
from drivers attaching attributes to the struct device for which
they're driving, so that should be nothing new to anyone.
If that's hard to get right, then we have an error in the design of
the driver model - such stuff should be _easy_ to get right. For
example, the driver model should guarantee that when a driver
attribute is removed from a struct device, its method won't be
called any further (if it doesn't do this, I suspect we have a fair
number of buggy drivers...)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-18 21:57 ` Russell King - ARM Linux
@ 2015-04-20 5:08 ` Jason Gunthorpe
-1 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 5:08 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 10:57:55PM +0100, Russell King - ARM Linux wrote:
> > But then we trundle down to:
> >
> > + ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
> > + vers.uuid);
> >
> > If we kref teedev so it is valid then calling a driver call back after
> > (or during) it's remove function is very likely to blow up.
>
> Why?
>
> Normally, the file_operations will be associated with the module which,
> in this case, called misc_register() - the same module which contains
> the remove function.
As I read it, tee is similar to TPM and RTC, this code is the mid
layer code, and resides in it's own module while the function pointers
in 'ops' reside in the actual driver. So there are two modules.
There is some mucking in tee to keep the driver module around, it is
probably OK for the chardev, but again, sysfs, probably not. Absent
that code I think the module ops points to could be unloaded and the
.text backing the ops function can go away.
However, I was worrying about the driver itself - what is a driver to
do if a callback is called after tee_unregister is called? It must do
nothing and return error. Drawing from TPM, most drivers did not have
this check, so they blow up. Ie devm frees all their resources after
tee_unregister returns and they quickly deref null, or sleep on
disabled IRQ or timer, or something else fatal.
It seems like a good idea to have the midlayer guarentee the driver
ops cannot be called after the midlayer unregister function returns so
that drivers can operate in a simple environment. Nulling ops also
avoids caring about the driver's module ref count.
> > Also, in TPM we discovered that adding a sysfs file was very ugly
> > (impossible?) because without the misc_mtx protection that open has,
> > getting a locked tee_device in the sysfs callback is difficult.
> So, attaching attributes to the class device is _really_ a no-go.
> The attributes should be attached to the parent device - the device
> which is actually being driven.
But common midlayer convention is to put them in the same directory as
the 'dev', ie:
/sys/devices/pnp0/00:06/rtc/rtc0/dev
/sys/devices/pnp0/00:06/rtc/rtc0/max_user_freq
Which, I think, is misc->this_device
> attribute is removed from a struct device, its method won't be
> called any further (if it doesn't do this, I suspect we have a fair
> number of buggy drivers...)
Hmm, I went and looked again, and it seems kernfs_drain is part of a
mechanism that causes kernfs_remove to wait until all opens are
closed, so it sure looks like a sysfs callback cannot be called after
it is removed.
Years ago lwn documented that this wasn't true,
https://lwn.net/Articles/54651/, it is nice to see that has changed.
I still suspect the expected way to write a new mid layer is to create
your own struct device and not rely on misc_device, but use-after-free
races from sysfs doesn't seem to be a motivating reason...
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread
* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 5:08 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 5:08 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 10:57:55PM +0100, Russell King - ARM Linux wrote:
> > But then we trundle down to:
> >
> > + ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
> > + vers.uuid);
> >
> > If we kref teedev so it is valid then calling a driver call back after
> > (or during) it's remove function is very likely to blow up.
>
> Why?
>
> Normally, the file_operations will be associated with the module which,
> in this case, called misc_register() - the same module which contains
> the remove function.
As I read it, tee is similar to TPM and RTC, this code is the mid
layer code, and resides in it's own module while the function pointers
in 'ops' reside in the actual driver. So there are two modules.
There is some mucking in tee to keep the driver module around, it is
probably OK for the chardev, but again, sysfs, probably not. Absent
that code I think the module ops points to could be unloaded and the
.text backing the ops function can go away.
However, I was worrying about the driver itself - what is a driver to
do if a callback is called after tee_unregister is called? It must do
nothing and return error. Drawing from TPM, most drivers did not have
this check, so they blow up. Ie devm frees all their resources after
tee_unregister returns and they quickly deref null, or sleep on
disabled IRQ or timer, or something else fatal.
It seems like a good idea to have the midlayer guarentee the driver
ops cannot be called after the midlayer unregister function returns so
that drivers can operate in a simple environment. Nulling ops also
avoids caring about the driver's module ref count.
> > Also, in TPM we discovered that adding a sysfs file was very ugly
> > (impossible?) because without the misc_mtx protection that open has,
> > getting a locked tee_device in the sysfs callback is difficult.
> So, attaching attributes to the class device is _really_ a no-go.
> The attributes should be attached to the parent device - the device
> which is actually being driven.
But common midlayer convention is to put them in the same directory as
the 'dev', ie:
/sys/devices/pnp0/00:06/rtc/rtc0/dev
/sys/devices/pnp0/00:06/rtc/rtc0/max_user_freq
Which, I think, is misc->this_device
> attribute is removed from a struct device, its method won't be
> called any further (if it doesn't do this, I suspect we have a fair
> number of buggy drivers...)
Hmm, I went and looked again, and it seems kernfs_drain is part of a
mechanism that causes kernfs_remove to wait until all opens are
closed, so it sure looks like a sysfs callback cannot be called after
it is removed.
Years ago lwn documented that this wasn't true,
https://lwn.net/Articles/54651/, it is nice to see that has changed.
I still suspect the expected way to write a new mid layer is to create
your own struct device and not rely on misc_device, but use-after-free
races from sysfs doesn't seem to be a motivating reason...
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-20 5:08 ` Jason Gunthorpe
@ 2015-04-20 14:54 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-20 14:54 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Russell King - ARM Linux, Jens Wiklander, valentin.manea,
devicetree, javier, emmanuel.michel, Herbert Xu, Arnd Bergmann,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sun, Apr 19, 2015 at 11:08:00PM -0600, Jason Gunthorpe wrote:
> I still suspect the expected way to write a new mid layer is to create
> your own struct device and not rely on misc_device,
Yes, that is the way. You can not use misc_device for anything other
than creating the char node that your driver can use through the fileops
you pass to it.
Do not use a misc_device to create sysfs files for, or anything else, it
will be wrong and racy, as you have pointed out.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread
* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 14:54 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-20 14:54 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Apr 19, 2015 at 11:08:00PM -0600, Jason Gunthorpe wrote:
> I still suspect the expected way to write a new mid layer is to create
> your own struct device and not rely on misc_device,
Yes, that is the way. You can not use misc_device for anything other
than creating the char node that your driver can use through the fileops
you pass to it.
Do not use a misc_device to create sysfs files for, or anything else, it
will be wrong and racy, as you have pointed out.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 15:56 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 15:56 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Russell King - ARM Linux, Jens Wiklander, valentin.manea,
devicetree, javier, emmanuel.michel, Herbert Xu, Arnd Bergmann,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Mon, Apr 20, 2015 at 04:54:32PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Apr 19, 2015 at 11:08:00PM -0600, Jason Gunthorpe wrote:
> > I still suspect the expected way to write a new mid layer is to create
> > your own struct device and not rely on misc_device,
>
> Yes, that is the way. You can not use misc_device for anything other
> than creating the char node that your driver can use through the fileops
> you pass to it.
>
> Do not use a misc_device to create sysfs files for, or anything else, it
> will be wrong and racy, as you have pointed out.
Thanks!
Can you quickly comment on when a misc device should be used compared to
alloc_chrdev_region & cdev_add ?
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread
* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 15:56 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 15:56 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 20, 2015 at 04:54:32PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Apr 19, 2015 at 11:08:00PM -0600, Jason Gunthorpe wrote:
> > I still suspect the expected way to write a new mid layer is to create
> > your own struct device and not rely on misc_device,
>
> Yes, that is the way. You can not use misc_device for anything other
> than creating the char node that your driver can use through the fileops
> you pass to it.
>
> Do not use a misc_device to create sysfs files for, or anything else, it
> will be wrong and racy, as you have pointed out.
Thanks!
Can you quickly comment on when a misc device should be used compared to
alloc_chrdev_region & cdev_add ?
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 15:56 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 15:56 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Russell King - ARM Linux, Jens Wiklander,
valentin.manea-hv44wF8Li93QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, javier-5MUHepqpBA1BDgjK7y7TUQ,
emmanuel.michel-qxv4g6HH51o, Herbert Xu, Arnd Bergmann,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jean-michel.delorme-qxv4g6HH51o,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Mon, Apr 20, 2015 at 04:54:32PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Apr 19, 2015 at 11:08:00PM -0600, Jason Gunthorpe wrote:
> > I still suspect the expected way to write a new mid layer is to create
> > your own struct device and not rely on misc_device,
>
> Yes, that is the way. You can not use misc_device for anything other
> than creating the char node that your driver can use through the fileops
> you pass to it.
>
> Do not use a misc_device to create sysfs files for, or anything else, it
> will be wrong and racy, as you have pointed out.
Thanks!
Can you quickly comment on when a misc device should be used compared to
alloc_chrdev_region & cdev_add ?
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 16:05 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-20 16:05 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Russell King - ARM Linux, Jens Wiklander, valentin.manea,
devicetree, javier, emmanuel.michel, Herbert Xu, Arnd Bergmann,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Mon, Apr 20, 2015 at 09:56:48AM -0600, Jason Gunthorpe wrote:
> On Mon, Apr 20, 2015 at 04:54:32PM +0200, Greg Kroah-Hartman wrote:
> > On Sun, Apr 19, 2015 at 11:08:00PM -0600, Jason Gunthorpe wrote:
> > > I still suspect the expected way to write a new mid layer is to create
> > > your own struct device and not rely on misc_device,
> >
> > Yes, that is the way. You can not use misc_device for anything other
> > than creating the char node that your driver can use through the fileops
> > you pass to it.
> >
> > Do not use a misc_device to create sysfs files for, or anything else, it
> > will be wrong and racy, as you have pointed out.
>
> Thanks!
>
> Can you quickly comment on when a misc device should be used compared to
> alloc_chrdev_region & cdev_add ?
If you need more than one character device node, don't use a misc
device. If you only need one, and nothing "special" or "fancy", and
don't want to put sysfs attribute files on your device, then use a misc
device.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread
* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 16:05 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-20 16:05 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 20, 2015 at 09:56:48AM -0600, Jason Gunthorpe wrote:
> On Mon, Apr 20, 2015 at 04:54:32PM +0200, Greg Kroah-Hartman wrote:
> > On Sun, Apr 19, 2015 at 11:08:00PM -0600, Jason Gunthorpe wrote:
> > > I still suspect the expected way to write a new mid layer is to create
> > > your own struct device and not rely on misc_device,
> >
> > Yes, that is the way. You can not use misc_device for anything other
> > than creating the char node that your driver can use through the fileops
> > you pass to it.
> >
> > Do not use a misc_device to create sysfs files for, or anything else, it
> > will be wrong and racy, as you have pointed out.
>
> Thanks!
>
> Can you quickly comment on when a misc device should be used compared to
> alloc_chrdev_region & cdev_add ?
If you need more than one character device node, don't use a misc
device. If you only need one, and nothing "special" or "fancy", and
don't want to put sysfs attribute files on your device, then use a misc
device.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 16:05 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-20 16:05 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Russell King - ARM Linux, Jens Wiklander,
valentin.manea-hv44wF8Li93QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, javier-5MUHepqpBA1BDgjK7y7TUQ,
emmanuel.michel-qxv4g6HH51o, Herbert Xu, Arnd Bergmann,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jean-michel.delorme-qxv4g6HH51o,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Mon, Apr 20, 2015 at 09:56:48AM -0600, Jason Gunthorpe wrote:
> On Mon, Apr 20, 2015 at 04:54:32PM +0200, Greg Kroah-Hartman wrote:
> > On Sun, Apr 19, 2015 at 11:08:00PM -0600, Jason Gunthorpe wrote:
> > > I still suspect the expected way to write a new mid layer is to create
> > > your own struct device and not rely on misc_device,
> >
> > Yes, that is the way. You can not use misc_device for anything other
> > than creating the char node that your driver can use through the fileops
> > you pass to it.
> >
> > Do not use a misc_device to create sysfs files for, or anything else, it
> > will be wrong and racy, as you have pointed out.
>
> Thanks!
>
> Can you quickly comment on when a misc device should be used compared to
> alloc_chrdev_region & cdev_add ?
If you need more than one character device node, don't use a misc
device. If you only need one, and nothing "special" or "fancy", and
don't want to put sysfs attribute files on your device, then use a misc
device.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 13:02 ` Jens Wiklander
0 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-20 13:02 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Russell King - ARM Linux, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 11:29:23AM -0600, Jason Gunthorpe wrote:
> On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > > [..]
> > > > + rc = misc_register(&teedev->miscdev);
> > > [..]
> > > > +void tee_unregister(struct tee_device *teedev)
> > > > +{
> > > [..]
> > > > + misc_deregister(&teedev->miscdev);
> > > > +}
> > > [..]
> > > >+static int optee_remove(struct platform_device *pdev)
> > > >+{
> > > >+ tee_unregister(optee->teedev);
> > >
> > > Isn't that a potential use after free? AFAIK misc_deregister does not
> > > guarentee the miscdev will no longer be accessed after it returns, and
> > > the devm will free it after optee_remove returns.
> > >
> > > Memory backing a stuct device needs to be freed via the release
> > > function.
> >
> > Out of interest, which struct device are you talking about here?
>
> Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
> refer to the entire thing, struct tee_device, struct misc_device, the
> driver allocations, etc.
>
> So, the first issue is the use-after-free via ioctl() touching struct
> tee_device that you described.
>
> But then we trundle down to:
>
> + ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
> + vers.uuid);
>
> If we kref teedev so it is valid then calling a driver call back after
> (or during) it's remove function is very likely to blow up.
>
> Also, in TPM we discovered that adding a sysfs file was very ugly
> (impossible?) because without the misc_mtx protection that open has,
> getting a locked tee_device in the sysfs callback is difficult.
>
> With TPM, we ended up trying lots of options for fixing struct
> misc_device in the tpm core, which is handling multiple sub drivers,
> and basically gave up. Gave each struct tpm_device an embedded struct
> device like Greg suggested here. Then the tpm core is working with the
> APIs, not struggling against them.
>
> But this is not a user-space invisible change, so better to do it right
> from day 1 ..
>
> We followed rtc as an example of how to create a mid-layer that
> exports it's own register function, with char dev and sysfs
> components. It seems properly implemented, and has elegant solutions
> to these problems (like ops):
> - Don't mess with modules, use 'ops' and set 'ops' to null when the
> driver removes. The driver core will keep the driver module around
> for you bettwen the probe/remove calls. Setting ops = NULL ensures driver
> module code cannot be called after remove.
> - Use locking for 'ops' to serialize driver callbacks with driver removal
> - Embed a struct device/etc in the struct tee_device and use the release
> function to deallocate struct tee_device. All callbacks from the
> driver/char/sysfs core can now use container_of on something that
> is already holds the right kref.
> - Consider an alloc/register pattern as we use now in TPM. This has proven
> smart for TPM as it allows:
> alloc tee_device + init struct device, etc
> driver setup
> core library helper calls for setup/etc
> driver register + char dev publish
>
> It appeared to me this driver was copying TPM's old architecture,
> which is very much known to be broken.
The struct tee_device holds a shared memory pool from which shared
memory objects are allocated. These shared memory objects can be mapped
both by user space and secure world. When a shared memory object is
freed secure world may need to be notified, OP-TEE doesn't need that but
other TEEs may need it. To come around the problem with what should
happen when the driver is removed I'm increasing the refcount on the
driver for each allocated shared memory object and created file
pointers. As long as any resource is in use by either user space or
secure world the driver can't be unloaded.
What should happen when user space or secure world has some shared
memory which the driver owns (either allocated from CMA or from some
memory range received from the TEE)? Killing the user space process
would be a bit unfriendly. Secure world may not be prepared to release
that memory right now either.
What if I:
* Keep all the reference counting I have today on the module
to make sure that the module can't be unloaded until all shared memory
resources are release
* Change to use the pattern (with a struct device etc) as described above.
I can't protect the ops with just a mutex since tee_ioctl_cmd() needs to
be multithreaded. I need some reference counting too,
try_module_get()/module_put() does exactly what I need here.
Is that pattern OK?
Regards,
Jens
^ permalink raw reply [flat|nested] 85+ messages in thread* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 13:02 ` Jens Wiklander
0 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-20 13:02 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 11:29:23AM -0600, Jason Gunthorpe wrote:
> On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > > [..]
> > > > + rc = misc_register(&teedev->miscdev);
> > > [..]
> > > > +void tee_unregister(struct tee_device *teedev)
> > > > +{
> > > [..]
> > > > + misc_deregister(&teedev->miscdev);
> > > > +}
> > > [..]
> > > >+static int optee_remove(struct platform_device *pdev)
> > > >+{
> > > >+ tee_unregister(optee->teedev);
> > >
> > > Isn't that a potential use after free? AFAIK misc_deregister does not
> > > guarentee the miscdev will no longer be accessed after it returns, and
> > > the devm will free it after optee_remove returns.
> > >
> > > Memory backing a stuct device needs to be freed via the release
> > > function.
> >
> > Out of interest, which struct device are you talking about here?
>
> Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
> refer to the entire thing, struct tee_device, struct misc_device, the
> driver allocations, etc.
>
> So, the first issue is the use-after-free via ioctl() touching struct
> tee_device that you described.
>
> But then we trundle down to:
>
> + ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
> + vers.uuid);
>
> If we kref teedev so it is valid then calling a driver call back after
> (or during) it's remove function is very likely to blow up.
>
> Also, in TPM we discovered that adding a sysfs file was very ugly
> (impossible?) because without the misc_mtx protection that open has,
> getting a locked tee_device in the sysfs callback is difficult.
>
> With TPM, we ended up trying lots of options for fixing struct
> misc_device in the tpm core, which is handling multiple sub drivers,
> and basically gave up. Gave each struct tpm_device an embedded struct
> device like Greg suggested here. Then the tpm core is working with the
> APIs, not struggling against them.
>
> But this is not a user-space invisible change, so better to do it right
> from day 1 ..
>
> We followed rtc as an example of how to create a mid-layer that
> exports it's own register function, with char dev and sysfs
> components. It seems properly implemented, and has elegant solutions
> to these problems (like ops):
> - Don't mess with modules, use 'ops' and set 'ops' to null when the
> driver removes. The driver core will keep the driver module around
> for you bettwen the probe/remove calls. Setting ops = NULL ensures driver
> module code cannot be called after remove.
> - Use locking for 'ops' to serialize driver callbacks with driver removal
> - Embed a struct device/etc in the struct tee_device and use the release
> function to deallocate struct tee_device. All callbacks from the
> driver/char/sysfs core can now use container_of on something that
> is already holds the right kref.
> - Consider an alloc/register pattern as we use now in TPM. This has proven
> smart for TPM as it allows:
> alloc tee_device + init struct device, etc
> driver setup
> core library helper calls for setup/etc
> driver register + char dev publish
>
> It appeared to me this driver was copying TPM's old architecture,
> which is very much known to be broken.
The struct tee_device holds a shared memory pool from which shared
memory objects are allocated. These shared memory objects can be mapped
both by user space and secure world. When a shared memory object is
freed secure world may need to be notified, OP-TEE doesn't need that but
other TEEs may need it. To come around the problem with what should
happen when the driver is removed I'm increasing the refcount on the
driver for each allocated shared memory object and created file
pointers. As long as any resource is in use by either user space or
secure world the driver can't be unloaded.
What should happen when user space or secure world has some shared
memory which the driver owns (either allocated from CMA or from some
memory range received from the TEE)? Killing the user space process
would be a bit unfriendly. Secure world may not be prepared to release
that memory right now either.
What if I:
* Keep all the reference counting I have today on the module
to make sure that the module can't be unloaded until all shared memory
resources are release
* Change to use the pattern (with a struct device etc) as described above.
I can't protect the ops with just a mutex since tee_ioctl_cmd() needs to
be multithreaded. I need some reference counting too,
try_module_get()/module_put() does exactly what I need here.
Is that pattern OK?
Regards,
Jens
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 13:02 ` Jens Wiklander
0 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-20 13:02 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Russell King - ARM Linux, valentin.manea-hv44wF8Li93QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, javier-5MUHepqpBA1BDgjK7y7TUQ,
emmanuel.michel-qxv4g6HH51o, Herbert Xu, Arnd Bergmann,
Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jean-michel.delorme-qxv4g6HH51o,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Sat, Apr 18, 2015 at 11:29:23AM -0600, Jason Gunthorpe wrote:
> On Sat, Apr 18, 2015 at 10:01:47AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Apr 17, 2015 at 10:30:54AM -0600, Jason Gunthorpe wrote:
> > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > + teedev = devm_kzalloc(dev, sizeof(*teedev), GFP_KERNEL);
> > > [..]
> > > > + rc = misc_register(&teedev->miscdev);
> > > [..]
> > > > +void tee_unregister(struct tee_device *teedev)
> > > > +{
> > > [..]
> > > > + misc_deregister(&teedev->miscdev);
> > > > +}
> > > [..]
> > > >+static int optee_remove(struct platform_device *pdev)
> > > >+{
> > > >+ tee_unregister(optee->teedev);
> > >
> > > Isn't that a potential use after free? AFAIK misc_deregister does not
> > > guarentee the miscdev will no longer be accessed after it returns, and
> > > the devm will free it after optee_remove returns.
> > >
> > > Memory backing a stuct device needs to be freed via the release
> > > function.
> >
> > Out of interest, which struct device are you talking about here?
>
> Sorry, I was imprecise. In the first paragraph I ment 'miscdev' to
> refer to the entire thing, struct tee_device, struct misc_device, the
> driver allocations, etc.
>
> So, the first issue is the use-after-free via ioctl() touching struct
> tee_device that you described.
>
> But then we trundle down to:
>
> + ctx->teedev->desc->ops->get_version(ctx, &vers.spec_version,
> + vers.uuid);
>
> If we kref teedev so it is valid then calling a driver call back after
> (or during) it's remove function is very likely to blow up.
>
> Also, in TPM we discovered that adding a sysfs file was very ugly
> (impossible?) because without the misc_mtx protection that open has,
> getting a locked tee_device in the sysfs callback is difficult.
>
> With TPM, we ended up trying lots of options for fixing struct
> misc_device in the tpm core, which is handling multiple sub drivers,
> and basically gave up. Gave each struct tpm_device an embedded struct
> device like Greg suggested here. Then the tpm core is working with the
> APIs, not struggling against them.
>
> But this is not a user-space invisible change, so better to do it right
> from day 1 ..
>
> We followed rtc as an example of how to create a mid-layer that
> exports it's own register function, with char dev and sysfs
> components. It seems properly implemented, and has elegant solutions
> to these problems (like ops):
> - Don't mess with modules, use 'ops' and set 'ops' to null when the
> driver removes. The driver core will keep the driver module around
> for you bettwen the probe/remove calls. Setting ops = NULL ensures driver
> module code cannot be called after remove.
> - Use locking for 'ops' to serialize driver callbacks with driver removal
> - Embed a struct device/etc in the struct tee_device and use the release
> function to deallocate struct tee_device. All callbacks from the
> driver/char/sysfs core can now use container_of on something that
> is already holds the right kref.
> - Consider an alloc/register pattern as we use now in TPM. This has proven
> smart for TPM as it allows:
> alloc tee_device + init struct device, etc
> driver setup
> core library helper calls for setup/etc
> driver register + char dev publish
>
> It appeared to me this driver was copying TPM's old architecture,
> which is very much known to be broken.
The struct tee_device holds a shared memory pool from which shared
memory objects are allocated. These shared memory objects can be mapped
both by user space and secure world. When a shared memory object is
freed secure world may need to be notified, OP-TEE doesn't need that but
other TEEs may need it. To come around the problem with what should
happen when the driver is removed I'm increasing the refcount on the
driver for each allocated shared memory object and created file
pointers. As long as any resource is in use by either user space or
secure world the driver can't be unloaded.
What should happen when user space or secure world has some shared
memory which the driver owns (either allocated from CMA or from some
memory range received from the TEE)? Killing the user space process
would be a bit unfriendly. Secure world may not be prepared to release
that memory right now either.
What if I:
* Keep all the reference counting I have today on the module
to make sure that the module can't be unloaded until all shared memory
resources are release
* Change to use the pattern (with a struct device etc) as described above.
I can't protect the ops with just a mutex since tee_ioctl_cmd() needs to
be multithreaded. I need some reference counting too,
try_module_get()/module_put() does exactly what I need here.
Is that pattern OK?
Regards,
Jens
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-20 13:02 ` Jens Wiklander
(?)
@ 2015-04-20 17:55 ` Jason Gunthorpe
-1 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 17:55 UTC (permalink / raw)
To: Jens Wiklander
Cc: Russell King - ARM Linux, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Mon, Apr 20, 2015 at 03:02:03PM +0200, Jens Wiklander wrote:
> > It appeared to me this driver was copying TPM's old architecture,
> > which is very much known to be broken.
>
> The struct tee_device holds a shared memory pool from which shared
> memory objects are allocated. These shared memory objects can be mapped
> both by user space and secure world.
So this is a whole other set of problems besides what was already
brought up.
You need to figure out a lifetime model for this shared memory that
works.
> To come around the problem with what should happen when the driver
> is removed I'm increasing the refcount on the driver for each
> allocated shared memory object and created file pointers. As long as
> any resource is in use by either user space or secure world the
> driver can't be unloaded.
This isn't how the kernel works. The module refcount effects module
unload (it protects the .text) - it does not interact with driver
detatch. Userspace can trigger driver detatch (which results in
tee_unregister being called) at any time via sysfs.
If you properly design for that case then module unload sequencing
works properly for free.
Based on what I gather, I would suggest the following sequence in
tee_unregister
- unregister all sysfs and char dev registrations.
- Write lock ops and set to null. This will error future cdev ioctls,
and guarentees no driver ops callbacks are in progress, or will be
started in future.
- Wait until all client accesses to shared memory are
released.
- Command the driver to release it's side of the
shared memory and wait for that to complete
- Free the shared memory
- deref the tee_device's struct device (match ref in tee_register)
Then in your struct tee_device's release function free the tee_device
memory.
Replace all the module locking code with an active count in struct
tee_device (see something like kernfs_drain for an example).
> * Change to use the pattern (with a struct device etc) as described
> above.
Yes, I think Greg confirmed you need to use a struct device, and purge
misc_device from the mid layer.
> I can't protect the ops with just a mutex since tee_ioctl_cmd() needs to
> be multithreaded.
Then use a sleeping read/write lock - aka an active count.
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread
* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 17:55 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 17:55 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 20, 2015 at 03:02:03PM +0200, Jens Wiklander wrote:
> > It appeared to me this driver was copying TPM's old architecture,
> > which is very much known to be broken.
>
> The struct tee_device holds a shared memory pool from which shared
> memory objects are allocated. These shared memory objects can be mapped
> both by user space and secure world.
So this is a whole other set of problems besides what was already
brought up.
You need to figure out a lifetime model for this shared memory that
works.
> To come around the problem with what should happen when the driver
> is removed I'm increasing the refcount on the driver for each
> allocated shared memory object and created file pointers. As long as
> any resource is in use by either user space or secure world the
> driver can't be unloaded.
This isn't how the kernel works. The module refcount effects module
unload (it protects the .text) - it does not interact with driver
detatch. Userspace can trigger driver detatch (which results in
tee_unregister being called) at any time via sysfs.
If you properly design for that case then module unload sequencing
works properly for free.
Based on what I gather, I would suggest the following sequence in
tee_unregister
- unregister all sysfs and char dev registrations.
- Write lock ops and set to null. This will error future cdev ioctls,
and guarentees no driver ops callbacks are in progress, or will be
started in future.
- Wait until all client accesses to shared memory are
released.
- Command the driver to release it's side of the
shared memory and wait for that to complete
- Free the shared memory
- deref the tee_device's struct device (match ref in tee_register)
Then in your struct tee_device's release function free the tee_device
memory.
Replace all the module locking code with an active count in struct
tee_device (see something like kernfs_drain for an example).
> * Change to use the pattern (with a struct device etc) as described
> above.
Yes, I think Greg confirmed you need to use a struct device, and purge
misc_device from the mid layer.
> I can't protect the ops with just a mutex since tee_ioctl_cmd() needs to
> be multithreaded.
Then use a sleeping read/write lock - aka an active count.
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 17:55 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 17:55 UTC (permalink / raw)
To: Jens Wiklander
Cc: Russell King - ARM Linux, valentin.manea-hv44wF8Li93QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, javier-5MUHepqpBA1BDgjK7y7TUQ,
emmanuel.michel-qxv4g6HH51o, Herbert Xu, Arnd Bergmann,
Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jean-michel.delorme-qxv4g6HH51o,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Mon, Apr 20, 2015 at 03:02:03PM +0200, Jens Wiklander wrote:
> > It appeared to me this driver was copying TPM's old architecture,
> > which is very much known to be broken.
>
> The struct tee_device holds a shared memory pool from which shared
> memory objects are allocated. These shared memory objects can be mapped
> both by user space and secure world.
So this is a whole other set of problems besides what was already
brought up.
You need to figure out a lifetime model for this shared memory that
works.
> To come around the problem with what should happen when the driver
> is removed I'm increasing the refcount on the driver for each
> allocated shared memory object and created file pointers. As long as
> any resource is in use by either user space or secure world the
> driver can't be unloaded.
This isn't how the kernel works. The module refcount effects module
unload (it protects the .text) - it does not interact with driver
detatch. Userspace can trigger driver detatch (which results in
tee_unregister being called) at any time via sysfs.
If you properly design for that case then module unload sequencing
works properly for free.
Based on what I gather, I would suggest the following sequence in
tee_unregister
- unregister all sysfs and char dev registrations.
- Write lock ops and set to null. This will error future cdev ioctls,
and guarentees no driver ops callbacks are in progress, or will be
started in future.
- Wait until all client accesses to shared memory are
released.
- Command the driver to release it's side of the
shared memory and wait for that to complete
- Free the shared memory
- deref the tee_device's struct device (match ref in tee_register)
Then in your struct tee_device's release function free the tee_device
memory.
Replace all the module locking code with an active count in struct
tee_device (see something like kernfs_drain for an example).
> * Change to use the pattern (with a struct device etc) as described
> above.
Yes, I think Greg confirmed you need to use a struct device, and purge
misc_device from the mid layer.
> I can't protect the ops with just a mutex since tee_ioctl_cmd() needs to
> be multithreaded.
Then use a sleeping read/write lock - aka an active count.
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-20 17:55 ` Jason Gunthorpe
@ 2015-04-21 5:59 ` Jens Wiklander
-1 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-21 5:59 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Russell King - ARM Linux, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Mon, Apr 20, 2015 at 11:55:15AM -0600, Jason Gunthorpe wrote:
> On Mon, Apr 20, 2015 at 03:02:03PM +0200, Jens Wiklander wrote:
> > > It appeared to me this driver was copying TPM's old architecture,
> > > which is very much known to be broken.
> >
> > The struct tee_device holds a shared memory pool from which shared
> > memory objects are allocated. These shared memory objects can be mapped
> > both by user space and secure world.
>
> So this is a whole other set of problems besides what was already
> brought up.
>
> You need to figure out a lifetime model for this shared memory that
> works.
>
> > To come around the problem with what should happen when the driver
> > is removed I'm increasing the refcount on the driver for each
> > allocated shared memory object and created file pointers. As long as
> > any resource is in use by either user space or secure world the
> > driver can't be unloaded.
>
> This isn't how the kernel works. The module refcount effects module
> unload (it protects the .text) - it does not interact with driver
> detatch. Userspace can trigger driver detatch (which results in
> tee_unregister being called) at any time via sysfs.
>
> If you properly design for that case then module unload sequencing
> works properly for free.
>
> Based on what I gather, I would suggest the following sequence in
> tee_unregister
> - unregister all sysfs and char dev registrations.
> - Write lock ops and set to null. This will error future cdev ioctls,
> and guarentees no driver ops callbacks are in progress, or will be
> started in future.
> - Wait until all client accesses to shared memory are
> released.
> - Command the driver to release it's side of the
> shared memory and wait for that to complete
> - Free the shared memory
> - deref the tee_device's struct device (match ref in tee_register)
>
> Then in your struct tee_device's release function free the tee_device
> memory.
>
> Replace all the module locking code with an active count in struct
> tee_device (see something like kernfs_drain for an example).
>
> > * Change to use the pattern (with a struct device etc) as described
> > above.
>
> Yes, I think Greg confirmed you need to use a struct device, and purge
> misc_device from the mid layer.
>
> > I can't protect the ops with just a mutex since tee_ioctl_cmd() needs to
> > be multithreaded.
>
> Then use a sleeping read/write lock - aka an active count.
Thanks for the clarification, I got it now.
Regards,
Jens
^ permalink raw reply [flat|nested] 85+ messages in thread
* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-21 5:59 ` Jens Wiklander
0 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-21 5:59 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 20, 2015 at 11:55:15AM -0600, Jason Gunthorpe wrote:
> On Mon, Apr 20, 2015 at 03:02:03PM +0200, Jens Wiklander wrote:
> > > It appeared to me this driver was copying TPM's old architecture,
> > > which is very much known to be broken.
> >
> > The struct tee_device holds a shared memory pool from which shared
> > memory objects are allocated. These shared memory objects can be mapped
> > both by user space and secure world.
>
> So this is a whole other set of problems besides what was already
> brought up.
>
> You need to figure out a lifetime model for this shared memory that
> works.
>
> > To come around the problem with what should happen when the driver
> > is removed I'm increasing the refcount on the driver for each
> > allocated shared memory object and created file pointers. As long as
> > any resource is in use by either user space or secure world the
> > driver can't be unloaded.
>
> This isn't how the kernel works. The module refcount effects module
> unload (it protects the .text) - it does not interact with driver
> detatch. Userspace can trigger driver detatch (which results in
> tee_unregister being called) at any time via sysfs.
>
> If you properly design for that case then module unload sequencing
> works properly for free.
>
> Based on what I gather, I would suggest the following sequence in
> tee_unregister
> - unregister all sysfs and char dev registrations.
> - Write lock ops and set to null. This will error future cdev ioctls,
> and guarentees no driver ops callbacks are in progress, or will be
> started in future.
> - Wait until all client accesses to shared memory are
> released.
> - Command the driver to release it's side of the
> shared memory and wait for that to complete
> - Free the shared memory
> - deref the tee_device's struct device (match ref in tee_register)
>
> Then in your struct tee_device's release function free the tee_device
> memory.
>
> Replace all the module locking code with an active count in struct
> tee_device (see something like kernfs_drain for an example).
>
> > * Change to use the pattern (with a struct device etc) as described
> > above.
>
> Yes, I think Greg confirmed you need to use a struct device, and purge
> misc_device from the mid layer.
>
> > I can't protect the ops with just a mutex since tee_ioctl_cmd() needs to
> > be multithreaded.
>
> Then use a sleeping read/write lock - aka an active count.
Thanks for the clarification, I got it now.
Regards,
Jens
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-17 7:50 ` Jens Wiklander
@ 2015-04-17 20:07 ` Arnd Bergmann
-1 siblings, 0 replies; 85+ messages in thread
From: Arnd Bergmann @ 2015-04-17 20:07 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, devicetree, Greg Kroah-Hartman,
javier, Herbert Xu, tpmdd-devel, valentin.manea,
jean-michel.delorme, emmanuel.michel
On Friday 17 April 2015 09:50:56 Jens Wiklander wrote:
> Documentation/ioctl/ioctl-number.txt | 1 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/tee/Kconfig | 8 +
> drivers/tee/Makefile | 3 +
> drivers/tee/tee.c | 253 +++++++++++++++++++++++++++
> drivers/tee/tee_private.h | 64 +++++++
> drivers/tee/tee_shm.c | 330 +++++++++++++++++++++++++++++++++++
> drivers/tee/tee_shm_pool.c | 246 ++++++++++++++++++++++++++
> include/linux/tee/tee.h | 180 +++++++++++++++++++
> include/linux/tee/tee_drv.h | 271 ++++++++++++++++++++++++++++
Hi Jens,
The driver looks very well implemented, but as you are introducing a new user
space API, we have to very carefully consider every aspect of that interface,
so I'm commenting mainly on user-visible parts.
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index 8136e1f..6e9bd04 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/sec-hw/tee.h Generic TEE subsystem
File name does not match.
> +static long tee_ioctl_cmd(struct tee_context *ctx,
> + struct tee_ioctl_cmd_data __user *ucmd)
> +{
> + long ret;
> + struct tee_ioctl_cmd_data cmd;
> + void __user *buf_ptr;
> +
> + ret = copy_from_user(&cmd, ucmd, sizeof(cmd));
> + if (ret)
> + return ret;
> +
> + buf_ptr = (void __user *)(uintptr_t)cmd.buf_ptr;
> + return ctx->teedev->desc->ops->cmd(ctx, buf_ptr, cmd.buf_len);
> +}
What is that double indirection for? Normally each command gets its
own data structure, and then you can handle each command in the common
abstraction.
> +static long tee_ioctl_mem_share(struct tee_context *ctx,
> + struct tee_ioctl_mem_share_data __user *udata)
> +{
> + /* Not supported yet */
> + return -ENOSYS;
> +}
> +
> +static long tee_ioctl_mem_unshare(struct tee_context *ctx,
> + struct tee_ioctl_mem_share_data __user *udata)
> +{
> + /* Not supported yet */
> + return -ENOSYS;
> +}
Why -ENOSYS? ioctl does exist ;-)
> +static const struct file_operations tee_fops = {
> + .owner = THIS_MODULE,
> + .open = tee_open,
> + .release = tee_release,
> + .unlocked_ioctl = tee_ioctl
> +};
Add a .compat_ioctl function, to make it work on arm64 as well.
If you got all the data structures right, you can use the same
tee_ioctl function.
Minor nit: put a comma behind the last line in each struct initialization
to make it easier to add another callback.
> +
> +static void tee_shm_release(struct tee_shm *shm);
Try to avoid forward declarations by reordering the code.
> +static struct sg_table *tee_shm_op_map_dma_buf(struct dma_buf_attachment
> + *attach, enum dma_data_direction dir)
> +{
> + return NULL;
> +}
> +
> +static void tee_shm_op_unmap_dma_buf(struct dma_buf_attachment *attach,
> + struct sg_table *table, enum dma_data_direction dir)
> +{
> +}
Since a lot of callbacks are empty here, I'd probably change the
caller to check for NULL pointer before calling these, and remove
the empty implementations, unless your next patch fills them with
content.
> +struct tee_shm *tee_shm_get_from_fd(int fd)
> +{
> + struct dma_buf *dmabuf = dma_buf_get(fd);
> +
> + if (IS_ERR(dmabuf))
> + return ERR_CAST(dmabuf);
> +
> + if (!is_shm_dma_buf(dmabuf)) {
> + dma_buf_put(dmabuf);
> + return ERR_PTR(-EINVAL);
> + }
> + return dmabuf->priv;
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_get_from_fd);
> +
> +void tee_shm_put(struct tee_shm *shm)
> +{
> + if (shm->flags & TEE_SHM_DMA_BUF)
> + dma_buf_put(shm->dmabuf);
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_put);
Can you explain why you picked dmabuf as the interface here? Normally this
is used when you have multiple DMA master devices access the same memory,
while my understanding of your use case is that you just have one other
piece of code running on the same CPU accessing this.
Do you need more than one such buffer per device? Could you perhaps just
implement mmap on the chardev as a lot of other drivers do?
> +struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
> + phys_addr_t *paddr, size_t *size)
I think it would be better not to have 'cma' as part of the function
name -- the driver really should not care at all.
What is the typical and maximum allocation size here?
> +++ b/include/linux/tee/tee.h
This belongs into include/uapi/linux/, because you are defining ioctl values
for user space.
> +#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)
> +#define _TEE_IOW(nr, size) _IOW(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr), size)
I would remove these macros and open-code the users of this as:
#define TEE_IOC_VERSION _IOR(TEE_IOC_MAGIC, TEE_IOC_BASE, struct tee_ioctl_version)
The reason is that a number of scripts try to scan the header files for
ioctl commands, which will fail if you add another indirection.
> +/*
> + * Version of the generic TEE subsystem, if it doesn't match what's
> + * returned by TEE_IOC_VERSION this header is not in sync with the kernel.
> + */
> +#define TEE_SUBSYS_VERSION 1
That does not work. When you introduce commands, you have to keep them working.
If you get an interface wrong, you can add another ioctl, but you can never
become incompatible. If a user applications wants to see if an interface is
supported, they can use a compile-time check. Building against a version 4.xyz
kernel header means that the code will not run on older kernels.
You can also copy the definition to user space and then do runtime checks
by trying out commands and falling back to something else.
> +
> +/* Flags relating to shared memory */
> +#define TEE_IOCTL_SHM_MAPPED 0x1 /* memory mapped in normal world */
> +#define TEE_IOCTL_SHM_DMA_BUF 0x2 /* dma-buf handle on shared memory */
> +
> +/**
> + * struct tee_version - TEE versions
> + * @gen_version: [out] Generic TEE driver version
> + * @spec_version: [out] Specific TEE driver version
> + * @uuid: [out] Specific TEE driver uuid, zero if not used
> + *
> + * Identifies the generic TEE driver, and the specific TEE driver.
> + * Used as argument for TEE_IOC_VERSION below.
> + */
> +struct tee_ioctl_version {
> + uint32_t gen_version;
> + uint32_t spec_version;
> + uint8_t uuid[16];
> +};
> +/**
> + * TEE_IOC_VERSION - query version of drivers
> + *
> + * Takes a tee_version struct and returns with the version numbers filled in.
> + */
> +#define TEE_IOC_VERSION _TEE_IOR(0, struct tee_ioctl_version)
Don't use uuids. You can probably just remove this entire command for
the reasons explained above.
> +/**
> + * struct tee_cmd_data - Opaque command argument
> + * @buf_ptr: [in] A __user pointer to a command buffer
> + * @buf_len: [in] 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.
> + * Used as argument for TEE_IOC_CMD below.
> + */
> +struct tee_ioctl_cmd_data {
> + uint64_t buf_ptr;
> + uint64_t buf_len;
> +};
Drop this one too. If someone wants commands that are not implemented
by the core, let them ask you to add them to the core, provided they are
useful and well-designed.
> + * 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.
> + * @fd: [out] dma_buf file descriptor of the shared memory
> + *
> + * The flags field should currently be zero as input. Updated by the call
> + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> + * This structure is used as argument for TEE_IOC_SHM_ALLOC below.
> + */
> +struct tee_ioctl_shm_alloc_data {
> + uint64_t size;
> + uint32_t flags;
> + int32_t fd;
> +};
> +/**
> + * 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_ioctl_shm_alloc_data)
See my questions above. Ideally we should be able to just use mmap here.
> +/**
> + * struct tee_mem_buf - share user space memory with Secure OS
> + * @ptr: A __user pointer to memory to share
> + * @size: Size of the memory to share
> + * Used in 'struct tee_mem_share_data' below.
> + */
> +struct tee_ioctl_mem_buf {
> + uint64_t ptr;
> + uint64_t size;
> +};
Why do you want both directions, exporting a kernel buffer to user
space, as well as using a user space buffer to give to the firmware?
If you can get by with just one of them, drop the other one.
> +/**
> + * struct tee_mem_dma_buf - share foreign dma_buf memory
> + * @fd: dma_buf file descriptor
> + * @pad: padding, set to zero by caller
> + * Used in 'struct tee_mem_share_data' below.
> + */
> +struct tee_ioctl_mem_dma_buf {
> + int32_t fd;
> + uint32_t pad;
> +};
What other driver do you have in mind that would provide the
file descriptor?
> +/**
> + * struct tee_mem_share_data - share memory with Secure OS
> + * @buf: [in] share user space memory
> + * @dma_buf: [in] share foreign dma_buf memory
> + * @flags: [in/out] Flags to/from sharing.
> + * @pad: [in/out] Padding, set to zero by caller
> + *
> + * The bits in @flags are defined by TEE_IOCTL_SHM_* above, undefined bits
> + * should be seto to zero as input. If TEE_IOCTL_SHM_DMA_BUF is set in the
> + * flags field use the dma_buf field, else the buf field in the union.
> + *
> + * Used as argument for TEE_IOC_MEM_SHARE and TEE_IOC_MEM_UNSHARE below.
> + */
> +struct tee_ioctl_mem_share_data {
> + union {
> + struct tee_ioctl_mem_buf buf;
> + struct tee_ioctl_mem_dma_buf dma_buf;
> + };
> + uint32_t flags;
> + uint32_t pad;
> +};
Better make that two distinct commands to avoid the union and the flags.
Arnd
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-17 20:07 ` Arnd Bergmann
0 siblings, 0 replies; 85+ messages in thread
From: Arnd Bergmann @ 2015-04-17 20:07 UTC (permalink / raw)
To: linux-arm-kernel
On Friday 17 April 2015 09:50:56 Jens Wiklander wrote:
> Documentation/ioctl/ioctl-number.txt | 1 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/tee/Kconfig | 8 +
> drivers/tee/Makefile | 3 +
> drivers/tee/tee.c | 253 +++++++++++++++++++++++++++
> drivers/tee/tee_private.h | 64 +++++++
> drivers/tee/tee_shm.c | 330 +++++++++++++++++++++++++++++++++++
> drivers/tee/tee_shm_pool.c | 246 ++++++++++++++++++++++++++
> include/linux/tee/tee.h | 180 +++++++++++++++++++
> include/linux/tee/tee_drv.h | 271 ++++++++++++++++++++++++++++
Hi Jens,
The driver looks very well implemented, but as you are introducing a new user
space API, we have to very carefully consider every aspect of that interface,
so I'm commenting mainly on user-visible parts.
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index 8136e1f..6e9bd04 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/sec-hw/tee.h Generic TEE subsystem
File name does not match.
> +static long tee_ioctl_cmd(struct tee_context *ctx,
> + struct tee_ioctl_cmd_data __user *ucmd)
> +{
> + long ret;
> + struct tee_ioctl_cmd_data cmd;
> + void __user *buf_ptr;
> +
> + ret = copy_from_user(&cmd, ucmd, sizeof(cmd));
> + if (ret)
> + return ret;
> +
> + buf_ptr = (void __user *)(uintptr_t)cmd.buf_ptr;
> + return ctx->teedev->desc->ops->cmd(ctx, buf_ptr, cmd.buf_len);
> +}
What is that double indirection for? Normally each command gets its
own data structure, and then you can handle each command in the common
abstraction.
> +static long tee_ioctl_mem_share(struct tee_context *ctx,
> + struct tee_ioctl_mem_share_data __user *udata)
> +{
> + /* Not supported yet */
> + return -ENOSYS;
> +}
> +
> +static long tee_ioctl_mem_unshare(struct tee_context *ctx,
> + struct tee_ioctl_mem_share_data __user *udata)
> +{
> + /* Not supported yet */
> + return -ENOSYS;
> +}
Why -ENOSYS? ioctl does exist ;-)
> +static const struct file_operations tee_fops = {
> + .owner = THIS_MODULE,
> + .open = tee_open,
> + .release = tee_release,
> + .unlocked_ioctl = tee_ioctl
> +};
Add a .compat_ioctl function, to make it work on arm64 as well.
If you got all the data structures right, you can use the same
tee_ioctl function.
Minor nit: put a comma behind the last line in each struct initialization
to make it easier to add another callback.
> +
> +static void tee_shm_release(struct tee_shm *shm);
Try to avoid forward declarations by reordering the code.
> +static struct sg_table *tee_shm_op_map_dma_buf(struct dma_buf_attachment
> + *attach, enum dma_data_direction dir)
> +{
> + return NULL;
> +}
> +
> +static void tee_shm_op_unmap_dma_buf(struct dma_buf_attachment *attach,
> + struct sg_table *table, enum dma_data_direction dir)
> +{
> +}
Since a lot of callbacks are empty here, I'd probably change the
caller to check for NULL pointer before calling these, and remove
the empty implementations, unless your next patch fills them with
content.
> +struct tee_shm *tee_shm_get_from_fd(int fd)
> +{
> + struct dma_buf *dmabuf = dma_buf_get(fd);
> +
> + if (IS_ERR(dmabuf))
> + return ERR_CAST(dmabuf);
> +
> + if (!is_shm_dma_buf(dmabuf)) {
> + dma_buf_put(dmabuf);
> + return ERR_PTR(-EINVAL);
> + }
> + return dmabuf->priv;
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_get_from_fd);
> +
> +void tee_shm_put(struct tee_shm *shm)
> +{
> + if (shm->flags & TEE_SHM_DMA_BUF)
> + dma_buf_put(shm->dmabuf);
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_put);
Can you explain why you picked dmabuf as the interface here? Normally this
is used when you have multiple DMA master devices access the same memory,
while my understanding of your use case is that you just have one other
piece of code running on the same CPU accessing this.
Do you need more than one such buffer per device? Could you perhaps just
implement mmap on the chardev as a lot of other drivers do?
> +struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
> + phys_addr_t *paddr, size_t *size)
I think it would be better not to have 'cma' as part of the function
name -- the driver really should not care at all.
What is the typical and maximum allocation size here?
> +++ b/include/linux/tee/tee.h
This belongs into include/uapi/linux/, because you are defining ioctl values
for user space.
> +#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)
> +#define _TEE_IOW(nr, size) _IOW(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr), size)
I would remove these macros and open-code the users of this as:
#define TEE_IOC_VERSION _IOR(TEE_IOC_MAGIC, TEE_IOC_BASE, struct tee_ioctl_version)
The reason is that a number of scripts try to scan the header files for
ioctl commands, which will fail if you add another indirection.
> +/*
> + * Version of the generic TEE subsystem, if it doesn't match what's
> + * returned by TEE_IOC_VERSION this header is not in sync with the kernel.
> + */
> +#define TEE_SUBSYS_VERSION 1
That does not work. When you introduce commands, you have to keep them working.
If you get an interface wrong, you can add another ioctl, but you can never
become incompatible. If a user applications wants to see if an interface is
supported, they can use a compile-time check. Building against a version 4.xyz
kernel header means that the code will not run on older kernels.
You can also copy the definition to user space and then do runtime checks
by trying out commands and falling back to something else.
> +
> +/* Flags relating to shared memory */
> +#define TEE_IOCTL_SHM_MAPPED 0x1 /* memory mapped in normal world */
> +#define TEE_IOCTL_SHM_DMA_BUF 0x2 /* dma-buf handle on shared memory */
> +
> +/**
> + * struct tee_version - TEE versions
> + * @gen_version: [out] Generic TEE driver version
> + * @spec_version: [out] Specific TEE driver version
> + * @uuid: [out] Specific TEE driver uuid, zero if not used
> + *
> + * Identifies the generic TEE driver, and the specific TEE driver.
> + * Used as argument for TEE_IOC_VERSION below.
> + */
> +struct tee_ioctl_version {
> + uint32_t gen_version;
> + uint32_t spec_version;
> + uint8_t uuid[16];
> +};
> +/**
> + * TEE_IOC_VERSION - query version of drivers
> + *
> + * Takes a tee_version struct and returns with the version numbers filled in.
> + */
> +#define TEE_IOC_VERSION _TEE_IOR(0, struct tee_ioctl_version)
Don't use uuids. You can probably just remove this entire command for
the reasons explained above.
> +/**
> + * struct tee_cmd_data - Opaque command argument
> + * @buf_ptr: [in] A __user pointer to a command buffer
> + * @buf_len: [in] 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.
> + * Used as argument for TEE_IOC_CMD below.
> + */
> +struct tee_ioctl_cmd_data {
> + uint64_t buf_ptr;
> + uint64_t buf_len;
> +};
Drop this one too. If someone wants commands that are not implemented
by the core, let them ask you to add them to the core, provided they are
useful and well-designed.
> + * 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.
> + * @fd: [out] dma_buf file descriptor of the shared memory
> + *
> + * The flags field should currently be zero as input. Updated by the call
> + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> + * This structure is used as argument for TEE_IOC_SHM_ALLOC below.
> + */
> +struct tee_ioctl_shm_alloc_data {
> + uint64_t size;
> + uint32_t flags;
> + int32_t fd;
> +};
> +/**
> + * 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_ioctl_shm_alloc_data)
See my questions above. Ideally we should be able to just use mmap here.
> +/**
> + * struct tee_mem_buf - share user space memory with Secure OS
> + * @ptr: A __user pointer to memory to share
> + * @size: Size of the memory to share
> + * Used in 'struct tee_mem_share_data' below.
> + */
> +struct tee_ioctl_mem_buf {
> + uint64_t ptr;
> + uint64_t size;
> +};
Why do you want both directions, exporting a kernel buffer to user
space, as well as using a user space buffer to give to the firmware?
If you can get by with just one of them, drop the other one.
> +/**
> + * struct tee_mem_dma_buf - share foreign dma_buf memory
> + * @fd: dma_buf file descriptor
> + * @pad: padding, set to zero by caller
> + * Used in 'struct tee_mem_share_data' below.
> + */
> +struct tee_ioctl_mem_dma_buf {
> + int32_t fd;
> + uint32_t pad;
> +};
What other driver do you have in mind that would provide the
file descriptor?
> +/**
> + * struct tee_mem_share_data - share memory with Secure OS
> + * @buf: [in] share user space memory
> + * @dma_buf: [in] share foreign dma_buf memory
> + * @flags: [in/out] Flags to/from sharing.
> + * @pad: [in/out] Padding, set to zero by caller
> + *
> + * The bits in @flags are defined by TEE_IOCTL_SHM_* above, undefined bits
> + * should be seto to zero as input. If TEE_IOCTL_SHM_DMA_BUF is set in the
> + * flags field use the dma_buf field, else the buf field in the union.
> + *
> + * Used as argument for TEE_IOC_MEM_SHARE and TEE_IOC_MEM_UNSHARE below.
> + */
> +struct tee_ioctl_mem_share_data {
> + union {
> + struct tee_ioctl_mem_buf buf;
> + struct tee_ioctl_mem_dma_buf dma_buf;
> + };
> + uint32_t flags;
> + uint32_t pad;
> +};
Better make that two distinct commands to avoid the union and the flags.
Arnd
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-17 20:07 ` Arnd Bergmann
(?)
@ 2015-04-18 7:20 ` Paul Bolle
-1 siblings, 0 replies; 85+ messages in thread
From: Paul Bolle @ 2015-04-18 7:20 UTC (permalink / raw)
To: Jens Wiklander
Cc: Arnd Bergmann, linux-kernel, linux-arm-kernel, devicetree,
Greg Kroah-Hartman, javier, Herbert Xu, tpmdd-devel,
valentin.manea, jean-michel.delorme, emmanuel.michel
On Fri, 2015-04-17 at 22:07 +0200, Arnd Bergmann wrote:
> On Friday 17 April 2015 09:50:56 Jens Wiklander wrote:
> > +static const struct file_operations tee_fops = {
> > + .owner = THIS_MODULE,
> > + .open = tee_open,
> > + .release = tee_release,
> > + .unlocked_ioctl = tee_ioctl
> > +};
>
> Add a .compat_ioctl function, to make it work on arm64 as well.
> If you got all the data structures right, you can use the same
> tee_ioctl function.
>
> Minor nit: put a comma behind the last line in each struct initialization
> to make it easier to add another callback.
And another nit: this is built-in only code, right? So, according to
include/linux/export.h, THIS_MODULE will be basically equivalent to
NULL. So I think you can drop that line.
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 7:20 ` Paul Bolle
0 siblings, 0 replies; 85+ messages in thread
From: Paul Bolle @ 2015-04-18 7:20 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 2015-04-17 at 22:07 +0200, Arnd Bergmann wrote:
> On Friday 17 April 2015 09:50:56 Jens Wiklander wrote:
> > +static const struct file_operations tee_fops = {
> > + .owner = THIS_MODULE,
> > + .open = tee_open,
> > + .release = tee_release,
> > + .unlocked_ioctl = tee_ioctl
> > +};
>
> Add a .compat_ioctl function, to make it work on arm64 as well.
> If you got all the data structures right, you can use the same
> tee_ioctl function.
>
> Minor nit: put a comma behind the last line in each struct initialization
> to make it easier to add another callback.
And another nit: this is built-in only code, right? So, according to
include/linux/export.h, THIS_MODULE will be basically equivalent to
NULL. So I think you can drop that line.
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 7:20 ` Paul Bolle
0 siblings, 0 replies; 85+ messages in thread
From: Paul Bolle @ 2015-04-18 7:20 UTC (permalink / raw)
To: Jens Wiklander
Cc: Arnd Bergmann, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman,
javier-5MUHepqpBA1BDgjK7y7TUQ, Herbert Xu,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
valentin.manea-hv44wF8Li93QT0dZR+AlfA,
jean-michel.delorme-qxv4g6HH51o, emmanuel.michel-qxv4g6HH51o
On Fri, 2015-04-17 at 22:07 +0200, Arnd Bergmann wrote:
> On Friday 17 April 2015 09:50:56 Jens Wiklander wrote:
> > +static const struct file_operations tee_fops = {
> > + .owner = THIS_MODULE,
> > + .open = tee_open,
> > + .release = tee_release,
> > + .unlocked_ioctl = tee_ioctl
> > +};
>
> Add a .compat_ioctl function, to make it work on arm64 as well.
> If you got all the data structures right, you can use the same
> tee_ioctl function.
>
> Minor nit: put a comma behind the last line in each struct initialization
> to make it easier to add another callback.
And another nit: this is built-in only code, right? So, according to
include/linux/export.h, THIS_MODULE will be basically equivalent to
NULL. So I think you can drop that line.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-17 20:07 ` Arnd Bergmann
@ 2015-04-20 6:20 ` Jens Wiklander
-1 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-20 6:20 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, linux-arm-kernel, devicetree, Greg Kroah-Hartman,
javier, Herbert Xu, tpmdd-devel, valentin.manea,
jean-michel.delorme, emmanuel.michel
Hi Arnd,
On Fri, Apr 17, 2015 at 10:07:02PM +0200, Arnd Bergmann wrote:
> On Friday 17 April 2015 09:50:56 Jens Wiklander wrote:
> > Documentation/ioctl/ioctl-number.txt | 1 +
> > drivers/Kconfig | 2 +
> > drivers/Makefile | 1 +
> > drivers/tee/Kconfig | 8 +
> > drivers/tee/Makefile | 3 +
> > drivers/tee/tee.c | 253 +++++++++++++++++++++++++++
> > drivers/tee/tee_private.h | 64 +++++++
> > drivers/tee/tee_shm.c | 330 +++++++++++++++++++++++++++++++++++
> > drivers/tee/tee_shm_pool.c | 246 ++++++++++++++++++++++++++
> > include/linux/tee/tee.h | 180 +++++++++++++++++++
> > include/linux/tee/tee_drv.h | 271 ++++++++++++++++++++++++++++
>
> Hi Jens,
>
> The driver looks very well implemented, but as you are introducing a new user
> space API, we have to very carefully consider every aspect of that interface,
> so I'm commenting mainly on user-visible parts.
>
> > diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> > index 8136e1f..6e9bd04 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/sec-hw/tee.h Generic TEE subsystem
>
> File name does not match.
Sorry, I'll fix.
>
> > +static long tee_ioctl_cmd(struct tee_context *ctx,
> > + struct tee_ioctl_cmd_data __user *ucmd)
> > +{
> > + long ret;
> > + struct tee_ioctl_cmd_data cmd;
> > + void __user *buf_ptr;
> > +
> > + ret = copy_from_user(&cmd, ucmd, sizeof(cmd));
> > + if (ret)
> > + return ret;
> > +
> > + buf_ptr = (void __user *)(uintptr_t)cmd.buf_ptr;
> > + return ctx->teedev->desc->ops->cmd(ctx, buf_ptr, cmd.buf_len);
> > +}
>
> What is that double indirection for? Normally each command gets its
> own data structure, and then you can handle each command in the common
> abstraction.
I'm not sure I understand what you mean. This function is a building
block for the TEE driver to supply whatever interface is needed for user
space. For a Global Platform like TEE it will typically have support for
TEEC_OpenSession(), TEEC_InvokeCommand(), TEEC_RequestCancellation() and
TEEC_CloseSession(). But how that's done is depending on how the
interface towards the TEE (in secure world) looks like. From what I've
heard so far those interfaces diverges a lot so we've compromised with
this function.
>
> > +static long tee_ioctl_mem_share(struct tee_context *ctx,
> > + struct tee_ioctl_mem_share_data __user *udata)
> > +{
> > + /* Not supported yet */
> > + return -ENOSYS;
> > +}
> > +
> > +static long tee_ioctl_mem_unshare(struct tee_context *ctx,
> > + struct tee_ioctl_mem_share_data __user *udata)
> > +{
> > + /* Not supported yet */
> > + return -ENOSYS;
> > +}
>
> Why -ENOSYS? ioctl does exist ;-)
You're right :-), is -EINVAL OK?
>
> > +static const struct file_operations tee_fops = {
> > + .owner = THIS_MODULE,
> > + .open = tee_open,
> > + .release = tee_release,
> > + .unlocked_ioctl = tee_ioctl
> > +};
>
> Add a .compat_ioctl function, to make it work on arm64 as well.
> If you got all the data structures right, you can use the same
> tee_ioctl function.
To handle the combination of kernel as arm64 and user space arm32?
Thanks, I'll fix.
>
> Minor nit: put a comma behind the last line in each struct initialization
> to make it easier to add another callback.
OK
>
> > +
> > +static void tee_shm_release(struct tee_shm *shm);
>
> Try to avoid forward declarations by reordering the code.
OK, will fix.
>
> > +static struct sg_table *tee_shm_op_map_dma_buf(struct dma_buf_attachment
> > + *attach, enum dma_data_direction dir)
> > +{
> > + return NULL;
> > +}
> > +
> > +static void tee_shm_op_unmap_dma_buf(struct dma_buf_attachment *attach,
> > + struct sg_table *table, enum dma_data_direction dir)
> > +{
> > +}
>
> Since a lot of callbacks are empty here, I'd probably change the
> caller to check for NULL pointer before calling these, and remove
> the empty implementations, unless your next patch fills them with
> content.
You mean I should update various functions in dma-buf.c? Dma-buf allows
having some functions as NULL but not these we had to provide here, I
don't know if there was a good reason for that or not.
>
> > +struct tee_shm *tee_shm_get_from_fd(int fd)
> > +{
> > + struct dma_buf *dmabuf = dma_buf_get(fd);
> > +
> > + if (IS_ERR(dmabuf))
> > + return ERR_CAST(dmabuf);
> > +
> > + if (!is_shm_dma_buf(dmabuf)) {
> > + dma_buf_put(dmabuf);
> > + return ERR_PTR(-EINVAL);
> > + }
> > + return dmabuf->priv;
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_get_from_fd);
> > +
> > +void tee_shm_put(struct tee_shm *shm)
> > +{
> > + if (shm->flags & TEE_SHM_DMA_BUF)
> > + dma_buf_put(shm->dmabuf);
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_put);
>
> Can you explain why you picked dmabuf as the interface here? Normally this
> is used when you have multiple DMA master devices access the same memory,
> while my understanding of your use case is that you just have one other
> piece of code running on the same CPU accessing this.
It could be any of the available CPUs in the system that will be
accessing this, both in secure and normal world. When we're doing a
secure video path use case some of the buffers will not be mapped in
normal world, but normal world will still keep track of the physical
addresses.
>
> Do you need more than one such buffer per device? Could you perhaps just
> implement mmap on the chardev as a lot of other drivers do?
Each buffer allocation corresponds to TEEC_AllocateSharedMemory() in Global
Platforms TEE Client API which we implement in user space. The number of
needed buffer depends on how the user space application is designed.
>
> > +struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
> > + phys_addr_t *paddr, size_t *size)
>
> I think it would be better not to have 'cma' as part of the function
> name -- the driver really should not care at all.
OK, I'll drop the _cma suffix.
>
> What is the typical and maximum allocation size here?
It depends on the design of the Trusted Application in secure world and
the client in user space. A few KiB could be the typical allocation
size, with a maximum at perhaps 512 KiB (for instance when loading a
very large Trusted Application).
>
> > +++ b/include/linux/tee/tee.h
>
> This belongs into include/uapi/linux/, because you are defining ioctl values
> for user space.
Thanks, I'll fix.
>
> > +#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)
> > +#define _TEE_IOW(nr, size) _IOW(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr), size)
>
> I would remove these macros and open-code the users of this as:
>
> #define TEE_IOC_VERSION _IOR(TEE_IOC_MAGIC, TEE_IOC_BASE, struct tee_ioctl_version)
>
> The reason is that a number of scripts try to scan the header files for
> ioctl commands, which will fail if you add another indirection.
Thanks, I'll fix.
>
> > +/*
> > + * Version of the generic TEE subsystem, if it doesn't match what's
> > + * returned by TEE_IOC_VERSION this header is not in sync with the kernel.
> > + */
> > +#define TEE_SUBSYS_VERSION 1
>
> That does not work. When you introduce commands, you have to keep them working.
> If you get an interface wrong, you can add another ioctl, but you can never
> become incompatible. If a user applications wants to see if an interface is
> supported, they can use a compile-time check. Building against a version 4.xyz
> kernel header means that the code will not run on older kernels.
>
> You can also copy the definition to user space and then do runtime checks
> by trying out commands and falling back to something else.
OK, I'll drop this.
>
> > +
> > +/* Flags relating to shared memory */
> > +#define TEE_IOCTL_SHM_MAPPED 0x1 /* memory mapped in normal world */
> > +#define TEE_IOCTL_SHM_DMA_BUF 0x2 /* dma-buf handle on shared memory */
> > +
> > +/**
> > + * struct tee_version - TEE versions
> > + * @gen_version: [out] Generic TEE driver version
> > + * @spec_version: [out] Specific TEE driver version
> > + * @uuid: [out] Specific TEE driver uuid, zero if not used
> > + *
> > + * Identifies the generic TEE driver, and the specific TEE driver.
> > + * Used as argument for TEE_IOC_VERSION below.
> > + */
> > +struct tee_ioctl_version {
> > + uint32_t gen_version;
> > + uint32_t spec_version;
> > + uint8_t uuid[16];
> > +};
> > +/**
> > + * TEE_IOC_VERSION - query version of drivers
> > + *
> > + * Takes a tee_version struct and returns with the version numbers filled in.
> > + */
> > +#define TEE_IOC_VERSION _TEE_IOR(0, struct tee_ioctl_version)
>
> Don't use uuids. You can probably just remove this entire command for
> the reasons explained above.
I agree that we can drop least one of the _version fields probably both,
but something is needed for user space to be able to know which TEE (in
secure world) it's communicating with. The uuid will let the client know
how how to format the commands passed to TEE_IOC_CMD below.
>
> > +/**
> > + * struct tee_cmd_data - Opaque command argument
> > + * @buf_ptr: [in] A __user pointer to a command buffer
> > + * @buf_len: [in] 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.
> > + * Used as argument for TEE_IOC_CMD below.
> > + */
> > +struct tee_ioctl_cmd_data {
> > + uint64_t buf_ptr;
> > + uint64_t buf_len;
> > +};
>
> Drop this one too. If someone wants commands that are not implemented
> by the core, let them ask you to add them to the core, provided they are
> useful and well-designed.
I've touched on this above, this function the essential when
communicating with the driver and secure world. Different TEEs (running
in some secure environment) provides different interfaces. By providing
an opaque channel we don't have to force something on the TEE. The
problem is moved to the user space library which is used when talking to
the TEE. The assumption here is that the interface provided the TEE is
stable or something that the specific TEE driver can handle with a glue
layer.
When the client opens /dev/teeX it will check that if it's a known TEE,
or it will close the device and try next and fail when there's no more
to try.
>
> > + * 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.
> > + * @fd: [out] dma_buf file descriptor of the shared memory
> > + *
> > + * The flags field should currently be zero as input. Updated by the call
> > + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> > + * This structure is used as argument for TEE_IOC_SHM_ALLOC below.
> > + */
> > +struct tee_ioctl_shm_alloc_data {
> > + uint64_t size;
> > + uint32_t flags;
> > + int32_t fd;
> > +};
> > +/**
> > + * 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_ioctl_shm_alloc_data)
>
> See my questions above. Ideally we should be able to just use mmap here.
We're are using mmap, but on the file descriptor returned here. If we
had only one file descriptor it would be hard for the subsystem or the
driver to tell if user space is allowed to map this particular memory
range.
>
> > +/**
> > + * struct tee_mem_buf - share user space memory with Secure OS
> > + * @ptr: A __user pointer to memory to share
> > + * @size: Size of the memory to share
> > + * Used in 'struct tee_mem_share_data' below.
> > + */
> > +struct tee_ioctl_mem_buf {
> > + uint64_t ptr;
> > + uint64_t size;
> > +};
>
> Why do you want both directions, exporting a kernel buffer to user
> space, as well as using a user space buffer to give to the firmware?
>
> If you can get by with just one of them, drop the other one.
We need to be able to export kernel buffers to user space as some secure
worlds has restrictions on which memory ranges can be used for shared
memory. The other way around (exporting user memory to secure world) is
an optimization (which we haven't implemented) to be able to avoid
double buffering when implementing TEEC_RegisterSharedMemory() in Global
Platforms TEE Client API in user space.
>
> > +/**
> > + * struct tee_mem_dma_buf - share foreign dma_buf memory
> > + * @fd: dma_buf file descriptor
> > + * @pad: padding, set to zero by caller
> > + * Used in 'struct tee_mem_share_data' below.
> > + */
> > +struct tee_ioctl_mem_dma_buf {
> > + int32_t fd;
> > + uint32_t pad;
> > +};
>
> What other driver do you have in mind that would provide the
> file descriptor?
This is primary to support secure video path. The file descriptor would
come from some driver handling hardware for decoding or rendering from
one secure buffer to another.
I'll drop this until we have code that need it.
>
> > +/**
> > + * struct tee_mem_share_data - share memory with Secure OS
> > + * @buf: [in] share user space memory
> > + * @dma_buf: [in] share foreign dma_buf memory
> > + * @flags: [in/out] Flags to/from sharing.
> > + * @pad: [in/out] Padding, set to zero by caller
> > + *
> > + * The bits in @flags are defined by TEE_IOCTL_SHM_* above, undefined bits
> > + * should be seto to zero as input. If TEE_IOCTL_SHM_DMA_BUF is set in the
> > + * flags field use the dma_buf field, else the buf field in the union.
> > + *
> > + * Used as argument for TEE_IOC_MEM_SHARE and TEE_IOC_MEM_UNSHARE below.
> > + */
> > +struct tee_ioctl_mem_share_data {
> > + union {
> > + struct tee_ioctl_mem_buf buf;
> > + struct tee_ioctl_mem_dma_buf dma_buf;
> > + };
> > + uint32_t flags;
> > + uint32_t pad;
> > +};
>
> Better make that two distinct commands to avoid the union and the flags.
OK, makes sense. This is currently not used by OP-TEE. As was suggested
elsewhere I'll drop the parts of the API which aren't currently needed
by OP-TEE.
Regards,
Jens
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 6:20 ` Jens Wiklander
0 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-20 6:20 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
On Fri, Apr 17, 2015 at 10:07:02PM +0200, Arnd Bergmann wrote:
> On Friday 17 April 2015 09:50:56 Jens Wiklander wrote:
> > Documentation/ioctl/ioctl-number.txt | 1 +
> > drivers/Kconfig | 2 +
> > drivers/Makefile | 1 +
> > drivers/tee/Kconfig | 8 +
> > drivers/tee/Makefile | 3 +
> > drivers/tee/tee.c | 253 +++++++++++++++++++++++++++
> > drivers/tee/tee_private.h | 64 +++++++
> > drivers/tee/tee_shm.c | 330 +++++++++++++++++++++++++++++++++++
> > drivers/tee/tee_shm_pool.c | 246 ++++++++++++++++++++++++++
> > include/linux/tee/tee.h | 180 +++++++++++++++++++
> > include/linux/tee/tee_drv.h | 271 ++++++++++++++++++++++++++++
>
> Hi Jens,
>
> The driver looks very well implemented, but as you are introducing a new user
> space API, we have to very carefully consider every aspect of that interface,
> so I'm commenting mainly on user-visible parts.
>
> > diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> > index 8136e1f..6e9bd04 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/sec-hw/tee.h Generic TEE subsystem
>
> File name does not match.
Sorry, I'll fix.
>
> > +static long tee_ioctl_cmd(struct tee_context *ctx,
> > + struct tee_ioctl_cmd_data __user *ucmd)
> > +{
> > + long ret;
> > + struct tee_ioctl_cmd_data cmd;
> > + void __user *buf_ptr;
> > +
> > + ret = copy_from_user(&cmd, ucmd, sizeof(cmd));
> > + if (ret)
> > + return ret;
> > +
> > + buf_ptr = (void __user *)(uintptr_t)cmd.buf_ptr;
> > + return ctx->teedev->desc->ops->cmd(ctx, buf_ptr, cmd.buf_len);
> > +}
>
> What is that double indirection for? Normally each command gets its
> own data structure, and then you can handle each command in the common
> abstraction.
I'm not sure I understand what you mean. This function is a building
block for the TEE driver to supply whatever interface is needed for user
space. For a Global Platform like TEE it will typically have support for
TEEC_OpenSession(), TEEC_InvokeCommand(), TEEC_RequestCancellation() and
TEEC_CloseSession(). But how that's done is depending on how the
interface towards the TEE (in secure world) looks like. From what I've
heard so far those interfaces diverges a lot so we've compromised with
this function.
>
> > +static long tee_ioctl_mem_share(struct tee_context *ctx,
> > + struct tee_ioctl_mem_share_data __user *udata)
> > +{
> > + /* Not supported yet */
> > + return -ENOSYS;
> > +}
> > +
> > +static long tee_ioctl_mem_unshare(struct tee_context *ctx,
> > + struct tee_ioctl_mem_share_data __user *udata)
> > +{
> > + /* Not supported yet */
> > + return -ENOSYS;
> > +}
>
> Why -ENOSYS? ioctl does exist ;-)
You're right :-), is -EINVAL OK?
>
> > +static const struct file_operations tee_fops = {
> > + .owner = THIS_MODULE,
> > + .open = tee_open,
> > + .release = tee_release,
> > + .unlocked_ioctl = tee_ioctl
> > +};
>
> Add a .compat_ioctl function, to make it work on arm64 as well.
> If you got all the data structures right, you can use the same
> tee_ioctl function.
To handle the combination of kernel as arm64 and user space arm32?
Thanks, I'll fix.
>
> Minor nit: put a comma behind the last line in each struct initialization
> to make it easier to add another callback.
OK
>
> > +
> > +static void tee_shm_release(struct tee_shm *shm);
>
> Try to avoid forward declarations by reordering the code.
OK, will fix.
>
> > +static struct sg_table *tee_shm_op_map_dma_buf(struct dma_buf_attachment
> > + *attach, enum dma_data_direction dir)
> > +{
> > + return NULL;
> > +}
> > +
> > +static void tee_shm_op_unmap_dma_buf(struct dma_buf_attachment *attach,
> > + struct sg_table *table, enum dma_data_direction dir)
> > +{
> > +}
>
> Since a lot of callbacks are empty here, I'd probably change the
> caller to check for NULL pointer before calling these, and remove
> the empty implementations, unless your next patch fills them with
> content.
You mean I should update various functions in dma-buf.c? Dma-buf allows
having some functions as NULL but not these we had to provide here, I
don't know if there was a good reason for that or not.
>
> > +struct tee_shm *tee_shm_get_from_fd(int fd)
> > +{
> > + struct dma_buf *dmabuf = dma_buf_get(fd);
> > +
> > + if (IS_ERR(dmabuf))
> > + return ERR_CAST(dmabuf);
> > +
> > + if (!is_shm_dma_buf(dmabuf)) {
> > + dma_buf_put(dmabuf);
> > + return ERR_PTR(-EINVAL);
> > + }
> > + return dmabuf->priv;
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_get_from_fd);
> > +
> > +void tee_shm_put(struct tee_shm *shm)
> > +{
> > + if (shm->flags & TEE_SHM_DMA_BUF)
> > + dma_buf_put(shm->dmabuf);
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_put);
>
> Can you explain why you picked dmabuf as the interface here? Normally this
> is used when you have multiple DMA master devices access the same memory,
> while my understanding of your use case is that you just have one other
> piece of code running on the same CPU accessing this.
It could be any of the available CPUs in the system that will be
accessing this, both in secure and normal world. When we're doing a
secure video path use case some of the buffers will not be mapped in
normal world, but normal world will still keep track of the physical
addresses.
>
> Do you need more than one such buffer per device? Could you perhaps just
> implement mmap on the chardev as a lot of other drivers do?
Each buffer allocation corresponds to TEEC_AllocateSharedMemory() in Global
Platforms TEE Client API which we implement in user space. The number of
needed buffer depends on how the user space application is designed.
>
> > +struct tee_shm_pool *tee_shm_pool_alloc_cma(struct device *dev, u_long *vaddr,
> > + phys_addr_t *paddr, size_t *size)
>
> I think it would be better not to have 'cma' as part of the function
> name -- the driver really should not care at all.
OK, I'll drop the _cma suffix.
>
> What is the typical and maximum allocation size here?
It depends on the design of the Trusted Application in secure world and
the client in user space. A few KiB could be the typical allocation
size, with a maximum at perhaps 512 KiB (for instance when loading a
very large Trusted Application).
>
> > +++ b/include/linux/tee/tee.h
>
> This belongs into include/uapi/linux/, because you are defining ioctl values
> for user space.
Thanks, I'll fix.
>
> > +#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)
> > +#define _TEE_IOW(nr, size) _IOW(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr), size)
>
> I would remove these macros and open-code the users of this as:
>
> #define TEE_IOC_VERSION _IOR(TEE_IOC_MAGIC, TEE_IOC_BASE, struct tee_ioctl_version)
>
> The reason is that a number of scripts try to scan the header files for
> ioctl commands, which will fail if you add another indirection.
Thanks, I'll fix.
>
> > +/*
> > + * Version of the generic TEE subsystem, if it doesn't match what's
> > + * returned by TEE_IOC_VERSION this header is not in sync with the kernel.
> > + */
> > +#define TEE_SUBSYS_VERSION 1
>
> That does not work. When you introduce commands, you have to keep them working.
> If you get an interface wrong, you can add another ioctl, but you can never
> become incompatible. If a user applications wants to see if an interface is
> supported, they can use a compile-time check. Building against a version 4.xyz
> kernel header means that the code will not run on older kernels.
>
> You can also copy the definition to user space and then do runtime checks
> by trying out commands and falling back to something else.
OK, I'll drop this.
>
> > +
> > +/* Flags relating to shared memory */
> > +#define TEE_IOCTL_SHM_MAPPED 0x1 /* memory mapped in normal world */
> > +#define TEE_IOCTL_SHM_DMA_BUF 0x2 /* dma-buf handle on shared memory */
> > +
> > +/**
> > + * struct tee_version - TEE versions
> > + * @gen_version: [out] Generic TEE driver version
> > + * @spec_version: [out] Specific TEE driver version
> > + * @uuid: [out] Specific TEE driver uuid, zero if not used
> > + *
> > + * Identifies the generic TEE driver, and the specific TEE driver.
> > + * Used as argument for TEE_IOC_VERSION below.
> > + */
> > +struct tee_ioctl_version {
> > + uint32_t gen_version;
> > + uint32_t spec_version;
> > + uint8_t uuid[16];
> > +};
> > +/**
> > + * TEE_IOC_VERSION - query version of drivers
> > + *
> > + * Takes a tee_version struct and returns with the version numbers filled in.
> > + */
> > +#define TEE_IOC_VERSION _TEE_IOR(0, struct tee_ioctl_version)
>
> Don't use uuids. You can probably just remove this entire command for
> the reasons explained above.
I agree that we can drop least one of the _version fields probably both,
but something is needed for user space to be able to know which TEE (in
secure world) it's communicating with. The uuid will let the client know
how how to format the commands passed to TEE_IOC_CMD below.
>
> > +/**
> > + * struct tee_cmd_data - Opaque command argument
> > + * @buf_ptr: [in] A __user pointer to a command buffer
> > + * @buf_len: [in] 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.
> > + * Used as argument for TEE_IOC_CMD below.
> > + */
> > +struct tee_ioctl_cmd_data {
> > + uint64_t buf_ptr;
> > + uint64_t buf_len;
> > +};
>
> Drop this one too. If someone wants commands that are not implemented
> by the core, let them ask you to add them to the core, provided they are
> useful and well-designed.
I've touched on this above, this function the essential when
communicating with the driver and secure world. Different TEEs (running
in some secure environment) provides different interfaces. By providing
an opaque channel we don't have to force something on the TEE. The
problem is moved to the user space library which is used when talking to
the TEE. The assumption here is that the interface provided the TEE is
stable or something that the specific TEE driver can handle with a glue
layer.
When the client opens /dev/teeX it will check that if it's a known TEE,
or it will close the device and try next and fail when there's no more
to try.
>
> > + * 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.
> > + * @fd: [out] dma_buf file descriptor of the shared memory
> > + *
> > + * The flags field should currently be zero as input. Updated by the call
> > + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> > + * This structure is used as argument for TEE_IOC_SHM_ALLOC below.
> > + */
> > +struct tee_ioctl_shm_alloc_data {
> > + uint64_t size;
> > + uint32_t flags;
> > + int32_t fd;
> > +};
> > +/**
> > + * 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_ioctl_shm_alloc_data)
>
> See my questions above. Ideally we should be able to just use mmap here.
We're are using mmap, but on the file descriptor returned here. If we
had only one file descriptor it would be hard for the subsystem or the
driver to tell if user space is allowed to map this particular memory
range.
>
> > +/**
> > + * struct tee_mem_buf - share user space memory with Secure OS
> > + * @ptr: A __user pointer to memory to share
> > + * @size: Size of the memory to share
> > + * Used in 'struct tee_mem_share_data' below.
> > + */
> > +struct tee_ioctl_mem_buf {
> > + uint64_t ptr;
> > + uint64_t size;
> > +};
>
> Why do you want both directions, exporting a kernel buffer to user
> space, as well as using a user space buffer to give to the firmware?
>
> If you can get by with just one of them, drop the other one.
We need to be able to export kernel buffers to user space as some secure
worlds has restrictions on which memory ranges can be used for shared
memory. The other way around (exporting user memory to secure world) is
an optimization (which we haven't implemented) to be able to avoid
double buffering when implementing TEEC_RegisterSharedMemory() in Global
Platforms TEE Client API in user space.
>
> > +/**
> > + * struct tee_mem_dma_buf - share foreign dma_buf memory
> > + * @fd: dma_buf file descriptor
> > + * @pad: padding, set to zero by caller
> > + * Used in 'struct tee_mem_share_data' below.
> > + */
> > +struct tee_ioctl_mem_dma_buf {
> > + int32_t fd;
> > + uint32_t pad;
> > +};
>
> What other driver do you have in mind that would provide the
> file descriptor?
This is primary to support secure video path. The file descriptor would
come from some driver handling hardware for decoding or rendering from
one secure buffer to another.
I'll drop this until we have code that need it.
>
> > +/**
> > + * struct tee_mem_share_data - share memory with Secure OS
> > + * @buf: [in] share user space memory
> > + * @dma_buf: [in] share foreign dma_buf memory
> > + * @flags: [in/out] Flags to/from sharing.
> > + * @pad: [in/out] Padding, set to zero by caller
> > + *
> > + * The bits in @flags are defined by TEE_IOCTL_SHM_* above, undefined bits
> > + * should be seto to zero as input. If TEE_IOCTL_SHM_DMA_BUF is set in the
> > + * flags field use the dma_buf field, else the buf field in the union.
> > + *
> > + * Used as argument for TEE_IOC_MEM_SHARE and TEE_IOC_MEM_UNSHARE below.
> > + */
> > +struct tee_ioctl_mem_share_data {
> > + union {
> > + struct tee_ioctl_mem_buf buf;
> > + struct tee_ioctl_mem_dma_buf dma_buf;
> > + };
> > + uint32_t flags;
> > + uint32_t pad;
> > +};
>
> Better make that two distinct commands to avoid the union and the flags.
OK, makes sense. This is currently not used by OP-TEE. As was suggested
elsewhere I'll drop the parts of the API which aren't currently needed
by OP-TEE.
Regards,
Jens
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-20 6:20 ` Jens Wiklander
@ 2015-04-20 18:20 ` Jason Gunthorpe
-1 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 18:20 UTC (permalink / raw)
To: Jens Wiklander
Cc: Arnd Bergmann, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Greg Kroah-Hartman, linux-kernel,
jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Mon, Apr 20, 2015 at 08:20:44AM +0200, Jens Wiklander wrote:
> I'm not sure I understand what you mean. This function is a building
> block for the TEE driver to supply whatever interface is needed for user
> space. For a Global Platform like TEE it will typically have support for
> TEEC_OpenSession(), TEEC_InvokeCommand(), TEEC_RequestCancellation() and
> TEEC_CloseSession(). But how that's done is depending on how the
> interface towards the TEE (in secure world) looks like. From what I've
> heard so far those interfaces diverges a lot so we've compromised with
> this function.
The goal of the mid layer is to bring all these differences into a
common abstraction, not punt on them to higher layers.
The goal if the driver is to translate and transport the common
abstraction to the hardware.
It is an absolute failure if each TEE driver implements a different
TEEC_OpenSession() ioctl. They must be the same, the common code
must de-marshal the request from user space and then call
ops->open_session()
Driver specific ioctls are a terrible way to start a new mid layer.
> > What is the typical and maximum allocation size here?
> It depends on the design of the Trusted Application in secure world and
> the client in user space. A few KiB could be the typical allocation
> size, with a maximum at perhaps 512 KiB (for instance when loading a
> very large Trusted Application).
So this TEE stuff also encompasses a 'firmware' loader (to the secure
world, presumably)?
That is probably your base level of 'ops' functionality, plus the
shared memroy stuff.
How does this work if two userspace things run concurrently with
different firmwares? Is there some locking or something? What is the
lifetime of this firmware tied to?
> I agree that we can drop least one of the _version fields probably both,
> but something is needed for user space to be able to know which TEE (in
> secure world) it's communicating with. The uuid will let the client know
> how how to format the commands passed to TEE_IOC_CMD below.
So you load the firmare, learn what command set it supports then use
TEE_IOC_CMD to shuttle firmware-specific data to and from?
> I've touched on this above, this function the essential when
> communicating with the driver and secure world. Different TEEs (running
> in some secure environment) provides different interfaces. By providing
> an opaque channel we don't have to force something on the TEE. The
> problem is moved to the user space library which is used when talking to
> the TEE. The assumption here is that the interface provided the TEE is
> stable or something that the specific TEE driver can handle with a glue
> layer.
I would use read/write for this, not ioctl. read/write can work with
select/poll so you can send your command then go into a polling loop
waiting for the reply from the firmware.
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread
* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-20 18:20 ` Jason Gunthorpe
0 siblings, 0 replies; 85+ messages in thread
From: Jason Gunthorpe @ 2015-04-20 18:20 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 20, 2015 at 08:20:44AM +0200, Jens Wiklander wrote:
> I'm not sure I understand what you mean. This function is a building
> block for the TEE driver to supply whatever interface is needed for user
> space. For a Global Platform like TEE it will typically have support for
> TEEC_OpenSession(), TEEC_InvokeCommand(), TEEC_RequestCancellation() and
> TEEC_CloseSession(). But how that's done is depending on how the
> interface towards the TEE (in secure world) looks like. From what I've
> heard so far those interfaces diverges a lot so we've compromised with
> this function.
The goal of the mid layer is to bring all these differences into a
common abstraction, not punt on them to higher layers.
The goal if the driver is to translate and transport the common
abstraction to the hardware.
It is an absolute failure if each TEE driver implements a different
TEEC_OpenSession() ioctl. They must be the same, the common code
must de-marshal the request from user space and then call
ops->open_session()
Driver specific ioctls are a terrible way to start a new mid layer.
> > What is the typical and maximum allocation size here?
> It depends on the design of the Trusted Application in secure world and
> the client in user space. A few KiB could be the typical allocation
> size, with a maximum at perhaps 512 KiB (for instance when loading a
> very large Trusted Application).
So this TEE stuff also encompasses a 'firmware' loader (to the secure
world, presumably)?
That is probably your base level of 'ops' functionality, plus the
shared memroy stuff.
How does this work if two userspace things run concurrently with
different firmwares? Is there some locking or something? What is the
lifetime of this firmware tied to?
> I agree that we can drop least one of the _version fields probably both,
> but something is needed for user space to be able to know which TEE (in
> secure world) it's communicating with. The uuid will let the client know
> how how to format the commands passed to TEE_IOC_CMD below.
So you load the firmare, learn what command set it supports then use
TEE_IOC_CMD to shuttle firmware-specific data to and from?
> I've touched on this above, this function the essential when
> communicating with the driver and secure world. Different TEEs (running
> in some secure environment) provides different interfaces. By providing
> an opaque channel we don't have to force something on the TEE. The
> problem is moved to the user space library which is used when talking to
> the TEE. The assumption here is that the interface provided the TEE is
> stable or something that the specific TEE driver can handle with a glue
> layer.
I would use read/write for this, not ioctl. read/write can work with
select/poll so you can send your command then go into a polling loop
waiting for the reply from the firmware.
Jason
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-20 18:20 ` Jason Gunthorpe
@ 2015-04-21 10:45 ` Jens Wiklander
-1 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-21 10:45 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Arnd Bergmann, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Greg Kroah-Hartman, linux-kernel,
jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Mon, Apr 20, 2015 at 12:20:52PM -0600, Jason Gunthorpe wrote:
> On Mon, Apr 20, 2015 at 08:20:44AM +0200, Jens Wiklander wrote:
>
> > I'm not sure I understand what you mean. This function is a building
> > block for the TEE driver to supply whatever interface is needed for user
> > space. For a Global Platform like TEE it will typically have support for
> > TEEC_OpenSession(), TEEC_InvokeCommand(), TEEC_RequestCancellation() and
> > TEEC_CloseSession(). But how that's done is depending on how the
> > interface towards the TEE (in secure world) looks like. From what I've
> > heard so far those interfaces diverges a lot so we've compromised with
> > this function.
>
> The goal of the mid layer is to bring all these differences into a
> common abstraction, not punt on them to higher layers.
>
> The goal if the driver is to translate and transport the common
> abstraction to the hardware.
>
> It is an absolute failure if each TEE driver implements a different
> TEEC_OpenSession() ioctl. They must be the same, the common code
> must de-marshal the request from user space and then call
> ops->open_session()
>
> Driver specific ioctls are a terrible way to start a new mid layer.
The example I gave above concerns Global Platform like TEEs, we're
trying to cover TEEs that doesn't follow Global Platform also here. Most
(or at least a significant part) deployed TEEs, in terms of volume,
today are not Global Platform compliant.
I'd like to view TEE_IOC_CMD as communication channel directly into the
TEE. What goes inside the channel is not something that this subsystem
cares about. The kernel driver will likely need to translate memory
references from user space inside this channel to something usable by
secure world (and vice versa), but apart from that it does as little as
possible except delivering messages.
There's no single definition of what interfaces a TEE has to normal
world. As soon as we try to define a common interface we're bound to
either miss a function completely or just define something that can't be
used by some TEE.
Global Platform has "TEE Client specification" and "TEE Internal Core
API Specification", but neither says anything about what happens between
the lower layers in user space and the TEE.
>
> > > What is the typical and maximum allocation size here?
> > It depends on the design of the Trusted Application in secure world and
> > the client in user space. A few KiB could be the typical allocation
> > size, with a maximum at perhaps 512 KiB (for instance when loading a
> > very large Trusted Application).
>
> So this TEE stuff also encompasses a 'firmware' loader (to the secure
> world, presumably)?
Yes, but that's driver specific. Some TEEs don't have this and the rest
does it in their own way. Global Platform doesn't say anything at all
about this.
>
> That is probably your base level of 'ops' functionality, plus the
> shared memroy stuff.
>
> How does this work if two userspace things run concurrently with
> different firmwares? Is there some locking or something? What is the
> lifetime of this firmware tied to?
In secure world there's a trusted OS (the TEE) with possibly embedded
Trusted Applications (TAs). The TEE may support loading TAs dynamically.
In the OP-TEE case when running on ARM with TrustZone it works like this:
1. The TEE is already loaded when kernel boots.
2. A tee_context is created when user space opens /dev/teeX, this
context holds the sessions.
3. TAs are loaded when needed when a session to the TA is opened.
4. When the context is closed all sessions that are still open are
closed.
5. What happens with the TAs when sessions are closed is internal to the
TEE. The TAs are stored in protected memory which the kernel can't
access any way.
OP-TEE is currently single threaded in the sense that only one thread
can be active in secure world at a time. We have some synchronization
around the SMC that enters secure world, but that's implementation
specific and only there to avoid unnecessary ping pong or spinlock in
secure world.
>
> > I agree that we can drop least one of the _version fields probably both,
> > but something is needed for user space to be able to know which TEE (in
> > secure world) it's communicating with. The uuid will let the client know
> > how how to format the commands passed to TEE_IOC_CMD below.
>
> So you load the firmare, learn what command set it supports then use
> TEE_IOC_CMD to shuttle firmware-specific data to and from?
Correct, except that the firmware (the TEE) is in most cases already
loaded at this stage.
>
> > I've touched on this above, this function the essential when
> > communicating with the driver and secure world. Different TEEs (running
> > in some secure environment) provides different interfaces. By providing
> > an opaque channel we don't have to force something on the TEE. The
> > problem is moved to the user space library which is used when talking to
> > the TEE. The assumption here is that the interface provided the TEE is
> > stable or something that the specific TEE driver can handle with a glue
> > layer.
>
> I would use read/write for this, not ioctl. read/write can work with
> select/poll so you can send your command then go into a polling loop
> waiting for the reply from the firmware.
There's two reasons I'm using ioctl instead.
On ARM with TrustZone (common case right now) the TEE is executing on
the same CPUs as the kernel (but with the ns-bit cleared instead of
set). Delivering a message/request is done with an SMC, you can compare
it with a syscall in user space. OP-TEE doesn't have a scheduler instead
we run on the time of the user space process doing the request. If we
where doing read/write/poll in which of the syscalls would we do the
SMC?
We have to be able to run several commands in parallel. How do we
connect the different reads and writes? A separate file descriptor would
do it, but we would need more than one for each session.
Regards,
Jens
^ permalink raw reply [flat|nested] 85+ messages in thread
* [tpmdd-devel] [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-21 10:45 ` Jens Wiklander
0 siblings, 0 replies; 85+ messages in thread
From: Jens Wiklander @ 2015-04-21 10:45 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 20, 2015 at 12:20:52PM -0600, Jason Gunthorpe wrote:
> On Mon, Apr 20, 2015 at 08:20:44AM +0200, Jens Wiklander wrote:
>
> > I'm not sure I understand what you mean. This function is a building
> > block for the TEE driver to supply whatever interface is needed for user
> > space. For a Global Platform like TEE it will typically have support for
> > TEEC_OpenSession(), TEEC_InvokeCommand(), TEEC_RequestCancellation() and
> > TEEC_CloseSession(). But how that's done is depending on how the
> > interface towards the TEE (in secure world) looks like. From what I've
> > heard so far those interfaces diverges a lot so we've compromised with
> > this function.
>
> The goal of the mid layer is to bring all these differences into a
> common abstraction, not punt on them to higher layers.
>
> The goal if the driver is to translate and transport the common
> abstraction to the hardware.
>
> It is an absolute failure if each TEE driver implements a different
> TEEC_OpenSession() ioctl. They must be the same, the common code
> must de-marshal the request from user space and then call
> ops->open_session()
>
> Driver specific ioctls are a terrible way to start a new mid layer.
The example I gave above concerns Global Platform like TEEs, we're
trying to cover TEEs that doesn't follow Global Platform also here. Most
(or at least a significant part) deployed TEEs, in terms of volume,
today are not Global Platform compliant.
I'd like to view TEE_IOC_CMD as communication channel directly into the
TEE. What goes inside the channel is not something that this subsystem
cares about. The kernel driver will likely need to translate memory
references from user space inside this channel to something usable by
secure world (and vice versa), but apart from that it does as little as
possible except delivering messages.
There's no single definition of what interfaces a TEE has to normal
world. As soon as we try to define a common interface we're bound to
either miss a function completely or just define something that can't be
used by some TEE.
Global Platform has "TEE Client specification" and "TEE Internal Core
API Specification", but neither says anything about what happens between
the lower layers in user space and the TEE.
>
> > > What is the typical and maximum allocation size here?
> > It depends on the design of the Trusted Application in secure world and
> > the client in user space. A few KiB could be the typical allocation
> > size, with a maximum at perhaps 512 KiB (for instance when loading a
> > very large Trusted Application).
>
> So this TEE stuff also encompasses a 'firmware' loader (to the secure
> world, presumably)?
Yes, but that's driver specific. Some TEEs don't have this and the rest
does it in their own way. Global Platform doesn't say anything at all
about this.
>
> That is probably your base level of 'ops' functionality, plus the
> shared memroy stuff.
>
> How does this work if two userspace things run concurrently with
> different firmwares? Is there some locking or something? What is the
> lifetime of this firmware tied to?
In secure world there's a trusted OS (the TEE) with possibly embedded
Trusted Applications (TAs). The TEE may support loading TAs dynamically.
In the OP-TEE case when running on ARM with TrustZone it works like this:
1. The TEE is already loaded when kernel boots.
2. A tee_context is created when user space opens /dev/teeX, this
context holds the sessions.
3. TAs are loaded when needed when a session to the TA is opened.
4. When the context is closed all sessions that are still open are
closed.
5. What happens with the TAs when sessions are closed is internal to the
TEE. The TAs are stored in protected memory which the kernel can't
access any way.
OP-TEE is currently single threaded in the sense that only one thread
can be active in secure world at a time. We have some synchronization
around the SMC that enters secure world, but that's implementation
specific and only there to avoid unnecessary ping pong or spinlock in
secure world.
>
> > I agree that we can drop least one of the _version fields probably both,
> > but something is needed for user space to be able to know which TEE (in
> > secure world) it's communicating with. The uuid will let the client know
> > how how to format the commands passed to TEE_IOC_CMD below.
>
> So you load the firmare, learn what command set it supports then use
> TEE_IOC_CMD to shuttle firmware-specific data to and from?
Correct, except that the firmware (the TEE) is in most cases already
loaded at this stage.
>
> > I've touched on this above, this function the essential when
> > communicating with the driver and secure world. Different TEEs (running
> > in some secure environment) provides different interfaces. By providing
> > an opaque channel we don't have to force something on the TEE. The
> > problem is moved to the user space library which is used when talking to
> > the TEE. The assumption here is that the interface provided the TEE is
> > stable or something that the specific TEE driver can handle with a glue
> > layer.
>
> I would use read/write for this, not ioctl. read/write can work with
> select/poll so you can send your command then go into a polling loop
> waiting for the reply from the firmware.
There's two reasons I'm using ioctl instead.
On ARM with TrustZone (common case right now) the TEE is executing on
the same CPUs as the kernel (but with the ns-bit cleared instead of
set). Delivering a message/request is done with an SMC, you can compare
it with a syscall in user space. OP-TEE doesn't have a scheduler instead
we run on the time of the user space process doing the request. If we
where doing read/write/poll in which of the syscalls would we do the
SMC?
We have to be able to run several commands in parallel. How do we
connect the different reads and writes? A separate file descriptor would
do it, but we would need more than one for each session.
Regards,
Jens
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-17 7:50 ` Jens Wiklander
@ 2015-04-18 8:55 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-18 8:55 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, devicetree, Arnd Bergmann,
javier, Herbert Xu, tpmdd-devel, valentin.manea,
jean-michel.delorme, emmanuel.michel
On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> +/**
> + * struct tee_cmd_data - Opaque command argument
> + * @buf_ptr: [in] A __user pointer to a command buffer
> + * @buf_len: [in] 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.
> + * Used as argument for TEE_IOC_CMD below.
> + */
> +struct tee_ioctl_cmd_data {
> + uint64_t buf_ptr;
> + uint64_t buf_len;
> +};
All structures that cross the user/kernel boundry must use the __
variant of variable names. So for this one, it must be __u64.
Same for all of your ioctl structures.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 8:55 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-18 8:55 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> +/**
> + * struct tee_cmd_data - Opaque command argument
> + * @buf_ptr: [in] A __user pointer to a command buffer
> + * @buf_len: [in] 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.
> + * Used as argument for TEE_IOC_CMD below.
> + */
> +struct tee_ioctl_cmd_data {
> + uint64_t buf_ptr;
> + uint64_t buf_len;
> +};
All structures that cross the user/kernel boundry must use the __
variant of variable names. So for this one, it must be __u64.
Same for all of your ioctl structures.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread
* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-17 7:50 ` Jens Wiklander
@ 2015-04-18 8:57 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-18 8:57 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, devicetree, Arnd Bergmann,
javier, Herbert Xu, tpmdd-devel, valentin.manea,
jean-michel.delorme, emmanuel.michel
On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> +struct tee_device {
> + char name[TEE_MAX_DEV_NAME_LEN];
> + const struct tee_desc *desc;
> + struct device *dev;
No, please embed the device in your structure, don't have a pointer to
it.
> + struct miscdevice miscdev;
This can be a pointer, as the lifecycle of your device is not dictated
by the miscdevice, but rather the 'struct device'.
> +
> + void *driver_data;
You don't need this, use 'struct device''s pointer instead.
> +
> + struct list_head list_shm;
> + struct tee_shm_pool *pool;
> +};
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 8:57 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-18 8:57 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> +struct tee_device {
> + char name[TEE_MAX_DEV_NAME_LEN];
> + const struct tee_desc *desc;
> + struct device *dev;
No, please embed the device in your structure, don't have a pointer to
it.
> + struct miscdevice miscdev;
This can be a pointer, as the lifecycle of your device is not dictated
by the miscdevice, but rather the 'struct device'.
> +
> + void *driver_data;
You don't need this, use 'struct device''s pointer instead.
> +
> + struct list_head list_shm;
> + struct tee_shm_pool *pool;
> +};
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 9:04 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 9:04 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, linux-kernel,
jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > +struct tee_device {
> > + char name[TEE_MAX_DEV_NAME_LEN];
> > + const struct tee_desc *desc;
> > + struct device *dev;
>
> No, please embed the device in your structure, don't have a pointer to
> it.
Greg, "dev" here is not a locally allocated device, but the parent device.
It's actually the same as struct tee_device.miscdev.parent, which could be
used instead and this member deleted.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 9:04 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 9:04 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > +struct tee_device {
> > + char name[TEE_MAX_DEV_NAME_LEN];
> > + const struct tee_desc *desc;
> > + struct device *dev;
>
> No, please embed the device in your structure, don't have a pointer to
> it.
Greg, "dev" here is not a locally allocated device, but the parent device.
It's actually the same as struct tee_device.miscdev.parent, which could be
used instead and this member deleted.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 9:04 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 9:04 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jens Wiklander, valentin.manea-hv44wF8Li93QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, javier-5MUHepqpBA1BDgjK7y7TUQ,
emmanuel.michel-qxv4g6HH51o, Herbert Xu, Arnd Bergmann,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jean-michel.delorme-qxv4g6HH51o,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > +struct tee_device {
> > + char name[TEE_MAX_DEV_NAME_LEN];
> > + const struct tee_desc *desc;
> > + struct device *dev;
>
> No, please embed the device in your structure, don't have a pointer to
> it.
Greg, "dev" here is not a locally allocated device, but the parent device.
It's actually the same as struct tee_device.miscdev.parent, which could be
used instead and this member deleted.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-18 9:04 ` Russell King - ARM Linux
@ 2015-04-18 18:47 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-18 18:47 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, linux-kernel,
jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > +struct tee_device {
> > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > + const struct tee_desc *desc;
> > > + struct device *dev;
> >
> > No, please embed the device in your structure, don't have a pointer to
> > it.
>
> Greg, "dev" here is not a locally allocated device, but the parent device.
> It's actually the same as struct tee_device.miscdev.parent, which could be
> used instead and this member deleted.
A miscdev doesn't need to have a "parent", it's just there to provide a
character device node to userspace, not to represent a "device that you
can do things with in the heirachy".
If you really want that, then use a real 'struct device' as should be
done here. Have just a pointer to a misc device, that is meant to be
dynamic.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 18:47 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-18 18:47 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > +struct tee_device {
> > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > + const struct tee_desc *desc;
> > > + struct device *dev;
> >
> > No, please embed the device in your structure, don't have a pointer to
> > it.
>
> Greg, "dev" here is not a locally allocated device, but the parent device.
> It's actually the same as struct tee_device.miscdev.parent, which could be
> used instead and this member deleted.
A miscdev doesn't need to have a "parent", it's just there to provide a
character device node to userspace, not to represent a "device that you
can do things with in the heirachy".
If you really want that, then use a real 'struct device' as should be
done here. Have just a pointer to a misc device, that is meant to be
dynamic.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-18 18:47 ` Greg Kroah-Hartman
@ 2015-04-18 19:02 ` Russell King - ARM Linux
-1 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 19:02 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, linux-kernel,
jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 08:47:13PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> > On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > +struct tee_device {
> > > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > > + const struct tee_desc *desc;
> > > > + struct device *dev;
> > >
> > > No, please embed the device in your structure, don't have a pointer to
> > > it.
> >
> > Greg, "dev" here is not a locally allocated device, but the parent device.
> > It's actually the same as struct tee_device.miscdev.parent, which could be
> > used instead and this member deleted.
>
> A miscdev doesn't need to have a "parent", it's just there to provide a
> character device node to userspace, not to represent a "device that you
> can do things with in the heirachy".
>
> If you really want that, then use a real 'struct device' as should be
> done here. Have just a pointer to a misc device, that is meant to be
> dynamic.
Let's rewind.
You are saying that "struct device *dev;" should be "struct device dev;"
I'm saying that you are mis-interpreting in your review what _that_ is.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 19:02 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 19:02 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 08:47:13PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> > On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > +struct tee_device {
> > > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > > + const struct tee_desc *desc;
> > > > + struct device *dev;
> > >
> > > No, please embed the device in your structure, don't have a pointer to
> > > it.
> >
> > Greg, "dev" here is not a locally allocated device, but the parent device.
> > It's actually the same as struct tee_device.miscdev.parent, which could be
> > used instead and this member deleted.
>
> A miscdev doesn't need to have a "parent", it's just there to provide a
> character device node to userspace, not to represent a "device that you
> can do things with in the heirachy".
>
> If you really want that, then use a real 'struct device' as should be
> done here. Have just a pointer to a misc device, that is meant to be
> dynamic.
Let's rewind.
You are saying that "struct device *dev;" should be "struct device dev;"
I'm saying that you are mis-interpreting in your review what _that_ is.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-18 19:02 ` Russell King - ARM Linux
@ 2015-04-18 20:37 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-18 20:37 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, linux-kernel,
jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 08:02:24PM +0100, Russell King - ARM Linux wrote:
> On Sat, Apr 18, 2015 at 08:47:13PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> > > On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > > +struct tee_device {
> > > > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > > > + const struct tee_desc *desc;
> > > > > + struct device *dev;
> > > >
> > > > No, please embed the device in your structure, don't have a pointer to
> > > > it.
> > >
> > > Greg, "dev" here is not a locally allocated device, but the parent device.
> > > It's actually the same as struct tee_device.miscdev.parent, which could be
> > > used instead and this member deleted.
> >
> > A miscdev doesn't need to have a "parent", it's just there to provide a
> > character device node to userspace, not to represent a "device that you
> > can do things with in the heirachy".
> >
> > If you really want that, then use a real 'struct device' as should be
> > done here. Have just a pointer to a misc device, that is meant to be
> > dynamic.
>
> Let's rewind.
>
> You are saying that "struct device *dev;" should be "struct device dev;"
Yes.
> I'm saying that you are mis-interpreting in your review what _that_ is.
Probably, I really have no idea what it is anymore. What it _should_ be
is the thing that controls the lifecycle of the structure. Do not use a
miscdevice for that, it will not work, as the TPM developers found out
the hard way.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 20:37 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-18 20:37 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 08:02:24PM +0100, Russell King - ARM Linux wrote:
> On Sat, Apr 18, 2015 at 08:47:13PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> > > On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > > +struct tee_device {
> > > > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > > > + const struct tee_desc *desc;
> > > > > + struct device *dev;
> > > >
> > > > No, please embed the device in your structure, don't have a pointer to
> > > > it.
> > >
> > > Greg, "dev" here is not a locally allocated device, but the parent device.
> > > It's actually the same as struct tee_device.miscdev.parent, which could be
> > > used instead and this member deleted.
> >
> > A miscdev doesn't need to have a "parent", it's just there to provide a
> > character device node to userspace, not to represent a "device that you
> > can do things with in the heirachy".
> >
> > If you really want that, then use a real 'struct device' as should be
> > done here. Have just a pointer to a misc device, that is meant to be
> > dynamic.
>
> Let's rewind.
>
> You are saying that "struct device *dev;" should be "struct device dev;"
Yes.
> I'm saying that you are mis-interpreting in your review what _that_ is.
Probably, I really have no idea what it is anymore. What it _should_ be
is the thing that controls the lifecycle of the structure. Do not use a
miscdevice for that, it will not work, as the TPM developers found out
the hard way.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-18 20:37 ` Greg Kroah-Hartman
@ 2015-04-18 20:50 ` Russell King - ARM Linux
-1 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 20:50 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, linux-kernel,
jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 10:37:16PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Apr 18, 2015 at 08:02:24PM +0100, Russell King - ARM Linux wrote:
> > On Sat, Apr 18, 2015 at 08:47:13PM +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> > > > On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > > > +struct tee_device {
> > > > > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > > > > + const struct tee_desc *desc;
> > > > > > + struct device *dev;
> > > > >
> > > > > No, please embed the device in your structure, don't have a pointer to
> > > > > it.
> > > >
> > > > Greg, "dev" here is not a locally allocated device, but the parent device.
> > > > It's actually the same as struct tee_device.miscdev.parent, which could be
> > > > used instead and this member deleted.
> > >
> > > A miscdev doesn't need to have a "parent", it's just there to provide a
> > > character device node to userspace, not to represent a "device that you
> > > can do things with in the heirachy".
> > >
> > > If you really want that, then use a real 'struct device' as should be
> > > done here. Have just a pointer to a misc device, that is meant to be
> > > dynamic.
> >
> > Let's rewind.
> >
> > You are saying that "struct device *dev;" should be "struct device dev;"
>
> Yes.
>
> > I'm saying that you are mis-interpreting in your review what _that_ is.
>
> Probably, I really have no idea what it is anymore. What it _should_ be
> is the thing that controls the lifecycle of the structure. Do not use a
> miscdevice for that, it will not work, as the TPM developers found out
> the hard way.
I _really_ don't understand what you're going on about.
The "struct device *dev" is a pointer to the struct device corresponding
to the _device_ which is being probed and the tee device is being
registered for - in the case of the submitted code, that is the
struct device embedded in the platform device.
This is a /really/ standard thing to do in drivers - saving a pointer
to the struct device which the driver is responsible for.
So why should this pointer become a struct device itself?
Greg, I think you have performed a disservice by poorly reviewing the
driver, and giving _incorrect_ comments. Please can you have another
look at both patches together and provide a better review. Thanks.
Second point _against_ embedding a struct device here - a struct device
is exposed to userspace. Why expose this to userspace - we have other
ways to manage the lifetime of data structures, such as krefs, which
are not exposed to userspace. What's wrong with using a kref to
control the lifetime of this structure?
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-18 20:50 ` Russell King - ARM Linux
0 siblings, 0 replies; 85+ messages in thread
From: Russell King - ARM Linux @ 2015-04-18 20:50 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 10:37:16PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Apr 18, 2015 at 08:02:24PM +0100, Russell King - ARM Linux wrote:
> > On Sat, Apr 18, 2015 at 08:47:13PM +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> > > > On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > > > +struct tee_device {
> > > > > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > > > > + const struct tee_desc *desc;
> > > > > > + struct device *dev;
> > > > >
> > > > > No, please embed the device in your structure, don't have a pointer to
> > > > > it.
> > > >
> > > > Greg, "dev" here is not a locally allocated device, but the parent device.
> > > > It's actually the same as struct tee_device.miscdev.parent, which could be
> > > > used instead and this member deleted.
> > >
> > > A miscdev doesn't need to have a "parent", it's just there to provide a
> > > character device node to userspace, not to represent a "device that you
> > > can do things with in the heirachy".
> > >
> > > If you really want that, then use a real 'struct device' as should be
> > > done here. Have just a pointer to a misc device, that is meant to be
> > > dynamic.
> >
> > Let's rewind.
> >
> > You are saying that "struct device *dev;" should be "struct device dev;"
>
> Yes.
>
> > I'm saying that you are mis-interpreting in your review what _that_ is.
>
> Probably, I really have no idea what it is anymore. What it _should_ be
> is the thing that controls the lifecycle of the structure. Do not use a
> miscdevice for that, it will not work, as the TPM developers found out
> the hard way.
I _really_ don't understand what you're going on about.
The "struct device *dev" is a pointer to the struct device corresponding
to the _device_ which is being probed and the tee device is being
registered for - in the case of the submitted code, that is the
struct device embedded in the platform device.
This is a /really/ standard thing to do in drivers - saving a pointer
to the struct device which the driver is responsible for.
So why should this pointer become a struct device itself?
Greg, I think you have performed a disservice by poorly reviewing the
driver, and giving _incorrect_ comments. Please can you have another
look at both patches together and provide a better review. Thanks.
Second point _against_ embedding a struct device here - a struct device
is exposed to userspace. Why expose this to userspace - we have other
ways to manage the lifetime of data structures, such as krefs, which
are not exposed to userspace. What's wrong with using a kref to
control the lifetime of this structure?
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 85+ messages in thread* Re: [RFC PATCH 1/2] tee: generic TEE subsystem
2015-04-18 20:50 ` Russell King - ARM Linux
@ 2015-04-19 7:00 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-19 7:00 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jens Wiklander, valentin.manea, devicetree, javier,
emmanuel.michel, Herbert Xu, Arnd Bergmann, linux-kernel,
jean-michel.delorme, tpmdd-devel, linux-arm-kernel
On Sat, Apr 18, 2015 at 09:50:19PM +0100, Russell King - ARM Linux wrote:
> On Sat, Apr 18, 2015 at 10:37:16PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Apr 18, 2015 at 08:02:24PM +0100, Russell King - ARM Linux wrote:
> > > On Sat, Apr 18, 2015 at 08:47:13PM +0200, Greg Kroah-Hartman wrote:
> > > > On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> > > > > On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > > > > +struct tee_device {
> > > > > > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > > > > > + const struct tee_desc *desc;
> > > > > > > + struct device *dev;
> > > > > >
> > > > > > No, please embed the device in your structure, don't have a pointer to
> > > > > > it.
> > > > >
> > > > > Greg, "dev" here is not a locally allocated device, but the parent device.
> > > > > It's actually the same as struct tee_device.miscdev.parent, which could be
> > > > > used instead and this member deleted.
> > > >
> > > > A miscdev doesn't need to have a "parent", it's just there to provide a
> > > > character device node to userspace, not to represent a "device that you
> > > > can do things with in the heirachy".
> > > >
> > > > If you really want that, then use a real 'struct device' as should be
> > > > done here. Have just a pointer to a misc device, that is meant to be
> > > > dynamic.
> > >
> > > Let's rewind.
> > >
> > > You are saying that "struct device *dev;" should be "struct device dev;"
> >
> > Yes.
> >
> > > I'm saying that you are mis-interpreting in your review what _that_ is.
> >
> > Probably, I really have no idea what it is anymore. What it _should_ be
> > is the thing that controls the lifecycle of the structure. Do not use a
> > miscdevice for that, it will not work, as the TPM developers found out
> > the hard way.
>
> I _really_ don't understand what you're going on about.
>
> The "struct device *dev" is a pointer to the struct device corresponding
> to the _device_ which is being probed and the tee device is being
> registered for - in the case of the submitted code, that is the
> struct device embedded in the platform device.
>
> This is a /really/ standard thing to do in drivers - saving a pointer
> to the struct device which the driver is responsible for.
Yes, but this structure says it is a "tee_device", and as such, should
be a real device, not just an internal structure that is never exposed
to userspace, right?
> So why should this pointer become a struct device itself?
Because it is a device. It should be a child of the platform device.
Unless it's just a "normal" device, then platform device shouldn't be
used here :)
> Greg, I think you have performed a disservice by poorly reviewing the
> driver, and giving _incorrect_ comments. Please can you have another
> look at both patches together and provide a better review. Thanks.
I think the comment about how the model is all messed up as it looks
like the TPM original code is correct.
> Second point _against_ embedding a struct device here - a struct device
> is exposed to userspace. Why expose this to userspace - we have other
> ways to manage the lifetime of data structures, such as krefs, which
> are not exposed to userspace. What's wrong with using a kref to
> control the lifetime of this structure?
It's a device, why wouldn't it be exposed to userspace.
If this isn't a device, then yes, it doesn't need to be. But then don't
call it a "tee_device" :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread* [RFC PATCH 1/2] tee: generic TEE subsystem
@ 2015-04-19 7:00 ` Greg Kroah-Hartman
0 siblings, 0 replies; 85+ messages in thread
From: Greg Kroah-Hartman @ 2015-04-19 7:00 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Apr 18, 2015 at 09:50:19PM +0100, Russell King - ARM Linux wrote:
> On Sat, Apr 18, 2015 at 10:37:16PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Apr 18, 2015 at 08:02:24PM +0100, Russell King - ARM Linux wrote:
> > > On Sat, Apr 18, 2015 at 08:47:13PM +0200, Greg Kroah-Hartman wrote:
> > > > On Sat, Apr 18, 2015 at 10:04:20AM +0100, Russell King - ARM Linux wrote:
> > > > > On Sat, Apr 18, 2015 at 10:57:12AM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Apr 17, 2015 at 09:50:56AM +0200, Jens Wiklander wrote:
> > > > > > > +struct tee_device {
> > > > > > > + char name[TEE_MAX_DEV_NAME_LEN];
> > > > > > > + const struct tee_desc *desc;
> > > > > > > + struct device *dev;
> > > > > >
> > > > > > No, please embed the device in your structure, don't have a pointer to
> > > > > > it.
> > > > >
> > > > > Greg, "dev" here is not a locally allocated device, but the parent device.
> > > > > It's actually the same as struct tee_device.miscdev.parent, which could be
> > > > > used instead and this member deleted.
> > > >
> > > > A miscdev doesn't need to have a "parent", it's just there to provide a
> > > > character device node to userspace, not to represent a "device that you
> > > > can do things with in the heirachy".
> > > >
> > > > If you really want that, then use a real 'struct device' as should be
> > > > done here. Have just a pointer to a misc device, that is meant to be
> > > > dynamic.
> > >
> > > Let's rewind.
> > >
> > > You are saying that "struct device *dev;" should be "struct device dev;"
> >
> > Yes.
> >
> > > I'm saying that you are mis-interpreting in your review what _that_ is.
> >
> > Probably, I really have no idea what it is anymore. What it _should_ be
> > is the thing that controls the lifecycle of the structure. Do not use a
> > miscdevice for that, it will not work, as the TPM developers found out
> > the hard way.
>
> I _really_ don't understand what you're going on about.
>
> The "struct device *dev" is a pointer to the struct device corresponding
> to the _device_ which is being probed and the tee device is being
> registered for - in the case of the submitted code, that is the
> struct device embedded in the platform device.
>
> This is a /really/ standard thing to do in drivers - saving a pointer
> to the struct device which the driver is responsible for.
Yes, but this structure says it is a "tee_device", and as such, should
be a real device, not just an internal structure that is never exposed
to userspace, right?
> So why should this pointer become a struct device itself?
Because it is a device. It should be a child of the platform device.
Unless it's just a "normal" device, then platform device shouldn't be
used here :)
> Greg, I think you have performed a disservice by poorly reviewing the
> driver, and giving _incorrect_ comments. Please can you have another
> look at both patches together and provide a better review. Thanks.
I think the comment about how the model is all messed up as it looks
like the TPM original code is correct.
> Second point _against_ embedding a struct device here - a struct device
> is exposed to userspace. Why expose this to userspace - we have other
> ways to manage the lifetime of data structures, such as krefs, which
> are not exposed to userspace. What's wrong with using a kref to
> control the lifetime of this structure?
It's a device, why wouldn't it be exposed to userspace.
If this isn't a device, then yes, it doesn't need to be. But then don't
call it a "tee_device" :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 85+ messages in thread