Re: [PATCH v4 1/2] create SMAF module

[Laura Abbott <labbott@redhat.com>]

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@linaro.org> 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