[PATCH v7 0/3] Secure Memory Allocation Framework

[PATCH v7 0/3] Secure Memory Allocation Framework

[Benjamin Gaignard]

version 7 changes:
 - rebased on kernel 4.6-rc7
 - simplify secure module API
 - add vma ops to be able to detect mmap/munmap calls
 - add ioctl to get number and allocator names
 - update libsmaf with adding tests
   https://git.linaro.org/people/benjamin.gaignard/libsmaf.git
 - add debug log in fake secure module

version 6 changes:
 - rebased on kernel 4.5-rc4
 - fix mmapping bug while requested allocation size isn't a a multiple of
   PAGE_SIZE (add a test for this in libsmaf)

version 5 changes:
 - rebased on kernel 4.3-rc6
 - rework locking schema and make handle status use an atomic_t
 - add a fake secure module to allow performing tests without trusted
   environment

version 4 changes:
 - rebased on kernel 4.3-rc3
 - fix missing EXPORT_SYMBOL for smaf_create_handle()

version 3 changes:
 - Remove ioctl for allocator selection instead provide the name of
   the targeted allocator with allocation request.
   Selecting allocator from userland isn't the prefered way of working
   but is needed when the first user of the buffer is a software component.
 - Fix issues in case of error while creating smaf handle.
 - Fix module license.
 - Update libsmaf and tests to care of the SMAF API evolution
   https://git.linaro.org/people/benjamin.gaignard/libsmaf.git

version 2 changes:
 - Add one ioctl to allow allocator selection from userspace.
   This is required for the uses case where the first user of
   the buffer is a software IP which can't perform dma_buf attachement.
 - Add name and ranking to allocator structure to be able to sort them.
 - Create a tiny library to test SMAF:
   https://git.linaro.org/people/benjamin.gaignard/libsmaf.git
 - Fix one issue when try to secure buffer without secure module registered

The outcome of the previous RFC about how do secure data path was the need
of a secure memory allocator (https://lkml.org/lkml/2015/5/5/551)

SMAF goal is to provide a framework that allow allocating and securing
memory by using dma_buf. Each platform have it own way to perform those two
features so SMAF design allow to register helper modules to perform them.

To be sure to select the best allocation method for devices SMAF implement
deferred allocation mechanism: memory allocation is only done when the first
device effectively required it.
Allocator modules have to implement a match() to let SMAF know if they are
compatibles with devices needs.
This patch set provide an example of allocator module which use
dma_{alloc/free/mmap}_attrs() and check if at least one device have
coherent_dma_mask set to DMA_BIT_MASK(32) in match function.
I have named smaf-cma.c like it is done for drm_gem_cma_helper.c even if
a better name could be found for this file.

Secure modules are responsibles of granting and revoking devices access rights
on the memory. Secure module is also called to check if CPU map memory into
kernel and user address spaces.
An example of secure module implementation can be found here:
http://git.linaro.org/people/benjamin.gaignard/optee-sdp.git
This code isn't yet part of the patch set because it depends on generic TEE
which is still under discussion (https://lwn.net/Articles/644646/)

For allocation part of SMAF code I get inspirated by Sumit Semwal work about
constraint aware allocator.

Benjamin Gaignard (3):
  create SMAF module
  SMAF: add CMA allocator
  SMAF: add fake secure module

 drivers/Kconfig                |   2 +
 drivers/Makefile               |   1 +
 drivers/smaf/Kconfig           |  17 +
 drivers/smaf/Makefile          |   3 +
 drivers/smaf/smaf-cma.c        | 199 +++++++++++
 drivers/smaf/smaf-core.c       | 794 +++++++++++++++++++++++++++++++++++++++++
 drivers/smaf/smaf-fakesecure.c |  85 +++++
 include/linux/smaf-allocator.h |  54 +++
 include/linux/smaf-secure.h    |  73 ++++
 include/uapi/linux/smaf.h      |  66 ++++
 10 files changed, 1294 insertions(+)
 create mode 100644 drivers/smaf/Kconfig
 create mode 100644 drivers/smaf/Makefile
 create mode 100644 drivers/smaf/smaf-cma.c
 create mode 100644 drivers/smaf/smaf-core.c
 create mode 100644 drivers/smaf/smaf-fakesecure.c
 create mode 100644 include/linux/smaf-allocator.h
 create mode 100644 include/linux/smaf-secure.h
 create mode 100644 include/uapi/linux/smaf.h

-- 
1.9.1

[PATCH v7 1/3] create SMAF module

[Benjamin Gaignard]

Secure Memory Allocation Framework goal is to be able
to allocate memory that can be securing.
There is so much ways to allocate and securing memory that SMAF
doesn't do it by itself but need help of additional modules.
To be sure to use the correct allocation method SMAF implement
deferred allocation (i.e. allocate memory when only really needed)

Allocation modules (smaf-alloctor.h):
SMAF could manage with multiple allocation modules at same time.
To select the good one SMAF call match() to be sure that a module
can allocate memory for a given list of devices. It is to the module
to check if the devices are compatible or not with it allocation
method.

Securing module (smaf-secure.h):
The way of how securing memory it is done is platform specific.
Secure module is responsible of grant/revoke memory access.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/Kconfig                |   2 +
 drivers/Makefile               |   1 +
 drivers/smaf/Kconfig           |   5 +
 drivers/smaf/Makefile          |   1 +
 drivers/smaf/smaf-core.c       | 794 +++++++++++++++++++++++++++++++++++++++++
 include/linux/smaf-allocator.h |  54 +++
 include/linux/smaf-secure.h    |  73 ++++
 include/uapi/linux/smaf.h      |  66 ++++
 8 files changed, 996 insertions(+)
 create mode 100644 drivers/smaf/Kconfig
 create mode 100644 drivers/smaf/Makefile
 create mode 100644 drivers/smaf/smaf-core.c
 create mode 100644 include/linux/smaf-allocator.h
 create mode 100644 include/linux/smaf-secure.h
 create mode 100644 include/uapi/linux/smaf.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index d2ac339..f5262fd 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -198,4 +198,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
 
 source "drivers/fpga/Kconfig"
 
+source "drivers/smaf/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 8f5d076..b2fb04a 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -172,3 +172,4 @@ obj-$(CONFIG_STM)		+= hwtracing/stm/
 obj-$(CONFIG_ANDROID)		+= android/
 obj-$(CONFIG_NVMEM)		+= nvmem/
 obj-$(CONFIG_FPGA)		+= fpga/
+obj-$(CONFIG_SMAF) 		+= smaf/
diff --git a/drivers/smaf/Kconfig b/drivers/smaf/Kconfig
new file mode 100644
index 0000000..d36651a
--- /dev/null
+++ b/drivers/smaf/Kconfig
@@ -0,0 +1,5 @@
+config SMAF
+	tristate "Secure Memory Allocation Framework"
+	depends on DMA_SHARED_BUFFER
+	help
+	  Choose this option to enable Secure Memory Allocation Framework
diff --git a/drivers/smaf/Makefile b/drivers/smaf/Makefile
new file mode 100644
index 0000000..40cd882
--- /dev/null
+++ b/drivers/smaf/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SMAF) += smaf-core.o
diff --git a/drivers/smaf/smaf-core.c b/drivers/smaf/smaf-core.c
new file mode 100644
index 0000000..3bf619c
--- /dev/null
+++ b/drivers/smaf/smaf-core.c
@@ -0,0 +1,794 @@
+/*
+ * smaf.c
+ *
+ * Copyright (C) Linaro SA 2015
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/cpu.h>
+#include <linux/device.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;
+	atomic_t 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;
+
+static bool have_secure_module(void)
+{
+	return !!smaf_dev.secure;
+}
+
+/**
+ * smaf_grant_access - return true if the specified device can get access
+ * to the memory area
+ *
+ * This function must be called with smaf_dev.lock set
+ */
+static bool smaf_grant_access(struct smaf_handle *handle, struct device *dev,
+			      dma_addr_t addr, size_t size,
+			      enum dma_data_direction dir)
+{
+	if (!atomic_read(&handle->is_secure))
+		return true;
+
+	if (!have_secure_module())
+		return false;
+
+	return smaf_dev.secure->grant_access(handle->secure_ctx,
+					     dev, addr, size, dir);
+}
+
+/**
+ * smaf_revoke_access
+ * This function must be called with smaf_dev.lock set
+ */
+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 (!atomic_read(&handle->is_secure))
+		return;
+
+	if (!have_secure_module())
+		return;
+
+	smaf_dev.secure->revoke_access(handle->secure_ctx,
+				       dev, addr, size, dir);
+}
+
+static int smaf_secure_handle(struct smaf_handle *handle)
+{
+	if (atomic_read(&handle->is_secure))
+		return 0;
+
+	if (!have_secure_module())
+		return -EINVAL;
+
+	handle->secure_ctx = smaf_dev.secure->create_ctx();
+
+	if (!handle->secure_ctx)
+		return -EINVAL;
+
+	atomic_set(&handle->is_secure, 1);
+	return 0;
+}
+
+static int smaf_unsecure_handle(struct smaf_handle *handle)
+{
+	if (!atomic_read(&handle->is_secure))
+		return 0;
+
+	if (!have_secure_module())
+		return -EINVAL;
+
+	if (smaf_dev.secure->destroy_ctx(handle->secure_ctx))
+		return -EINVAL;
+
+	handle->secure_ctx = NULL;
+	atomic_set(&handle->is_secure, 0);
+	return 0;
+}
+
+int smaf_register_secure(struct smaf_secure *s)
+{
+	/* make sure that secure module have all required functions
+	 * to avoid test them each time later
+	 */
+	WARN_ON(!s || !s->create_ctx || !s->destroy_ctx ||
+		!s->grant_access || !s->revoke_access);
+
+	mutex_lock(&smaf_dev.lock);
+	smaf_dev.secure = s;
+	mutex_unlock(&smaf_dev.lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(smaf_register_secure);
+
+void smaf_unregister_secure(struct smaf_secure *s)
+{
+	mutex_lock(&smaf_dev.lock);
+	if (smaf_dev.secure == s)
+		smaf_dev.secure = NULL;
+	mutex_unlock(&smaf_dev.lock);
+}
+EXPORT_SYMBOL(smaf_unregister_secure);
+
+static struct smaf_allocator *smaf_find_allocator(struct dma_buf *dmabuf)
+{
+	struct smaf_allocator *alloc;
+
+	list_for_each_entry(alloc, &smaf_dev.head, list_node) {
+		if (alloc->match(dmabuf))
+			return alloc;
+	}
+
+	return NULL;
+}
+
+static struct smaf_allocator *smaf_get_first_allocator(struct dma_buf *dmabuf)
+{
+	/* the first allocator of the list is the preferred allocator */
+	return list_first_entry(&smaf_dev.head, struct smaf_allocator,
+			list_node);
+}
+
+static int smaf_allocator_compare(void *priv,
+				  struct list_head *lh_a,
+				  struct list_head *lh_b)
+{
+	struct smaf_allocator *a = list_entry(lh_a,
+					      struct smaf_allocator, list_node);
+	struct smaf_allocator *b = list_entry(lh_b,
+					      struct smaf_allocator, list_node);
+	int diff;
+
+	diff = b->ranking - a->ranking;
+	if (diff)
+		return diff;
+
+	return strcmp(a->name, b->name);
+}
+
+int smaf_register_allocator(struct smaf_allocator *alloc)
+{
+	WARN_ON(!alloc || !alloc->match || !alloc->allocate || !alloc->name);
+
+	mutex_lock(&smaf_dev.lock);
+	list_add(&alloc->list_node, &smaf_dev.head);
+	list_sort(NULL, &smaf_dev.head, smaf_allocator_compare);
+	mutex_unlock(&smaf_dev.lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(smaf_register_allocator);
+
+void smaf_unregister_allocator(struct smaf_allocator *alloc)
+{
+	mutex_lock(&smaf_dev.lock);
+	list_del(&alloc->list_node);
+	mutex_unlock(&smaf_dev.lock);
+}
+EXPORT_SYMBOL(smaf_unregister_allocator);
+
+static struct dma_buf_attachment *smaf_find_attachment(struct dma_buf *db_alloc,
+						       struct device *dev)
+{
+	struct dma_buf_attachment *attach_obj;
+
+	list_for_each_entry(attach_obj, &db_alloc->attachments, node) {
+		if (attach_obj->dev == dev)
+			return attach_obj;
+	}
+
+	return NULL;
+}
+
+static struct sg_table *smaf_map_dma_buf(struct dma_buf_attachment *attachment,
+					 enum dma_data_direction direction)
+{
+	struct dma_buf_attachment *db_attachment;
+	struct dma_buf *dmabuf = attachment->dmabuf;
+	struct smaf_handle *handle = dmabuf->priv;
+	struct sg_table *sgt;
+	unsigned int count_done, count;
+	struct scatterlist *sg;
+
+	/* try to find an allocator */
+	if (!handle->allocator) {
+		struct smaf_allocator *alloc;
+
+		mutex_lock(&smaf_dev.lock);
+		alloc = smaf_find_allocator(dmabuf);
+		mutex_unlock(&smaf_dev.lock);
+
+		/* still no allocator ? */
+		if (!alloc)
+			return NULL;
+
+		handle->allocator = alloc;
+	}
+
+	if (!handle->db_alloc) {
+		struct dma_buf *db_alloc;
+
+		db_alloc = handle->allocator->allocate(dmabuf,
+						       handle->length,
+						       handle->flags);
+		if (!db_alloc)
+			return NULL;
+
+		handle->db_alloc = db_alloc;
+	}
+
+	db_attachment = smaf_find_attachment(handle->db_alloc, attachment->dev);
+	sgt = dma_buf_map_attachment(db_attachment, direction);
+
+	if (!sgt)
+		return NULL;
+
+	if (!atomic_read(&handle->is_secure))
+		return sgt;
+
+	mutex_lock(&smaf_dev.lock);
+
+	/* now secure the data */
+	for_each_sg(sgt->sgl, sg, sgt->nents, count_done) {
+		if (!smaf_grant_access(handle, db_attachment->dev,
+				       sg_phys(sg), sg->length, direction))
+			goto failed;
+	}
+
+	mutex_unlock(&smaf_dev.lock);
+	return sgt;
+
+failed:
+	for_each_sg(sgt->sgl, sg, count_done, count) {
+		smaf_revoke_access(handle, db_attachment->dev,
+				   sg_phys(sg), sg->length, direction);
+	}
+
+	mutex_unlock(&smaf_dev.lock);
+
+	sg_free_table(sgt);
+	kfree(sgt);
+	return NULL;
+}
+
+static void smaf_unmap_dma_buf(struct dma_buf_attachment *attachment,
+			       struct sg_table *sgt,
+			       enum dma_data_direction direction)
+{
+	struct dma_buf_attachment *db_attachment;
+	struct dma_buf *dmabuf = attachment->dmabuf;
+	struct smaf_handle *handle = dmabuf->priv;
+	struct scatterlist *sg;
+	unsigned int count;
+
+	if (!handle->db_alloc)
+		return;
+
+	db_attachment = smaf_find_attachment(handle->db_alloc, attachment->dev);
+	if (!db_attachment)
+		return;
+
+	if (atomic_read(&handle->is_secure)) {
+		mutex_lock(&smaf_dev.lock);
+		for_each_sg(sgt->sgl, sg, sgt->nents, count) {
+			smaf_revoke_access(handle, db_attachment->dev,
+					   sg_phys(sg), sg->length, direction);
+		}
+		mutex_unlock(&smaf_dev.lock);
+	}
+
+	dma_buf_unmap_attachment(db_attachment, sgt, direction);
+}
+
+static void smaf_vm_close(struct vm_area_struct *vma)
+{
+	struct smaf_handle *handle = vma->vm_private_data;
+	enum dma_data_direction dir;
+
+	if (vma->vm_flags == VM_READ)
+		dir = DMA_TO_DEVICE;
+
+	if (vma->vm_flags == VM_WRITE)
+		dir = DMA_FROM_DEVICE;
+
+	if (vma->vm_flags == (VM_READ | VM_WRITE))
+		dir = DMA_BIDIRECTIONAL;
+
+	mutex_lock(&smaf_dev.lock);
+	smaf_revoke_access(handle, get_cpu_device(0), 0, handle->length, dir);
+	mutex_unlock(&smaf_dev.lock);
+}
+
+static struct vm_operations_struct smaf_vma_ops = {
+	.close = smaf_vm_close,
+};
+
+static int smaf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+	bool ret;
+	enum dma_data_direction dir;
+
+	/* if no allocator attached, get the first allocator */
+	if (!handle->allocator) {
+		struct smaf_allocator *alloc;
+
+		mutex_lock(&smaf_dev.lock);
+		alloc = smaf_get_first_allocator(dmabuf);
+		mutex_unlock(&smaf_dev.lock);
+
+		/* still no allocator ? */
+		if (!alloc)
+			return -EINVAL;
+
+		handle->allocator = alloc;
+	}
+
+	if (!handle->db_alloc) {
+		struct dma_buf *db_alloc;
+
+		db_alloc = handle->allocator->allocate(dmabuf,
+						       handle->length,
+						       handle->flags);
+		if (!db_alloc)
+			return -EINVAL;
+
+		handle->db_alloc = db_alloc;
+	}
+
+	vma->vm_private_data = handle;
+	vma->vm_ops = &smaf_vma_ops;
+
+	if (vma->vm_flags == VM_READ)
+		dir = DMA_TO_DEVICE;
+
+	if (vma->vm_flags == VM_WRITE)
+		dir = DMA_FROM_DEVICE;
+
+	if (vma->vm_flags == (VM_READ | VM_WRITE))
+		dir = DMA_BIDIRECTIONAL;
+
+	mutex_lock(&smaf_dev.lock);
+	ret = smaf_grant_access(handle,
+				get_cpu_device(0), 0, handle->length, dir);
+	mutex_unlock(&smaf_dev.lock);
+
+	if (!ret)
+		return -EINVAL;
+
+	return dma_buf_mmap(handle->db_alloc, vma, 0);
+}
+
+static void smaf_dma_buf_release(struct dma_buf *dmabuf)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+
+	if (handle->db_alloc)
+		dma_buf_put(handle->db_alloc);
+
+	mutex_lock(&smaf_dev.lock);
+	smaf_unsecure_handle(handle);
+	mutex_unlock(&smaf_dev.lock);
+
+	kfree(handle);
+}
+
+static int smaf_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
+					 enum dma_data_direction dir)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+	bool ret;
+
+	if (!handle->db_alloc)
+		return -EINVAL;
+
+	mutex_lock(&smaf_dev.lock);
+	ret = smaf_grant_access(handle,
+				get_cpu_device(0), 0, handle->length, dir);
+	mutex_unlock(&smaf_dev.lock);
+
+	if (!ret)
+		return -EINVAL;
+
+	return dma_buf_begin_cpu_access(handle->db_alloc, dir);
+}
+
+static int smaf_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
+				       enum dma_data_direction dir)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+	int ret;
+
+	if (!handle->db_alloc)
+		return -EINVAL;
+
+	ret = dma_buf_end_cpu_access(handle->db_alloc, dir);
+
+	mutex_lock(&smaf_dev.lock);
+	smaf_revoke_access(handle, get_cpu_device(0), 0, handle->length, dir);
+	mutex_unlock(&smaf_dev.lock);
+
+	return ret;
+}
+
+static void *smaf_dma_buf_kmap_atomic(struct dma_buf *dmabuf,
+				      unsigned long offset)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+
+	if (!handle->db_alloc)
+		return NULL;
+
+	return dma_buf_kmap_atomic(handle->db_alloc, offset);
+}
+
+static void smaf_dma_buf_kunmap_atomic(struct dma_buf *dmabuf,
+				       unsigned long offset, void *ptr)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+
+	if (!handle->db_alloc)
+		return;
+
+	dma_buf_kunmap_atomic(handle->db_alloc, offset, ptr);
+}
+
+static void *smaf_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+
+	if (!handle->db_alloc)
+		return NULL;
+
+	return dma_buf_kmap(handle->db_alloc, offset);
+}
+
+static void smaf_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset,
+				void *ptr)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+
+	if (!handle->db_alloc)
+		return;
+
+	dma_buf_kunmap(handle->db_alloc, offset, ptr);
+}
+
+static void *smaf_dma_buf_vmap(struct dma_buf *dmabuf)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+
+	if (!handle->db_alloc)
+		return NULL;
+
+	return dma_buf_vmap(handle->db_alloc);
+}
+
+static void smaf_dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+
+	if (!handle->db_alloc)
+		return;
+
+	dma_buf_vunmap(handle->db_alloc, vaddr);
+}
+
+static int smaf_attach(struct dma_buf *dmabuf, struct device *dev,
+		       struct dma_buf_attachment *attach)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+	struct dma_buf_attachment *db_attach;
+
+	if (!handle->db_alloc)
+		return 0;
+
+	db_attach = dma_buf_attach(handle->db_alloc, dev);
+
+	return IS_ERR(db_attach);
+}
+
+static void smaf_detach(struct dma_buf *dmabuf,
+			struct dma_buf_attachment *attach)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+	struct dma_buf_attachment *db_attachment;
+
+	if (!handle->db_alloc)
+		return;
+
+	db_attachment = smaf_find_attachment(handle->db_alloc, attach->dev);
+	dma_buf_detach(handle->db_alloc, db_attachment);
+}
+
+static struct dma_buf_ops smaf_dma_buf_ops = {
+	.attach = smaf_attach,
+	.detach = smaf_detach,
+	.map_dma_buf = smaf_map_dma_buf,
+	.unmap_dma_buf = smaf_unmap_dma_buf,
+	.mmap = smaf_mmap,
+	.release = smaf_dma_buf_release,
+	.begin_cpu_access = smaf_dma_buf_begin_cpu_access,
+	.end_cpu_access = smaf_dma_buf_end_cpu_access,
+	.kmap_atomic = smaf_dma_buf_kmap_atomic,
+	.kunmap_atomic = smaf_dma_buf_kunmap_atomic,
+	.kmap = smaf_dma_buf_kmap,
+	.kunmap = smaf_dma_buf_kunmap,
+	.vmap = smaf_dma_buf_vmap,
+	.vunmap = smaf_dma_buf_vunmap,
+};
+
+static bool is_smaf_dmabuf(struct dma_buf *dmabuf)
+{
+	return dmabuf->ops == &smaf_dma_buf_ops;
+}
+
+bool smaf_is_secure(struct dma_buf *dmabuf)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+
+	if (!is_smaf_dmabuf(dmabuf))
+		return false;
+
+	return atomic_read(&handle->is_secure);
+}
+EXPORT_SYMBOL(smaf_is_secure);
+
+int smaf_set_secure(struct dma_buf *dmabuf, bool secure)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+	int ret;
+
+	if (!is_smaf_dmabuf(dmabuf))
+		return -EINVAL;
+
+	mutex_lock(&smaf_dev.lock);
+	if (secure)
+		ret = smaf_secure_handle(handle);
+	else
+		ret = smaf_unsecure_handle(handle);
+	mutex_unlock(&smaf_dev.lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(smaf_set_secure);
+
+int smaf_select_allocator_by_name(struct dma_buf *dmabuf, char *name)
+{
+	struct smaf_handle *handle = dmabuf->priv;
+	struct smaf_allocator *alloc;
+
+	if (!is_smaf_dmabuf(dmabuf))
+		return -EINVAL;
+
+	if (handle->allocator)
+		return -EINVAL;
+
+	mutex_lock(&smaf_dev.lock);
+
+	list_for_each_entry(alloc, &smaf_dev.head, list_node) {
+		if (!strncmp(alloc->name, name, ALLOCATOR_NAME_LENGTH)) {
+			handle->allocator = alloc;
+			handle->db_alloc = NULL;
+		}
+	}
+
+	mutex_unlock(&smaf_dev.lock);
+
+	if (!handle->allocator)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL(smaf_select_allocator_by_name);
+
+struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags)
+{
+	struct smaf_handle *handle;
+
+	DEFINE_DMA_BUF_EXPORT_INFO(info);
+
+	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+	if (!handle)
+		return ERR_PTR(-ENOMEM);
+
+	info.ops = &smaf_dma_buf_ops;
+	info.size = round_up(length, PAGE_SIZE);
+	info.flags = flags;
+	info.priv = handle;
+
+	handle->dmabuf = dma_buf_export(&info);
+	if (IS_ERR(handle->dmabuf)) {
+		kfree(handle);
+		return NULL;
+	}
+
+	handle->length = info.size;
+	handle->flags = flags;
+
+	return handle;
+}
+EXPORT_SYMBOL(smaf_create_handle);
+
+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;
+	}
+	case SMAF_IOC_GET_INFO:
+	{
+		struct smaf_info info;
+		struct smaf_allocator *alloc;
+
+		if (copy_from_user(&info, (void __user *)arg, _IOC_SIZE(cmd)))
+			return -EFAULT;
+
+		info.count = 0;
+		list_for_each_entry(alloc,  &smaf_dev.head, list_node) {
+			if (info.count++ == info.index)
+				strncpy(info.name, alloc->name,
+					ALLOCATOR_NAME_LENGTH);
+		}
+
+		if (info.index >= info.count)
+			return -EINVAL;
+
+		if (copy_to_user((void __user *)arg, &info, _IOC_SIZE(cmd)))
+			return -EFAULT;
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct file_operations smaf_fops = {
+	.owner = THIS_MODULE,
+	.unlocked_ioctl = smaf_ioctl,
+};
+
+static int __init smaf_init(void)
+{
+	int ret = 0;
+
+	smaf_dev.misc_dev.minor = MISC_DYNAMIC_MINOR;
+	smaf_dev.misc_dev.name  = "smaf";
+	smaf_dev.misc_dev.fops  = &smaf_fops;
+
+	/* register misc device */
+	ret = misc_register(&smaf_dev.misc_dev);
+	if (ret < 0)
+		return ret;
+
+	mutex_init(&smaf_dev.lock);
+	INIT_LIST_HEAD(&smaf_dev.head);
+
+	return ret;
+}
+module_init(smaf_init);
+
+static void __exit smaf_deinit(void)
+{
+	misc_deregister(&smaf_dev.misc_dev);
+}
+module_exit(smaf_deinit);
+
+MODULE_DESCRIPTION("Secure Memory Allocation Framework");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
diff --git a/include/linux/smaf-allocator.h b/include/linux/smaf-allocator.h
new file mode 100644
index 0000000..f764ef4
--- /dev/null
+++ b/include/linux/smaf-allocator.h
@@ -0,0 +1,54 @@
+/*
+ * smaf-allocator.h
+ *
+ * Copyright (C) Linaro SA 2015
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef _SMAF_ALLOCATOR_H_
+#define _SMAF_ALLOCATOR_H_
+
+#include <linux/dma-buf.h>
+#include <linux/list.h>
+
+/**
+ * struct smaf_allocator - implement dma_buf_ops like functions
+ *
+ * @match: match function to check if allocator can accept the devices
+ *	   attached to dmabuf
+ * @allocate: allocate memory with the given length and flags
+ *	      return a dma_buf handle
+ * @name: allocator name
+ * @ranking: allocator ranking (bigger is better)
+ */
+struct smaf_allocator {
+	struct list_head list_node;
+	bool (*match)(struct dma_buf *dmabuf);
+	struct dma_buf *(*allocate)(struct dma_buf *dmabuf,
+				    size_t length, unsigned int flags);
+	const char *name;
+	int ranking;
+};
+
+/**
+ * smaf_register_allocator - register an allocator to be used by SMAF
+ * @alloc: smaf_allocator structure
+ */
+int smaf_register_allocator(struct smaf_allocator *alloc);
+
+/**
+ * smaf_unregister_allocator - unregister alloctor
+ * @alloc: smaf_allocator structure
+ */
+void smaf_unregister_allocator(struct smaf_allocator *alloc);
+
+/**
+ * smaf_select_allocator_by_name - select an allocator by it name
+ * return 0 if the allocator has been found and selected.
+ * @dmabuf: dma_buf buffer handler
+ * @name: name of the allocator to be selected
+ */
+int smaf_select_allocator_by_name(struct dma_buf *dmabuf, char *name);
+
+#endif
diff --git a/include/linux/smaf-secure.h b/include/linux/smaf-secure.h
new file mode 100644
index 0000000..fffcde8
--- /dev/null
+++ b/include/linux/smaf-secure.h
@@ -0,0 +1,73 @@
+/*
+ * smaf-secure.h
+ *
+ * Copyright (C) Linaro SA 2015
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef _SMAF_SECURE_H_
+#define _SMAF_SECURE_H_
+
+#include <linux/dma-buf.h>
+#include <linux/dma-mapping.h>
+
+/**
+ * struct smaf_secure
+ * @create_ctx:		create a context for one dmabuf.
+ * @destroy_ctx:	destroy context.
+ * @grant_access:	check and provide access to memory area for a specific
+ *			device. Return true if the request is valid.
+ * @revoke_access:	remove device access rights.
+ */
+struct smaf_secure {
+	void *(*create_ctx)(void);
+	int (*destroy_ctx)(void *ctx);
+	bool (*grant_access)(void *ctx,
+			     struct device *dev,
+			     size_t addr, size_t size,
+			     enum dma_data_direction direction);
+	void (*revoke_access)(void *ctx,
+			      struct device *dev,
+			      size_t addr, size_t size,
+			      enum dma_data_direction direction);
+};
+
+/**
+ * smaf_register_secure - register secure module helper
+ * Secure module helper should be platform specific so only one can be
+ * registered.
+ *
+ * @sec: secure module to be registered
+ */
+int smaf_register_secure(struct smaf_secure *sec);
+
+/**
+ * smaf_unregister_secure - unregister secure module helper
+ */
+void smaf_unregister_secure(struct smaf_secure *sec);
+
+/**
+ * smaf_is_secure - test is a dma_buf handle has been secured by SMAF
+ * @dmabuf: dma_buf handle to be tested
+ */
+bool smaf_is_secure(struct dma_buf *dmabuf);
+
+/**
+ * smaf_set_secure - change dma_buf handle secure status
+ * @dmabuf: dma_buf handle to be change
+ * @secure: if true secure dma_buf handle
+ */
+int smaf_set_secure(struct dma_buf *dmabuf, bool secure);
+
+/**
+ * smaf_create_handle - create a smaf_handle with the give length and flags
+ * do not allocate memory but provide smaf_handle->dmabuf that can be
+ * shared between devices.
+ *
+ * @length: buffer size
+ * @flags: handle flags
+ */
+struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags);
+
+#endif
diff --git a/include/uapi/linux/smaf.h b/include/uapi/linux/smaf.h
new file mode 100644
index 0000000..5a9201b
--- /dev/null
+++ b/include/uapi/linux/smaf.h
@@ -0,0 +1,66 @@
+/*
+ * smaf.h
+ *
+ * Copyright (C) Linaro SA 2015
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef _UAPI_SMAF_H_
+#define _UAPI_SMAF_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+#define ALLOCATOR_NAME_LENGTH 64
+
+/**
+ * struct smaf_create_data - allocation parameters
+ * @length:	size of the allocation
+ * @flags:	flags passed to allocator
+ * @name:	name of the allocator to be selected, could be NULL
+ * @fd:		returned file descriptor
+ */
+struct smaf_create_data {
+	size_t length;
+	unsigned int flags;
+	char name[ALLOCATOR_NAME_LENGTH];
+	int fd;
+};
+
+/**
+ * struct smaf_secure_flag - set/get secure flag
+ * @fd:		file descriptor
+ * @secure:	secure flag value (set or get)
+ */
+struct smaf_secure_flag {
+	int fd;
+	int secure;
+};
+
+/**
+ * struct smaf_info - get registered allocator name per index
+ * @index:	allocator's index
+ * @count:	return number of registered allocators
+ * @name:	return allocator name
+ */
+struct smaf_info {
+	int index;
+	int count;
+	char name[ALLOCATOR_NAME_LENGTH];
+};
+
+#define SMAF_IOC_MAGIC	'S'
+
+#define SMAF_IOC_CREATE		 _IOWR(SMAF_IOC_MAGIC, 0, \
+				       struct smaf_create_data)
+
+#define SMAF_IOC_GET_SECURE_FLAG _IOWR(SMAF_IOC_MAGIC, 1, \
+				       struct smaf_secure_flag)
+
+#define SMAF_IOC_SET_SECURE_FLAG _IOWR(SMAF_IOC_MAGIC, 2, \
+				       struct smaf_secure_flag)
+
+#define SMAF_IOC_GET_INFO	 _IOWR(SMAF_IOC_MAGIC, 3, struct smaf_info)
+
+#endif
-- 
1.9.1

[PATCH v7 2/3] SMAF: add CMA allocator

[Benjamin Gaignard]

SMAF CMA allocator implement helpers functions to allow SMAF
to allocate contiguous memory.

match() each if at least one of the attached devices have coherent_dma_mask
set to DMA_BIT_MASK(32).

For allocation it use dma_alloc_attrs() with DMA_ATTR_WRITE_COMBINE and not
dma_alloc_writecombine to be compatible with ARM 64bits architecture

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/smaf/Kconfig    |   6 ++
 drivers/smaf/Makefile   |   1 +
 drivers/smaf/smaf-cma.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 drivers/smaf/smaf-cma.c

diff --git a/drivers/smaf/Kconfig b/drivers/smaf/Kconfig
index d36651a..058ec4c 100644
--- a/drivers/smaf/Kconfig
+++ b/drivers/smaf/Kconfig
@@ -3,3 +3,9 @@ config SMAF
 	depends on DMA_SHARED_BUFFER
 	help
 	  Choose this option to enable Secure Memory Allocation Framework
+
+config SMAF_CMA
+	tristate "SMAF CMA allocator"
+	depends on SMAF && HAVE_DMA_ATTRS
+	help
+	  Choose this option to enable CMA allocation within SMAF
diff --git a/drivers/smaf/Makefile b/drivers/smaf/Makefile
index 40cd882..05bab01b 100644
--- a/drivers/smaf/Makefile
+++ b/drivers/smaf/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_SMAF) += smaf-core.o
+obj-$(CONFIG_SMAF_CMA) += smaf-cma.o
diff --git a/drivers/smaf/smaf-cma.c b/drivers/smaf/smaf-cma.c
new file mode 100644
index 0000000..6366777
--- /dev/null
+++ b/drivers/smaf/smaf-cma.c
@@ -0,0 +1,199 @@
+/*
+ * smaf-cma.c
+ *
+ * Copyright (C) Linaro SA 2015
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/smaf-allocator.h>
+
+struct smaf_cma_buffer_info {
+	struct device *dev;
+	size_t size;
+	void *vaddr;
+	dma_addr_t paddr;
+};
+
+/**
+ * find_matching_device - iterate over the attached devices to find one
+ * with coherent_dma_mask correctly set to DMA_BIT_MASK(32).
+ * Matching device (if any) will be used to aim CMA area.
+ */
+static struct device *find_matching_device(struct dma_buf *dmabuf)
+{
+	struct dma_buf_attachment *attach_obj;
+
+	list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
+		if (attach_obj->dev->coherent_dma_mask == DMA_BIT_MASK(32))
+			return attach_obj->dev;
+	}
+
+	return NULL;
+}
+
+/**
+ * smaf_cma_match - return true if at least one device has been found
+ */
+static bool smaf_cma_match(struct dma_buf *dmabuf)
+{
+	return !!find_matching_device(dmabuf);
+}
+
+static void smaf_cma_release(struct dma_buf *dmabuf)
+{
+	struct smaf_cma_buffer_info *info = dmabuf->priv;
+	DEFINE_DMA_ATTRS(attrs);
+
+	dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
+
+	dma_free_attrs(info->dev, info->size, info->vaddr, info->paddr, &attrs);
+
+	kfree(info);
+}
+
+static struct sg_table *smaf_cma_map(struct dma_buf_attachment *attachment,
+				     enum dma_data_direction direction)
+{
+	struct smaf_cma_buffer_info *info = attachment->dmabuf->priv;
+	struct sg_table *sgt;
+	int ret;
+
+	sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
+	if (!sgt)
+		return NULL;
+
+	ret = dma_get_sgtable(info->dev, sgt, info->vaddr,
+			      info->paddr, info->size);
+	if (ret < 0)
+		goto out;
+
+	sg_dma_address(sgt->sgl) = info->paddr;
+	return sgt;
+
+out:
+	kfree(sgt);
+	return NULL;
+}
+
+static void smaf_cma_unmap(struct dma_buf_attachment *attachment,
+			   struct sg_table *sgt,
+			   enum dma_data_direction direction)
+{
+	/* do nothing */
+}
+
+static int smaf_cma_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
+{
+	struct smaf_cma_buffer_info *info = dmabuf->priv;
+	int ret;
+	DEFINE_DMA_ATTRS(attrs);
+
+	dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
+
+	if (info->size < vma->vm_end - vma->vm_start)
+		return -EINVAL;
+
+	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
+	ret = dma_mmap_attrs(info->dev, vma, info->vaddr, info->paddr,
+			     info->size, &attrs);
+
+	return ret;
+}
+
+static void *smaf_cma_vmap(struct dma_buf *dmabuf)
+{
+	struct smaf_cma_buffer_info *info = dmabuf->priv;
+
+	return info->vaddr;
+}
+
+static void *smaf_kmap_atomic(struct dma_buf *dmabuf, unsigned long offset)
+{
+	struct smaf_cma_buffer_info *info = dmabuf->priv;
+
+	return (void *)info->vaddr + offset;
+}
+
+static struct dma_buf_ops smaf_cma_ops = {
+	.map_dma_buf = smaf_cma_map,
+	.unmap_dma_buf = smaf_cma_unmap,
+	.mmap = smaf_cma_mmap,
+	.release = smaf_cma_release,
+	.kmap_atomic = smaf_kmap_atomic,
+	.kmap = smaf_kmap_atomic,
+	.vmap = smaf_cma_vmap,
+};
+
+static struct dma_buf *smaf_cma_allocate(struct dma_buf *dmabuf,
+					 size_t length, unsigned int flags)
+{
+	struct dma_buf_attachment *attach_obj;
+	struct smaf_cma_buffer_info *info;
+	struct dma_buf *cma_dmabuf;
+	int ret;
+
+	DEFINE_DMA_BUF_EXPORT_INFO(export);
+	DEFINE_DMA_ATTRS(attrs);
+
+	dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return NULL;
+
+	info->dev = find_matching_device(dmabuf);
+	info->size = length;
+	info->vaddr = dma_alloc_attrs(info->dev, info->size, &info->paddr,
+				      GFP_KERNEL | __GFP_NOWARN, &attrs);
+	if (!info->vaddr) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	export.ops = &smaf_cma_ops;
+	export.size = info->size;
+	export.flags = flags;
+	export.priv = info;
+
+	cma_dmabuf = dma_buf_export(&export);
+	if (IS_ERR(cma_dmabuf))
+		goto error;
+
+	list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
+		dma_buf_attach(cma_dmabuf, attach_obj->dev);
+	}
+
+	return cma_dmabuf;
+
+error:
+	kfree(info);
+	return NULL;
+}
+
+static struct smaf_allocator smaf_cma = {
+	.match = smaf_cma_match,
+	.allocate = smaf_cma_allocate,
+	.name = "smaf-cma",
+	.ranking = 0,
+};
+
+static int __init smaf_cma_init(void)
+{
+	INIT_LIST_HEAD(&smaf_cma.list_node);
+	return smaf_register_allocator(&smaf_cma);
+}
+module_init(smaf_cma_init);
+
+static void __exit smaf_cma_deinit(void)
+{
+	smaf_unregister_allocator(&smaf_cma);
+}
+module_exit(smaf_cma_deinit);
+
+MODULE_DESCRIPTION("SMAF CMA module");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
-- 
1.9.1

[PATCH v7 3/3] SMAF: add fake secure module

[Benjamin Gaignard]

This module is allow testing secure calls of SMAF.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/smaf/Kconfig           |  6 +++
 drivers/smaf/Makefile          |  1 +
 drivers/smaf/smaf-fakesecure.c | 85 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 drivers/smaf/smaf-fakesecure.c

diff --git a/drivers/smaf/Kconfig b/drivers/smaf/Kconfig
index 058ec4c..fd17005 100644
--- a/drivers/smaf/Kconfig
+++ b/drivers/smaf/Kconfig
@@ -9,3 +9,9 @@ config SMAF_CMA
 	depends on SMAF && HAVE_DMA_ATTRS
 	help
 	  Choose this option to enable CMA allocation within SMAF
+
+config SMAF_FAKE_SECURE
+	tristate "SMAF fake secure module"
+	depends on SMAF
+	help
+	  Choose this option to enable fake secure module for test purpose
diff --git a/drivers/smaf/Makefile b/drivers/smaf/Makefile
index 05bab01b..00d5cd4 100644
--- a/drivers/smaf/Makefile
+++ b/drivers/smaf/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_SMAF) += smaf-core.o
 obj-$(CONFIG_SMAF_CMA) += smaf-cma.o
+obj-$(CONFIG_SMAF_FAKE_SECURE) += smaf-fakesecure.o
diff --git a/drivers/smaf/smaf-fakesecure.c b/drivers/smaf/smaf-fakesecure.c
new file mode 100644
index 0000000..5f3d355
--- /dev/null
+++ b/drivers/smaf/smaf-fakesecure.c
@@ -0,0 +1,85 @@
+/*
+ * smaf-fakesecure.c
+ *
+ * Copyright (C) Linaro SA 2015
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/smaf-secure.h>
+
+#define MAGIC 0xDEADBEEF
+
+struct fake_private {
+	int magic;
+};
+
+static void *smaf_fakesecure_create(void)
+{
+	struct fake_private *priv;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv->magic = MAGIC;
+
+	return priv;
+}
+
+static int smaf_fakesecure_destroy(void *ctx)
+{
+	struct fake_private *priv = (struct fake_private *)ctx;
+
+	WARN_ON(!priv || (priv->magic != MAGIC));
+	kfree(priv);
+
+	return 0;
+}
+
+static bool smaf_fakesecure_grant_access(void *ctx,
+					 struct device *dev,
+					 size_t addr, size_t size,
+					 enum dma_data_direction direction)
+{
+	struct fake_private *priv = (struct fake_private *)ctx;
+
+	WARN_ON(!priv || (priv->magic != MAGIC));
+	pr_debug("grant requested by device %s\n",
+		 dev->driver ? dev->driver->name : "cpu");
+
+	return priv->magic == MAGIC;
+}
+
+static void smaf_fakesecure_revoke_access(void *ctx,
+					  struct device *dev,
+					  size_t addr, size_t size,
+					  enum dma_data_direction direction)
+{
+	struct fake_private *priv = (struct fake_private *)ctx;
+
+	WARN_ON(!priv || (priv->magic != MAGIC));
+	pr_debug("revoke requested by device %s\n",
+		 dev->driver ? dev->driver->name : "cpu");
+}
+
+static struct smaf_secure fake = {
+	.create_ctx = smaf_fakesecure_create,
+	.destroy_ctx = smaf_fakesecure_destroy,
+	.grant_access = smaf_fakesecure_grant_access,
+	.revoke_access = smaf_fakesecure_revoke_access,
+};
+
+static int __init smaf_fakesecure_init(void)
+{
+	return smaf_register_secure(&fake);
+}
+module_init(smaf_fakesecure_init);
+
+static void __exit smaf_fakesecure_deinit(void)
+{
+	smaf_unregister_secure(&fake);
+}
+module_exit(smaf_fakesecure_deinit);
+
+MODULE_DESCRIPTION("SMAF fake secure module for test purpose");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
-- 
1.9.1

Re: [PATCH v7 1/3] create SMAF module

[Emil Velikov]

 Hi Benjamin,

I'd suspect you're interested in some feedback on these, so here is a few :-)
Sadly (ideally?) nothing serious, but a bunch minor suggestions, plus
the odd bug.

On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:

> --- /dev/null
> +++ b/drivers/smaf/smaf-core.c
> @@ -0,0 +1,794 @@
> +/*
> + * smaf.c
The comment does not match the actual file name.

You could give a brief summary of the file(s), if you're feeling gracious ;-)


> +
> +/**
> + * smaf_grant_access - return true if the specified device can get access
> + * to the memory area
> + *
Reading this makes me wonder if {request,allow}_access won't be better name ?


> +static int smaf_secure_handle(struct smaf_handle *handle)
> +{
> +       if (atomic_read(&handle->is_secure))
> +               return 0;
> +
> +       if (!have_secure_module())
> +               return -EINVAL;
> +
> +       handle->secure_ctx = smaf_dev.secure->create_ctx();
> +
Should one use a temporary variable so that the caller provided
storage is unchanged in case of an error ?

> +       if (!handle->secure_ctx)
> +               return -EINVAL;
> +
> +       atomic_set(&handle->is_secure, 1);
> +       return 0;
> +}
> +


> +int smaf_register_secure(struct smaf_secure *s)
> +{
> +       /* make sure that secure module have all required functions
> +        * to avoid test them each time later
> +        */
> +       WARN_ON(!s || !s->create_ctx || !s->destroy_ctx ||
> +               !s->grant_access || !s->revoke_access);
> +
Is something like below reasonable thing to do in the kernel ?
Same question goes for smaf_register_allocator() further down.

if (!s || ....)
  return -ESHOULDNEVERHAPPEN;



> +static struct vm_operations_struct smaf_vma_ops = {
Ops/vfucs normally are const data. Is there something preventing us here ?

> +       .close = smaf_vm_close,
> +};
> +
> +static int smaf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> +{
> +       struct smaf_handle *handle = dmabuf->priv;
> +       bool ret;
> +       enum dma_data_direction dir;
> +
> +       /* if no allocator attached, get the first allocator */
> +       if (!handle->allocator) {
> +               struct smaf_allocator *alloc;
> +
> +               mutex_lock(&smaf_dev.lock);
> +               alloc = smaf_get_first_allocator(dmabuf);
> +               mutex_unlock(&smaf_dev.lock);
> +
> +               /* still no allocator ? */
> +               if (!alloc)
> +                       return -EINVAL;
> +
> +               handle->allocator = alloc;
> +       }
> +
> +       if (!handle->db_alloc) {
> +               struct dma_buf *db_alloc;
> +
> +               db_alloc = handle->allocator->allocate(dmabuf,
> +                                                      handle->length,
> +                                                      handle->flags);
> +               if (!db_alloc)
> +                       return -EINVAL;
> +
> +               handle->db_alloc = db_alloc;
> +       }
> +
The above half of the function looks identical to smaf_map_dma_buf().
Worth factoring it out to a helper function ?


> +static int smaf_attach(struct dma_buf *dmabuf, struct device *dev,
> +                      struct dma_buf_attachment *attach)
> +{
> +       struct smaf_handle *handle = dmabuf->priv;
> +       struct dma_buf_attachment *db_attach;
> +
> +       if (!handle->db_alloc)
> +               return 0;
> +
Shouldn't one return an error (-EINVAL or similar) here ?


> +static struct dma_buf_ops smaf_dma_buf_ops = {
const ? From a very quick look the compiler should warn us about it -
"smaf_dma_buf_ops discards const qualifier" or similar.


> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags)
> +{
> +       struct smaf_handle *handle;
> +
> +       DEFINE_DMA_BUF_EXPORT_INFO(info);
> +
> +       handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> +       if (!handle)
> +               return ERR_PTR(-ENOMEM);
> +
Err this should be return NULL; correct ?

> +       info.ops = &smaf_dma_buf_ops;
> +       info.size = round_up(length, PAGE_SIZE);
> +       info.flags = flags;
> +       info.priv = handle;
> +
> +       handle->dmabuf = dma_buf_export(&info);
> +       if (IS_ERR(handle->dmabuf)) {
> +               kfree(handle);
> +               return NULL;
> +       }
> +
> +       handle->length = info.size;
> +       handle->flags = flags;
> +
> +       return handle;
> +}
> +EXPORT_SYMBOL(smaf_create_handle);
> +
> +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);
We want to sanitise the input data.{length,flags} before sending it
deeper in the kernel.

> +               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);
Missing free(handle), here and through the rest of the case statement ?

> +                               return -EINVAL;
> +                       }
> +               }
> +
> +               handle->fd = dma_buf_fd(handle->dmabuf, data.flags);
> +               if (handle->fd < 0) {
Worth adding smaf_DEselect_allocator_by_name() and using it here + below ?

> +                       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);
Worth adding if (data.fd == -1) return -EINVAL; ?



> +
> +static const struct file_operations smaf_fops = {
> +       .owner = THIS_MODULE,
There was a recent 'crusade' to get rid of these. Are you sure we
want/need this ?

> +       .unlocked_ioctl = smaf_ioctl,
> +};
> +
> +static int __init smaf_init(void)
> +{
> +       int ret = 0;
> +
Please drop the default initialization.

> +       smaf_dev.misc_dev.minor = MISC_DYNAMIC_MINOR;
> +       smaf_dev.misc_dev.name  = "smaf";
> +       smaf_dev.misc_dev.fops  = &smaf_fops;
> +
Initialize the global static variable (smaf_dev) upon declaration ?


> --- /dev/null
> +++ b/include/linux/smaf-secure.h

> +/**
> + * smaf_create_handle - create a smaf_handle with the give length and flags
> + * do not allocate memory but provide smaf_handle->dmabuf that can be
> + * shared between devices.
> + *
> + * @length: buffer size
> + * @flags: handle flags
> + */
> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags);
> +
Inspired by the bug (?) in this function I think you want to document
the return value throughout the header.


> +#endif
> diff --git a/include/uapi/linux/smaf.h b/include/uapi/linux/smaf.h
> new file mode 100644
> index 0000000..5a9201b
> --- /dev/null
> +++ b/include/uapi/linux/smaf.h
> @@ -0,0 +1,66 @@
> +/*
> + * smaf.h
> + *
Would be nice if we had more elaborate comment in an UAPI header.


> +/**
> + * struct smaf_create_data - allocation parameters
> + * @length:    size of the allocation
> + * @flags:     flags passed to allocator
> + * @name:      name of the allocator to be selected, could be NULL
Is it guaranteed to be null terminated ? If so one should mentioned it
otherwise your userspace should be fixed.
Same comments apply for smaf_info::name.


> + * @fd:                returned file descriptor
> + */
> +struct smaf_create_data {
> +       size_t length;
> +       unsigned int flags;
> +       char name[ALLOCATOR_NAME_LENGTH];
> +       int fd;
The structs here feels quite fragile. Please read up on Daniel
Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
useful is such process.

Hopefully I haven't lost the plot with the above, if I had don't be
shy to point out.

Thanks,
Emil

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt

Re: [PATCH v7 2/3] SMAF: add CMA allocator

[Emil Velikov]

Hi Benjamin,

On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> SMAF CMA allocator implement helpers functions to allow SMAF
> to allocate contiguous memory.
>
> match() each if at least one of the attached devices have coherent_dma_mask
> set to DMA_BIT_MASK(32).
>
What is the idea behind the hardcoded 32. Wouldn't it be better to avoid that ?


> +static void smaf_cma_release(struct dma_buf *dmabuf)
> +{
> +       struct smaf_cma_buffer_info *info = dmabuf->priv;
> +       DEFINE_DMA_ATTRS(attrs);
> +
> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
Imho it's worth storing the dma_attrs within smaf_cma_buffer_info.
This way it's less likely for things to go wrong, if one forgets to
update one of the three in the future.


> +static void smaf_cma_unmap(struct dma_buf_attachment *attachment,
> +                          struct sg_table *sgt,
> +                          enum dma_data_direction direction)
> +{
> +       /* do nothing */
There could/should really be a comment explaining why we "do nothing"
here, right ?

> +}
> +
> +static int smaf_cma_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> +{
> +       struct smaf_cma_buffer_info *info = dmabuf->priv;
> +       int ret;
> +       DEFINE_DMA_ATTRS(attrs);
> +
> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
> +       if (info->size < vma->vm_end - vma->vm_start)
> +               return -EINVAL;
> +
> +       vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> +       ret = dma_mmap_attrs(info->dev, vma, info->vaddr, info->paddr,
> +                            info->size, &attrs);
> +
> +       return ret;
Kill the temporary variable 'ret' ?


> +static struct dma_buf_ops smaf_cma_ops = {
const ? Afaict the compiler would/should warn you about discarding it
as the ops are defined const.


> +static struct dma_buf *smaf_cma_allocate(struct dma_buf *dmabuf,
> +                                        size_t length, unsigned int flags)
> +{
> +       struct dma_buf_attachment *attach_obj;
> +       struct smaf_cma_buffer_info *info;
> +       struct dma_buf *cma_dmabuf;
> +       int ret;
> +
> +       DEFINE_DMA_BUF_EXPORT_INFO(export);
> +       DEFINE_DMA_ATTRS(attrs);
> +
> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
> +       info = kzalloc(sizeof(*info), GFP_KERNEL);
> +       if (!info)
> +               return NULL;
> +
> +       info->dev = find_matching_device(dmabuf);
find_matching_device() can return NULL. We should handle that imho.

> +       info->size = length;
> +       info->vaddr = dma_alloc_attrs(info->dev, info->size, &info->paddr,
> +                                     GFP_KERNEL | __GFP_NOWARN, &attrs);
> +       if (!info->vaddr) {
> +               ret = -ENOMEM;
set-but-unused-variable 'ret' ?

> +               goto error;
> +       }
> +
> +       export.ops = &smaf_cma_ops;
> +       export.size = info->size;
> +       export.flags = flags;
> +       export.priv = info;
> +
> +       cma_dmabuf = dma_buf_export(&export);
> +       if (IS_ERR(cma_dmabuf))
Missing dma_free_attrs() ? I'd add another label in the error path and
handle it there.

> +               goto error;
> +
> +       list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
> +               dma_buf_attach(cma_dmabuf, attach_obj->dev);
Imho one should error out if attach fails. Or warn at the very least ?


> +static int __init smaf_cma_init(void)
> +{
> +       INIT_LIST_HEAD(&smaf_cma.list_node);
Isn't this something that smaf_register_allocator() should be doing ?


Regards,
Emil

Re: [PATCH v7 3/3] SMAF: add fake secure module

[Emil Velikov]

Hi Benjamin,

On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> This module is allow testing secure calls of SMAF.
>
"Add fake secure module" does sound like something not (m)any people
want to hear ;-)
Have you considered calling it 'dummy', 'test' or similar ?


> --- /dev/null
> +++ b/drivers/smaf/smaf-fakesecure.c
> @@ -0,0 +1,85 @@
> +/*
> + * smaf-fakesecure.c
> + *
> + * Copyright (C) Linaro SA 2015
> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/smaf-secure.h>
> +
> +#define MAGIC 0xDEADBEEF
> +
> +struct fake_private {
> +       int magic;
> +};
> +
> +static void *smaf_fakesecure_create(void)
> +{
> +       struct fake_private *priv;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
Missing ENOMEM handling ?

> +       priv->magic = MAGIC;
> +
> +       return priv;
> +}
> +
> +static int smaf_fakesecure_destroy(void *ctx)
> +{
> +       struct fake_private *priv = (struct fake_private *)ctx;
You might want to flesh this cast into a (inline) helper and use it throughout ?


... and that is all. Hope these were useful, or at the very least not
utterly wrong, suggestions :-)


Regards,
Emil

P.S. From a quick look userspace has some subtle bugs/odd practises.
Let me know if you're interested in my input.

Re: [PATCH v7 1/3] create SMAF module

[Benjamin Gaignard]

Hello Emil,

thanks for your review.
I have understand most of your remarks and I'm fixing them
but some points aren't obvious for me...

2016-05-17 0:58 GMT+02:00 Emil Velikov <emil.l.velikov@gmail.com>:
>  Hi Benjamin,
>
> I'd suspect you're interested in some feedback on these, so here is a few :-)
> Sadly (ideally?) nothing serious, but a bunch minor suggestions, plus
> the odd bug.
>
> On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
>
>> --- /dev/null
>> +++ b/drivers/smaf/smaf-core.c
>> @@ -0,0 +1,794 @@
>> +/*
>> + * smaf.c
> The comment does not match the actual file name.
>
> You could give a brief summary of the file(s), if you're feeling gracious ;-)
>
>
>> +
>> +/**
>> + * smaf_grant_access - return true if the specified device can get access
>> + * to the memory area
>> + *
> Reading this makes me wonder if {request,allow}_access won't be better name ?
>

grant and revoke sound more secure oriented for me but that could change

>
>> +static int smaf_secure_handle(struct smaf_handle *handle)
>> +{
>> +       if (atomic_read(&handle->is_secure))
>> +               return 0;
>> +
>> +       if (!have_secure_module())
>> +               return -EINVAL;
>> +
>> +       handle->secure_ctx = smaf_dev.secure->create_ctx();
>> +
> Should one use a temporary variable so that the caller provided
> storage is unchanged in case of an error ?
>
>> +       if (!handle->secure_ctx)
>> +               return -EINVAL;
>> +
>> +       atomic_set(&handle->is_secure, 1);
>> +       return 0;
>> +}
>> +
>
>
>> +int smaf_register_secure(struct smaf_secure *s)
>> +{
>> +       /* make sure that secure module have all required functions
>> +        * to avoid test them each time later
>> +        */
>> +       WARN_ON(!s || !s->create_ctx || !s->destroy_ctx ||
>> +               !s->grant_access || !s->revoke_access);
>> +
> Is something like below reasonable thing to do in the kernel ?
> Same question goes for smaf_register_allocator() further down.
>
> if (!s || ....)
>   return -ESHOULDNEVERHAPPEN;
>
>
>
>> +static struct vm_operations_struct smaf_vma_ops = {
> Ops/vfucs normally are const data. Is there something preventing us here ?
>
>> +       .close = smaf_vm_close,
>> +};
>> +
>> +static int smaf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
>> +{
>> +       struct smaf_handle *handle = dmabuf->priv;
>> +       bool ret;
>> +       enum dma_data_direction dir;
>> +
>> +       /* if no allocator attached, get the first allocator */
>> +       if (!handle->allocator) {
>> +               struct smaf_allocator *alloc;
>> +
>> +               mutex_lock(&smaf_dev.lock);
>> +               alloc = smaf_get_first_allocator(dmabuf);
>> +               mutex_unlock(&smaf_dev.lock);
>> +
>> +               /* still no allocator ? */
>> +               if (!alloc)
>> +                       return -EINVAL;
>> +
>> +               handle->allocator = alloc;
>> +       }
>> +
>> +       if (!handle->db_alloc) {
>> +               struct dma_buf *db_alloc;
>> +
>> +               db_alloc = handle->allocator->allocate(dmabuf,
>> +                                                      handle->length,
>> +                                                      handle->flags);
>> +               if (!db_alloc)
>> +                       return -EINVAL;
>> +
>> +               handle->db_alloc = db_alloc;
>> +       }
>> +
> The above half of the function looks identical to smaf_map_dma_buf().
> Worth factoring it out to a helper function ?
>
>
>> +static int smaf_attach(struct dma_buf *dmabuf, struct device *dev,
>> +                      struct dma_buf_attachment *attach)
>> +{
>> +       struct smaf_handle *handle = dmabuf->priv;
>> +       struct dma_buf_attachment *db_attach;
>> +
>> +       if (!handle->db_alloc)
>> +               return 0;
>> +
> Shouldn't one return an error (-EINVAL or similar) here ?

No because a device could attach itself on the buffer and the
allocator will only
be selected at the first map_attach call.
The goal is to delay the allocation until all devices are attached to
select the best allocator.

>
>
>> +static struct dma_buf_ops smaf_dma_buf_ops = {
> const ? From a very quick look the compiler should warn us about it -
> "smaf_dma_buf_ops discards const qualifier" or similar.
>
>
>> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags)
>> +{
>> +       struct smaf_handle *handle;
>> +
>> +       DEFINE_DMA_BUF_EXPORT_INFO(info);
>> +
>> +       handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>> +       if (!handle)
>> +               return ERR_PTR(-ENOMEM);
>> +
> Err this should be return NULL; correct ?
>
>> +       info.ops = &smaf_dma_buf_ops;
>> +       info.size = round_up(length, PAGE_SIZE);
>> +       info.flags = flags;
>> +       info.priv = handle;
>> +
>> +       handle->dmabuf = dma_buf_export(&info);
>> +       if (IS_ERR(handle->dmabuf)) {
>> +               kfree(handle);
>> +               return NULL;
>> +       }
>> +
>> +       handle->length = info.size;
>> +       handle->flags = flags;
>> +
>> +       return handle;
>> +}
>> +EXPORT_SYMBOL(smaf_create_handle);
>> +
>> +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);
> We want to sanitise the input data.{length,flags} before sending it
> deeper in the kernel.

Sorry but can you elaborate little more here ?
I don't understand your expectations.

>
>> +               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);
> Missing free(handle), here and through the rest of the case statement ?
>
>> +                               return -EINVAL;
>> +                       }
>> +               }
>> +
>> +               handle->fd = dma_buf_fd(handle->dmabuf, data.flags);
>> +               if (handle->fd < 0) {
> Worth adding smaf_DEselect_allocator_by_name() and using it here + below ?
>
>> +                       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);
> Worth adding if (data.fd == -1) return -EINVAL; ?
>
>
>
>> +
>> +static const struct file_operations smaf_fops = {
>> +       .owner = THIS_MODULE,
> There was a recent 'crusade' to get rid of these. Are you sure we
> want/need this ?
>
>> +       .unlocked_ioctl = smaf_ioctl,
>> +};
>> +
>> +static int __init smaf_init(void)
>> +{
>> +       int ret = 0;
>> +
> Please drop the default initialization.
>
>> +       smaf_dev.misc_dev.minor = MISC_DYNAMIC_MINOR;
>> +       smaf_dev.misc_dev.name  = "smaf";
>> +       smaf_dev.misc_dev.fops  = &smaf_fops;
>> +
> Initialize the global static variable (smaf_dev) upon declaration ?
>
>
>> --- /dev/null
>> +++ b/include/linux/smaf-secure.h
>
>> +/**
>> + * smaf_create_handle - create a smaf_handle with the give length and flags
>> + * do not allocate memory but provide smaf_handle->dmabuf that can be
>> + * shared between devices.
>> + *
>> + * @length: buffer size
>> + * @flags: handle flags
>> + */
>> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags);
>> +
> Inspired by the bug (?) in this function I think you want to document
> the return value throughout the header.

It is useless the add this function in this .h file I will remove it
and fix the comment in structure defintion
>
>> +#endif
>> diff --git a/include/uapi/linux/smaf.h b/include/uapi/linux/smaf.h
>> new file mode 100644
>> index 0000000..5a9201b
>> --- /dev/null
>> +++ b/include/uapi/linux/smaf.h
>> @@ -0,0 +1,66 @@
>> +/*
>> + * smaf.h
>> + *
> Would be nice if we had more elaborate comment in an UAPI header.
>
>
>> +/**
>> + * struct smaf_create_data - allocation parameters
>> + * @length:    size of the allocation
>> + * @flags:     flags passed to allocator
>> + * @name:      name of the allocator to be selected, could be NULL
> Is it guaranteed to be null terminated ? If so one should mentioned it
> otherwise your userspace should be fixed.
> Same comments apply for smaf_info::name.

I have used strncpy everywhere to avoid this problem but maybe it is not enough

>
>
>> + * @fd:                returned file descriptor
>> + */
>> +struct smaf_create_data {
>> +       size_t length;
>> +       unsigned int flags;
>> +       char name[ALLOCATOR_NAME_LENGTH];
>> +       int fd;
> The structs here feels quite fragile. Please read up on Daniel
> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
> useful is such process.
>
if I describe the structures like this:
/**
 * struct smaf_create_data - allocation parameters
 * @length: size of the allocation
 * @flags: flags passed to allocator
 * @name_len: length of name
 * @name: name of the allocator to be selected, could be NULL
 * @fd: returned file descriptor
 */
struct smaf_create_data {
size_t length;
unsigned int flags;
size_t name_len;
char __user *name;
int fd;
char padding[44];
};

does it sound more robust for you ?

> Hopefully I haven't lost the plot with the above, if I had don't be
> shy to point out.
>
> Thanks,
> Emil
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt



-- 
Benjamin Gaignard

Graphic Working Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 2/3] SMAF: add CMA allocator

[Benjamin Gaignard]

Hi Emil,

2016-05-17 1:05 GMT+02:00 Emil Velikov <emil.l.velikov@gmail.com>:
> Hi Benjamin,
>
> On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
>> SMAF CMA allocator implement helpers functions to allow SMAF
>> to allocate contiguous memory.
>>
>> match() each if at least one of the attached devices have coherent_dma_mask
>> set to DMA_BIT_MASK(32).
>>
> What is the idea behind the hardcoded 32. Wouldn't it be better to avoid that ?
>

Device dma_bit_mask field has to be set to DMA_BIT_MASK(32) to target
a CMA area.
I haven't see any other #define for that.

>
>> +static void smaf_cma_release(struct dma_buf *dmabuf)
>> +{
>> +       struct smaf_cma_buffer_info *info = dmabuf->priv;
>> +       DEFINE_DMA_ATTRS(attrs);
>> +
>> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
>> +
> Imho it's worth storing the dma_attrs within smaf_cma_buffer_info.
> This way it's less likely for things to go wrong, if one forgets to
> update one of the three in the future.

Here I have duplicate what is done everywhere else but I could try to
add it into
smaf_cma_buffer_info structure.

>
>> +static void smaf_cma_unmap(struct dma_buf_attachment *attachment,
>> +                          struct sg_table *sgt,
>> +                          enum dma_data_direction direction)
>> +{
>> +       /* do nothing */
> There could/should really be a comment explaining why we "do nothing"
> here, right ?
>

I haven't used DMA_ATTR_NO_KERNEL_MAPPING while allocating the buffer
so kernel mapping is set by default and I don't have to manage map refcounting.

>> +}
>> +
>> +static int smaf_cma_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
>> +{
>> +       struct smaf_cma_buffer_info *info = dmabuf->priv;
>> +       int ret;
>> +       DEFINE_DMA_ATTRS(attrs);
>> +
>> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
>> +
>> +       if (info->size < vma->vm_end - vma->vm_start)
>> +               return -EINVAL;
>> +
>> +       vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>> +       ret = dma_mmap_attrs(info->dev, vma, info->vaddr, info->paddr,
>> +                            info->size, &attrs);
>> +
>> +       return ret;
> Kill the temporary variable 'ret' ?

sure
>
>
>> +static struct dma_buf_ops smaf_cma_ops = {
> const ? Afaict the compiler would/should warn you about discarding it
> as the ops are defined const.
>
>
>> +static struct dma_buf *smaf_cma_allocate(struct dma_buf *dmabuf,
>> +                                        size_t length, unsigned int flags)
>> +{
>> +       struct dma_buf_attachment *attach_obj;
>> +       struct smaf_cma_buffer_info *info;
>> +       struct dma_buf *cma_dmabuf;
>> +       int ret;
>> +
>> +       DEFINE_DMA_BUF_EXPORT_INFO(export);
>> +       DEFINE_DMA_ATTRS(attrs);
>> +
>> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
>> +
>> +       info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +       if (!info)
>> +               return NULL;
>> +
>> +       info->dev = find_matching_device(dmabuf);
> find_matching_device() can return NULL. We should handle that imho.
>

If the returned device have an associated CMA area then it will use it else
if dev have not CMA area or if find_matching_device() return NULL
the default CMA area will be used

>> +       info->size = length;
>> +       info->vaddr = dma_alloc_attrs(info->dev, info->size, &info->paddr,
>> +                                     GFP_KERNEL | __GFP_NOWARN, &attrs);
>> +       if (!info->vaddr) {
>> +               ret = -ENOMEM;
> set-but-unused-variable 'ret' ?
>
I will remove it

>> +               goto error;
>> +       }
>> +
>> +       export.ops = &smaf_cma_ops;
>> +       export.size = info->size;
>> +       export.flags = flags;
>> +       export.priv = info;
>> +
>> +       cma_dmabuf = dma_buf_export(&export);
>> +       if (IS_ERR(cma_dmabuf))
> Missing dma_free_attrs() ? I'd add another label in the error path and
> handle it there.
>
OK

>> +               goto error;
>> +
>> +       list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
>> +               dma_buf_attach(cma_dmabuf, attach_obj->dev);
> Imho one should error out if attach fails. Or warn at the very least ?
>
>
>> +static int __init smaf_cma_init(void)
>> +{
>> +       INIT_LIST_HEAD(&smaf_cma.list_node);
> Isn't this something that smaf_register_allocator() should be doing ?
>

Yes for sure

>
> Regards,
> Emil



-- 
Benjamin Gaignard

Graphic Working Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 3/3] SMAF: add fake secure module

[Benjamin Gaignard]

2016-05-17 1:10 GMT+02:00 Emil Velikov <emil.l.velikov@gmail.com>:
> Hi Benjamin,
>
> On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
>> This module is allow testing secure calls of SMAF.
>>
> "Add fake secure module" does sound like something not (m)any people
> want to hear ;-)
> Have you considered calling it 'dummy', 'test' or similar ?

"test" is better name, I will change to that

>
>
>> --- /dev/null
>> +++ b/drivers/smaf/smaf-fakesecure.c
>> @@ -0,0 +1,85 @@
>> +/*
>> + * smaf-fakesecure.c
>> + *
>> + * Copyright (C) Linaro SA 2015
>> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/smaf-secure.h>
>> +
>> +#define MAGIC 0xDEADBEEF
>> +
>> +struct fake_private {
>> +       int magic;
>> +};
>> +
>> +static void *smaf_fakesecure_create(void)
>> +{
>> +       struct fake_private *priv;
>> +
>> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> Missing ENOMEM handling ?
>
>> +       priv->magic = MAGIC;
>> +
>> +       return priv;
>> +}
>> +
>> +static int smaf_fakesecure_destroy(void *ctx)
>> +{
>> +       struct fake_private *priv = (struct fake_private *)ctx;
> You might want to flesh this cast into a (inline) helper and use it throughout ?
>
>
> ... and that is all. Hope these were useful, or at the very least not
> utterly wrong, suggestions :-)
>
>
> Regards,
> Emil
>
> P.S. From a quick look userspace has some subtle bugs/odd practises.
> Let me know if you're interested in my input.

Your advices are welcome for userspace too

Thanks
Benjamin



-- 
Benjamin Gaignard

Graphic Working Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 1/3] create SMAF module

[Daniel Vetter]

On Tue, May 17, 2016 at 03:50:41PM +0200, Benjamin Gaignard wrote:
> 2016-05-17 0:58 GMT+02:00 Emil Velikov <emil.l.velikov@gmail.com>:
> > On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> >> + * @fd:                returned file descriptor
> >> + */
> >> +struct smaf_create_data {
> >> +       size_t length;
> >> +       unsigned int flags;
> >> +       char name[ALLOCATOR_NAME_LENGTH];
> >> +       int fd;
> > The structs here feels quite fragile. Please read up on Daniel
> > Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
> > useful is such process.
> >
> if I describe the structures like this:
> /**
>  * struct smaf_create_data - allocation parameters
>  * @length: size of the allocation
>  * @flags: flags passed to allocator
>  * @name_len: length of name
>  * @name: name of the allocator to be selected, could be NULL
>  * @fd: returned file descriptor
>  */
> struct smaf_create_data {
> size_t length;
> unsigned int flags;
> size_t name_len;
> char __user *name;
> int fd;
> char padding[44];
> };
> 
> does it sound more robust for you ?
> 
> > Hopefully I haven't lost the plot with the above, if I had don't be
> > shy to point out.
> >
> > Thanks,
> > Emil
> >
> > [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt

Please read this doc in it's entirety, ask on irc if you don't get certain
parts. Then come back an rework your patch.

Super short summary: _All_ the types you've used are absolute no-go in
ioctl structs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 1/3] create SMAF module

[Emil Velikov]

On 17 May 2016 at 14:50, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> Hello Emil,
>
> thanks for your review.
> I have understand most of your remarks and I'm fixing them
> but some points aren't obvious for me...
>
Sure thing. Thanks for being honest.

>
> No because a device could attach itself on the buffer and the
> allocator will only
> be selected at the first map_attach call.
> The goal is to delay the allocation until all devices are attached to
> select the best allocator.
>
I see. Makes sense.


>>> +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);
>> We want to sanitise the input data.{length,flags} before sending it
>> deeper in the kernel.
>
> Sorry but can you elaborate little more here ?
> I don't understand your expectations.
>
You want to determine which flags are 'considered useful' at this
stage, and reject anything else. As-is you inject any flags that the
user gives you directly into the 'guts' of the kernel. This is not
good from security and future expandability POV.

About the length you want a similar thing. size_t is unsigned (great),
although ideally you'll want to check/determine if one cannot exploit
it, integer overflow being the more common suspect. This may be quite
hard to track, so I'd stick with the flags checking at least.


> It is useless the add this function in this .h file I will remove it
> and fix the comment in structure defintion

I've seen both approaches - description next to the declaration or
definition. I'd rather not pick sides, as people might throw rotten
fruit at me ;-)


>>> +/**
>>> + * struct smaf_create_data - allocation parameters
>>> + * @length:    size of the allocation
>>> + * @flags:     flags passed to allocator
>>> + * @name:      name of the allocator to be selected, could be NULL
Just occurred to - you might want to comment what the user should
expect if NULL. Any at random one will be used or otherwise. Very
quick description on the heuristics used might be good as well.

>> Is it guaranteed to be null terminated ? If so one should mentioned it
>> otherwise your userspace should be fixed.
>> Same comments apply for smaf_info::name.
>
> I have used strncpy everywhere to avoid this problem but maybe it is not enough
>
According to the man page

"The strncpy() function is similar, except that at most n bytes of src
are copied.  Warning: If there is no null byte among the first n bytes
of src, the string placed in dest will _not_ be null-terminated."

Annotation is mine obviously. I believe that after the strncpy 'name'
is/was assumed (used as) a NULL terminated string.

>>
>>
>>> + * @fd:                returned file descriptor
>>> + */
>>> +struct smaf_create_data {
>>> +       size_t length;
>>> +       unsigned int flags;
>>> +       char name[ALLOCATOR_NAME_LENGTH];
>>> +       int fd;
>> The structs here feels quite fragile. Please read up on Daniel
>> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
>> useful is such process.
>>
> if I describe the structures like this:
> /**
>  * struct smaf_create_data - allocation parameters
>  * @length: size of the allocation
>  * @flags: flags passed to allocator
>  * @name_len: length of name
>  * @name: name of the allocator to be selected, could be NULL
>  * @fd: returned file descriptor
>  */
> struct smaf_create_data {
> size_t length;
> unsigned int flags;
> size_t name_len;
> char __user *name;
> int fd;
> char padding[44];
> };
>
> does it sound more robust for you ?
>
Seems like you changed 'name' from fixed size array to char *. Which
actually gets us slightly further away from robust.

As Daniel said, please read through the hole file.

Here is a (slightly incomplete) gist of it all:
- you want to to use __[us]{8,16,32,64} and __kernel_size_t types everywhere
- each member of the struct must be the same offset for both 32bit and
64bit builds. ^^ helps with that
- double check for gaps in the struct - I think you have a few
- each struct should have it's old 'flags' which you'll use to
indicate the capability of the ioctl and thus the size of struct used.
i.e. it's for future use.

Obviously the other two structs need similar polish.

Here is how you can check things with pahole:
 - Create a simple C file that includes the header and has an instance
of each struct - it doesn't have to be use any of them.
 - Compile for 32 and 64 bit with -g -O0. Compare the struct layout -
it should be identical in both cases.

Hope that makes things a bit clearer.

Emil

Re: [PATCH v7 1/3] create SMAF module

[Daniel Vetter]

On Tue, May 17, 2016 at 08:04:59PM +0100, Emil Velikov wrote:
> On 17 May 2016 at 14:50, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> > Hello Emil,
> >
> > thanks for your review.
> > I have understand most of your remarks and I'm fixing them
> > but some points aren't obvious for me...
> >
> Sure thing. Thanks for being honest.
> 
> >
> > No because a device could attach itself on the buffer and the
> > allocator will only
> > be selected at the first map_attach call.
> > The goal is to delay the allocation until all devices are attached to
> > select the best allocator.
> >
> I see. Makes sense.
> 
> 
> >>> +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);
> >> We want to sanitise the input data.{length,flags} before sending it
> >> deeper in the kernel.
> >
> > Sorry but can you elaborate little more here ?
> > I don't understand your expectations.
> >
> You want to determine which flags are 'considered useful' at this
> stage, and reject anything else. As-is you inject any flags that the
> user gives you directly into the 'guts' of the kernel. This is not
> good from security and future expandability POV.
> 
> About the length you want a similar thing. size_t is unsigned (great),
> although ideally you'll want to check/determine if one cannot exploit
> it, integer overflow being the more common suspect. This may be quite
> hard to track, so I'd stick with the flags checking at least.
> 
> 
> > It is useless the add this function in this .h file I will remove it
> > and fix the comment in structure defintion
> 
> I've seen both approaches - description next to the declaration or
> definition. I'd rather not pick sides, as people might throw rotten
> fruit at me ;-)
> 
> 
> >>> +/**
> >>> + * struct smaf_create_data - allocation parameters
> >>> + * @length:    size of the allocation
> >>> + * @flags:     flags passed to allocator
> >>> + * @name:      name of the allocator to be selected, could be NULL
> Just occurred to - you might want to comment what the user should
> expect if NULL. Any at random one will be used or otherwise. Very
> quick description on the heuristics used might be good as well.
> 
> >> Is it guaranteed to be null terminated ? If so one should mentioned it
> >> otherwise your userspace should be fixed.
> >> Same comments apply for smaf_info::name.
> >
> > I have used strncpy everywhere to avoid this problem but maybe it is not enough
> >
> According to the man page
> 
> "The strncpy() function is similar, except that at most n bytes of src
> are copied.  Warning: If there is no null byte among the first n bytes
> of src, the string placed in dest will _not_ be null-terminated."
> 
> Annotation is mine obviously. I believe that after the strncpy 'name'
> is/was assumed (used as) a NULL terminated string.
> 
> >>
> >>
> >>> + * @fd:                returned file descriptor
> >>> + */
> >>> +struct smaf_create_data {
> >>> +       size_t length;
> >>> +       unsigned int flags;
> >>> +       char name[ALLOCATOR_NAME_LENGTH];
> >>> +       int fd;
> >> The structs here feels quite fragile. Please read up on Daniel
> >> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
> >> useful is such process.
> >>
> > if I describe the structures like this:
> > /**
> >  * struct smaf_create_data - allocation parameters
> >  * @length: size of the allocation
> >  * @flags: flags passed to allocator
> >  * @name_len: length of name
> >  * @name: name of the allocator to be selected, could be NULL
> >  * @fd: returned file descriptor
> >  */
> > struct smaf_create_data {
> > size_t length;
> > unsigned int flags;
> > size_t name_len;
> > char __user *name;
> > int fd;
> > char padding[44];
> > };
> >
> > does it sound more robust for you ?
> >
> Seems like you changed 'name' from fixed size array to char *. Which
> actually gets us slightly further away from robust.
> 
> As Daniel said, please read through the hole file.
> 
> Here is a (slightly incomplete) gist of it all:
> - you want to to use __[us]{8,16,32,64} and __kernel_size_t types everywhere

Please don't use __kernel_size_t, it's only for backwards compat if you
already botched an ioctl definition ;-)

Also I recomened you only use 32/64bit sized types, otherwise aligning
stuff gets more complicated. Maybe u8 for strings.

Also you must align everything to its own size explicitly by adding
padding fields. And if you have a 64 bit value anywhere, you must align
the entire struct to 64bits too, otherwise just align to 32bits.

This is a bit more than what's required, but it's much harder to screw up
that way.
-Daniel

> - each member of the struct must be the same offset for both 32bit and
> 64bit builds. ^^ helps with that
> - double check for gaps in the struct - I think you have a few
> - each struct should have it's old 'flags' which you'll use to
> indicate the capability of the ioctl and thus the size of struct used.
> i.e. it's for future use.
> 
> Obviously the other two structs need similar polish.
> 
> Here is how you can check things with pahole:
>  - Create a simple C file that includes the header and has an instance
> of each struct - it doesn't have to be use any of them.
>  - Compile for 32 and 64 bit with -g -O0. Compare the struct layout -
> it should be identical in both cases.
> 
> Hope that makes things a bit clearer.
> 
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 1/3] create SMAF module

[Emil Velikov]

On 17 May 2016 at 22:29, Daniel Vetter <daniel@ffwll.ch> wrote:

> Please don't use __kernel_size_t, it's only for backwards compat if you
> already botched an ioctl definition ;-)
>
That explains why I've not seen it in (m)any other UAPI headers but
our legacy ones.

Thank you !
Emil