| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 5 Oct 2015 18:58:57 -0700
From: Laura Abbott <labbott@...hat.com>
To: Benjamin Gaignard <benjamin.gaignard@...aro.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, daniel.vetter@...ll.ch,
robdclark@...il.com, treding@...dia.com, sumit.semwal@...aro.org,
tom.cooksey@....com, daniel.stone@...labora.com,
linux-security-module@...r.kernel.org, xiaoquan.li@...antecorp.com
Cc: tom.gall@...aro.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v4 1/2] create SMAF module
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@...aro.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists