From: Laura Abbott <labbott@redhat.com>
To: Benjamin Gaignard <benjamin.gaignard@linaro.org>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, daniel.vetter@ffwll.ch,
robdclark@gmail.com, treding@nvidia.com, sumit.semwal@linaro.org,
tom.cooksey@arm.com, daniel.stone@collabora.com,
linux-security-module@vger.kernel.org,
xiaoquan.li@vivantecorp.com
Cc: tom.gall@linaro.org, linaro-mm-sig@lists.linaro.org
Subject: Re: [PATCH v4 1/2] create SMAF module
Date: Mon, 5 Oct 2015 18:58:57 -0700 [thread overview]
Message-ID: <56132AE1.6060503@redhat.com> (raw)
In-Reply-To: <1444039898-7927-2-git-send-email-benjamin.gaignard@linaro.org>
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
next prev parent reply other threads:[~2015-10-06 1:59 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-05 10:11 [PATCH v4 0/2] RFC: Secure Memory Allocation Framework Benjamin Gaignard
2015-10-05 10:11 ` [PATCH v4 1/2] create SMAF module Benjamin Gaignard
2015-10-05 22:02 ` kbuild test robot
2015-10-06 1:58 ` Laura Abbott [this message]
2015-10-06 8:56 ` Benjamin Gaignard
2015-10-05 10:11 ` [PATCH v4 2/2] SMAF: add CMA allocator Benjamin Gaignard
2015-10-05 10:50 ` kbuild test robot
2015-10-05 10:50 ` [RFC PATCH] SMAF: smaf_cma can be static kbuild test robot
2015-10-05 11:50 ` [PATCH v4 2/2] SMAF: add CMA allocator kbuild test robot
2015-10-06 2:07 ` [PATCH v4 0/2] RFC: Secure Memory Allocation Framework Laura Abbott
2015-10-06 7:03 ` Benjamin Gaignard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56132AE1.6060503@redhat.com \
--to=labbott@redhat.com \
--cc=benjamin.gaignard@linaro.org \
--cc=daniel.stone@collabora.com \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=robdclark@gmail.com \
--cc=sumit.semwal@linaro.org \
--cc=tom.cooksey@arm.com \
--cc=tom.gall@linaro.org \
--cc=treding@nvidia.com \
--cc=xiaoquan.li@vivantecorp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).