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 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