Re: [PATCH v4 1/2] create SMAF module
From: Laura Abbott
Date: Mon Oct 05 2015 - 21:59:07 EST
On 10/05/2015 03:11 AM, Benjamin Gaignard wrote:
diff --git a/drivers/smaf/smaf-core.c b/drivers/smaf/smaf-core.c
new file mode 100644
index 0000000..37914e7
--- /dev/null
+++ b/drivers/smaf/smaf-core.c
@@ -0,0 +1,736 @@
+/*
+ * smaf.c
+ *
+ * Copyright (C) Linaro SA 2015
+ * Author: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx> for Linaro.
+ * License terms: GNU General Public License (GPL), version 2
+ */
+
+#include <linux/device.h>
+#include <linux/dma-buf.h>
+#include <linux/dma-mapping.h>
+#include <linux/fs.h>
+#include <linux/ioctl.h>
+#include <linux/list_sort.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/smaf.h>
+#include <linux/smaf-allocator.h>
+#include <linux/smaf-secure.h>
+#include <linux/uaccess.h>
+
+struct smaf_handle {
+ struct dma_buf *dmabuf;
+ struct smaf_allocator *allocator;
+ struct dma_buf *db_alloc;
+ size_t length;
+ unsigned int flags;
+ int fd;
+ bool is_secure;
+ void *secure_ctx;
+};
+
+/**
+ * struct smaf_device - smaf device node private data
+ * @misc_dev: the misc device
+ * @head: list of allocator
+ * @lock: list and secure pointer mutex
+ * @secure: pointer to secure functions helpers
+ */
+struct smaf_device {
+ struct miscdevice misc_dev;
+ struct list_head head;
+ /* list and secure pointer lock*/
+ struct mutex lock;
+ struct smaf_secure *secure;
+};
+
+static struct smaf_device smaf_dev;
+
+/**
+ * smaf_allow_cpu_access return true if CPU can access to memory
+ * if their is no secure module associated to SMAF assume that CPU can get
+ * access to the memory.
+ */
+static bool smaf_allow_cpu_access(struct smaf_handle *handle,
+ unsigned long flags)
+{
+ if (!handle->is_secure)
+ return true;
+
+ if (!smaf_dev.secure)
+ return true;
+
+ if (!smaf_dev.secure->allow_cpu_access)
+ return true;
+
+ return smaf_dev.secure->allow_cpu_access(handle->secure_ctx, flags);
+}
+
+static int smaf_grant_access(struct smaf_handle *handle, struct device *dev,
+ dma_addr_t addr, size_t size,
+ enum dma_data_direction dir)
+{
+ if (!handle->is_secure)
+ return 0;
+
+ if (!smaf_dev.secure)
+ return -EINVAL;
+
+ if (!smaf_dev.secure->grant_access)
+ return -EINVAL;
+
+ return smaf_dev.secure->grant_access(handle->secure_ctx,
+ dev, addr, size, dir);
+}
+
+static void smaf_revoke_access(struct smaf_handle *handle, struct device *dev,
+ dma_addr_t addr, size_t size,
+ enum dma_data_direction dir)
+{
+ if (!handle->is_secure)
+ return;
+
+ if (!smaf_dev.secure)
+ return;
+
+ if (!smaf_dev.secure->revoke_access)
+ return;
+
+ smaf_dev.secure->revoke_access(handle->secure_ctx,
+ dev, addr, size, dir);
+}
+
+static int smaf_secure_handle(struct smaf_handle *handle)
+{
+ if (handle->is_secure)
+ return 0;
+
+ if (!smaf_dev.secure)
+ return -EINVAL;
+
+ if (!smaf_dev.secure->create_context)
+ return -EINVAL;
+
+ handle->secure_ctx = smaf_dev.secure->create_context();
+
+ if (!handle->secure_ctx)
+ return -EINVAL;
+
+ handle->is_secure = true;
+ return 0;
+}
+
+static int smaf_unsecure_handle(struct smaf_handle *handle)
+{
+ if (!handle->is_secure)
+ return 0;
+
+ if (!smaf_dev.secure)
+ return -EINVAL;
+
+ if (!smaf_dev.secure->destroy_context)
+ return -EINVAL;
+
+ if (smaf_dev.secure->destroy_context(handle->secure_ctx))
+ return -EINVAL;
+
+ handle->secure_ctx = NULL;
+ handle->is_secure = false;
+ return 0;
+}
All these functions need to be protected by a lock, otherwise the
secure state could change. For that matter, I think the smaf_handle
needs a lock to protect its state as well for places like map_dma_buf
<snip>
+static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ switch (cmd) {
+ case SMAF_IOC_CREATE:
+ {
+ struct smaf_create_data data;
+ struct smaf_handle *handle;
+
+ if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+ return -EFAULT;
+
+ handle = smaf_create_handle(data.length, data.flags);
+ if (!handle)
+ return -EINVAL;
+
+ if (data.name[0]) {
+ /* user force allocator selection */
+ if (smaf_select_allocator_by_name(handle->dmabuf,
+ data.name)) {
+ dma_buf_put(handle->dmabuf);
+ return -EINVAL;
+ }
+ }
+
+ handle->fd = dma_buf_fd(handle->dmabuf, data.flags);
+ if (handle->fd < 0) {
+ dma_buf_put(handle->dmabuf);
+ return -EINVAL;
+ }
+
+ data.fd = handle->fd;
+ if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
+ dma_buf_put(handle->dmabuf);
+ return -EFAULT;
+ }
+ break;
+ }
+ case SMAF_IOC_GET_SECURE_FLAG:
+ {
+ struct smaf_secure_flag data;
+ struct dma_buf *dmabuf;
+
+ if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+ return -EFAULT;
+
+ dmabuf = dma_buf_get(data.fd);
+ if (!dmabuf)
+ return -EINVAL;
+
+ data.secure = smaf_is_secure(dmabuf);
+ dma_buf_put(dmabuf);
+
+ if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd)))
+ return -EFAULT;
+ break;
+ }
+ case SMAF_IOC_SET_SECURE_FLAG:
+ {
+ struct smaf_secure_flag data;
+ struct dma_buf *dmabuf;
+ int ret;
+
+ if (!smaf_dev.secure)
+ return -EINVAL;
+
+ if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+ return -EFAULT;
+
+ dmabuf = dma_buf_get(data.fd);
+ if (!dmabuf)
+ return -EINVAL;
+
+ ret = smaf_set_secure(dmabuf, data.secure);
+
+ dma_buf_put(dmabuf);
+
+ if (ret)
+ return -EINVAL;
+
+ break;
+ }
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
How would you see this tying into something like Ion? It seems like
Ion and SMAF have non-zero over lapping functionality for some things
or that SMAF could be implemented as a heap type. I think my biggest
concern here is that it seems like either Ion or SMAF is going to feel
redundant as an interface.
Thanks,
Laura
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/