| 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
| ||
|
Message-ID: <20160608132034.GM1165@e106497-lin.cambridge.arm.com>
Date: Wed, 8 Jun 2016 14:20:34 +0100
From: Liviu Dudau <Liviu.Dudau@....com>
To: Laura Abbott <labbott@...hat.com>
Cc: Sumit Semwal <sumit.semwal@...aro.org>,
John Stultz <john.stultz@...aro.org>,
Arve Hjønnevåg <arve@...roid.com>,
Riley Andrews <riandrews@...roid.com>,
Daniel Vetter <daniel.vetter@...ll.ch>,
linaro-mm-sig@...ts.linaro.org, devel@...verdev.osuosl.org,
linux-kernel@...r.kernel.org,
Eun Taik Lee <eun.taik.lee@...sung.com>,
Jon Medhurst <tixy@...aro.org>,
Mitchel Humpherys <mitchelh@...eaurora.org>,
Jeremy Gebben <jgebben@...eaurora.org>,
Bryan Huntsman <bryanh@...eaurora.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [RFC][PATCH 4/6] staging: android: ion: Pull out ion ioctls to a
separate file
On Mon, Jun 06, 2016 at 11:23:31AM -0700, Laura Abbott wrote:
>
> The number of Ion ioctls may continue to grow along with necessary
> validation. Pull it out into a separate file for easier management
> and review.
>
> Signed-off-by: Laura Abbott <labbott@...hat.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@....com>
> ---
> drivers/staging/android/ion/Makefile | 3 +-
> drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++
> drivers/staging/android/ion/ion.c | 208 +-------------------------------
> drivers/staging/android/ion/ion_priv.h | 92 ++++++++++++++
> 4 files changed, 244 insertions(+), 203 deletions(-)
> create mode 100644 drivers/staging/android/ion/ion-ioctl.c
>
> diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
> index 18cc2aa..376c2b2 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -1,4 +1,5 @@
> -obj-$(CONFIG_ION) += ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \
> +obj-$(CONFIG_ION) += ion.o ion-ioctl.o ion_heap.o \
> + ion_page_pool.o ion_system_heap.o \
> ion_carveout_heap.o ion_chunk_heap.o ion_cma_heap.o
> obj-$(CONFIG_ION_TEST) += ion_test.o
> ifdef CONFIG_COMPAT
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> new file mode 100644
> index 0000000..341ba7d
> --- /dev/null
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -0,0 +1,144 @@
> +/*
> + *
> + * Copyright (C) 2011 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +
> +#include "ion.h"
> +#include "ion_priv.h"
> +#include "compat_ion.h"
> +
> +/* fix up the cases where the ioctl direction bits are incorrect */
> +static unsigned int ion_ioctl_dir(unsigned int cmd)
> +{
> + switch (cmd) {
> + case ION_IOC_SYNC:
> + case ION_IOC_FREE:
> + case ION_IOC_CUSTOM:
> + return _IOC_WRITE;
> + default:
> + return _IOC_DIR(cmd);
> + }
> +}
> +
> +long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + struct ion_client *client = filp->private_data;
> + struct ion_device *dev = client->dev;
> + struct ion_handle *cleanup_handle = NULL;
> + int ret = 0;
> + unsigned int dir;
> +
> + union {
> + struct ion_fd_data fd;
> + struct ion_allocation_data allocation;
> + struct ion_handle_data handle;
> + struct ion_custom_data custom;
> + } data;
> +
> + dir = ion_ioctl_dir(cmd);
> +
> + if (_IOC_SIZE(cmd) > sizeof(data))
> + return -EINVAL;
> +
> + if (dir & _IOC_WRITE)
> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> + return -EFAULT;
> +
> + switch (cmd) {
> + case ION_IOC_ALLOC:
> + {
> + struct ion_handle *handle;
> +
> + handle = ion_alloc(client, data.allocation.len,
> + data.allocation.align,
> + data.allocation.heap_id_mask,
> + data.allocation.flags);
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> +
> + data.allocation.handle = handle->id;
> +
> + cleanup_handle = handle;
> + break;
> + }
> + case ION_IOC_FREE:
> + {
> + struct ion_handle *handle;
> +
> + mutex_lock(&client->lock);
> + handle = ion_handle_get_by_id_nolock(client, data.handle.handle);
> + if (IS_ERR(handle)) {
> + mutex_unlock(&client->lock);
> + return PTR_ERR(handle);
> + }
> + ion_free_nolock(client, handle);
> + ion_handle_put_nolock(handle);
> + mutex_unlock(&client->lock);
> + break;
> + }
> + case ION_IOC_SHARE:
> + case ION_IOC_MAP:
> + {
> + struct ion_handle *handle;
> +
> + handle = ion_handle_get_by_id(client, data.handle.handle);
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> + data.fd.fd = ion_share_dma_buf_fd(client, handle);
> + ion_handle_put(handle);
> + if (data.fd.fd < 0)
> + ret = data.fd.fd;
> + break;
> + }
> + case ION_IOC_IMPORT:
> + {
> + struct ion_handle *handle;
> +
> + handle = ion_import_dma_buf_fd(client, data.fd.fd);
> + if (IS_ERR(handle))
> + ret = PTR_ERR(handle);
> + else
> + data.handle.handle = handle->id;
> + break;
> + }
> + case ION_IOC_SYNC:
> + {
> + ret = ion_sync_for_device(client, data.fd.fd);
> + break;
> + }
> + case ION_IOC_CUSTOM:
> + {
> + if (!dev->custom_ioctl)
> + return -ENOTTY;
> + ret = dev->custom_ioctl(client, data.custom.cmd,
> + data.custom.arg);
> + break;
> + }
> + default:
> + return -ENOTTY;
> + }
> +
> + if (dir & _IOC_READ) {
> + if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
> + if (cleanup_handle)
> + ion_free(client, cleanup_handle);
> + return -EFAULT;
> + }
> + }
> + return ret;
> +}
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 739060f..129707f 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -41,81 +41,6 @@
> #include "ion_priv.h"
> #include "compat_ion.h"
>
> -/**
> - * struct ion_device - the metadata of the ion device node
> - * @dev: the actual misc device
> - * @buffers: an rb tree of all the existing buffers
> - * @buffer_lock: lock protecting the tree of buffers
> - * @lock: rwsem protecting the tree of heaps and clients
> - * @heaps: list of all the heaps in the system
> - * @user_clients: list of all the clients created from userspace
> - */
> -struct ion_device {
> - struct miscdevice dev;
> - struct rb_root buffers;
> - struct mutex buffer_lock;
> - struct rw_semaphore lock;
> - long (*custom_ioctl)(struct ion_client *client, unsigned int cmd,
> - unsigned long arg);
> - struct rb_root clients;
> - struct dentry *debug_root;
> - struct dentry *heaps_debug_root;
> - struct dentry *clients_debug_root;
> - struct idr idr;
> - int heap_cnt;
> -};
> -
> -/**
> - * struct ion_client - a process/hw block local address space
> - * @node: node in the tree of all clients
> - * @dev: backpointer to ion device
> - * @handles: an rb tree of all the handles in this client
> - * @idr: an idr space for allocating handle ids
> - * @lock: lock protecting the tree of handles
> - * @name: used for debugging
> - * @display_name: used for debugging (unique version of @name)
> - * @display_serial: used for debugging (to make display_name unique)
> - * @task: used for debugging
> - *
> - * A client represents a list of buffers this client may access.
> - * The mutex stored here is used to protect both handles tree
> - * as well as the handles themselves, and should be held while modifying either.
> - */
> -struct ion_client {
> - struct rb_node node;
> - struct ion_device *dev;
> - struct rb_root handles;
> - struct idr idr;
> - struct mutex lock;
> - const char *name;
> - char *display_name;
> - int display_serial;
> - struct task_struct *task;
> - pid_t pid;
> - struct dentry *debug_root;
> -};
> -
> -/**
> - * ion_handle - a client local reference to a buffer
> - * @ref: reference count
> - * @client: back pointer to the client the buffer resides in
> - * @buffer: pointer to the buffer
> - * @node: node in the client's handle rbtree
> - * @kmap_cnt: count of times this client has mapped to kernel
> - * @id: client-unique id allocated by client->idr
> - *
> - * Modifications to node, map_cnt or mapping should be protected by the
> - * lock in the client. Other fields are never changed after initialization.
> - */
> -struct ion_handle {
> - struct kref ref;
> - struct ion_client *client;
> - struct ion_buffer *buffer;
> - struct rb_node node;
> - unsigned int kmap_cnt;
> - int id;
> -};
> -
> bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer)
> {
> return (buffer->flags & ION_FLAG_CACHED) &&
> @@ -388,7 +313,7 @@ static void ion_handle_get(struct ion_handle *handle)
> kref_get(&handle->ref);
> }
>
> -static int ion_handle_put_nolock(struct ion_handle *handle)
> +int ion_handle_put_nolock(struct ion_handle *handle)
> {
> int ret;
>
> @@ -397,7 +322,7 @@ static int ion_handle_put_nolock(struct ion_handle *handle)
> return ret;
> }
>
> -static int ion_handle_put(struct ion_handle *handle)
> +int ion_handle_put(struct ion_handle *handle)
> {
> struct ion_client *client = handle->client;
> int ret;
> @@ -427,7 +352,7 @@ static struct ion_handle *ion_handle_lookup(struct ion_client *client,
> return ERR_PTR(-EINVAL);
> }
>
> -static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> +struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> int id)
> {
> struct ion_handle *handle;
> @@ -439,7 +364,7 @@ static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> return handle ? handle : ERR_PTR(-EINVAL);
> }
>
> -static struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
> +struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
> int id)
> {
> struct ion_handle *handle;
> @@ -570,7 +495,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
> }
> EXPORT_SYMBOL(ion_alloc);
>
> -static void ion_free_nolock(struct ion_client *client, struct ion_handle *handle)
> +void ion_free_nolock(struct ion_client *client, struct ion_handle *handle)
> {
> bool valid_handle;
>
> @@ -1294,7 +1219,7 @@ struct ion_handle *ion_import_dma_buf_fd(struct ion_client *client, int fd)
> }
> EXPORT_SYMBOL(ion_import_dma_buf_fd);
>
> -static int ion_sync_for_device(struct ion_client *client, int fd)
> +int ion_sync_for_device(struct ion_client *client, int fd)
> {
> struct dma_buf *dmabuf;
> struct ion_buffer *buffer;
> @@ -1318,127 +1243,6 @@ static int ion_sync_for_device(struct ion_client *client, int fd)
> return 0;
> }
>
> -/* fix up the cases where the ioctl direction bits are incorrect */
> -static unsigned int ion_ioctl_dir(unsigned int cmd)
> -{
> - switch (cmd) {
> - case ION_IOC_SYNC:
> - case ION_IOC_FREE:
> - case ION_IOC_CUSTOM:
> - return _IOC_WRITE;
> - default:
> - return _IOC_DIR(cmd);
> - }
> -}
> -
> -static long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> -{
> - struct ion_client *client = filp->private_data;
> - struct ion_device *dev = client->dev;
> - struct ion_handle *cleanup_handle = NULL;
> - int ret = 0;
> - unsigned int dir;
> -
> - union {
> - struct ion_fd_data fd;
> - struct ion_allocation_data allocation;
> - struct ion_handle_data handle;
> - struct ion_custom_data custom;
> - } data;
> -
> - dir = ion_ioctl_dir(cmd);
> -
> - if (_IOC_SIZE(cmd) > sizeof(data))
> - return -EINVAL;
> -
> - if (dir & _IOC_WRITE)
> - if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> - return -EFAULT;
> -
> - switch (cmd) {
> - case ION_IOC_ALLOC:
> - {
> - struct ion_handle *handle;
> -
> - handle = ion_alloc(client, data.allocation.len,
> - data.allocation.align,
> - data.allocation.heap_id_mask,
> - data.allocation.flags);
> - if (IS_ERR(handle))
> - return PTR_ERR(handle);
> -
> - data.allocation.handle = handle->id;
> -
> - cleanup_handle = handle;
> - break;
> - }
> - case ION_IOC_FREE:
> - {
> - struct ion_handle *handle;
> -
> - mutex_lock(&client->lock);
> - handle = ion_handle_get_by_id_nolock(client, data.handle.handle);
> - if (IS_ERR(handle)) {
> - mutex_unlock(&client->lock);
> - return PTR_ERR(handle);
> - }
> - ion_free_nolock(client, handle);
> - ion_handle_put_nolock(handle);
> - mutex_unlock(&client->lock);
> - break;
> - }
> - case ION_IOC_SHARE:
> - case ION_IOC_MAP:
> - {
> - struct ion_handle *handle;
> -
> - handle = ion_handle_get_by_id(client, data.handle.handle);
> - if (IS_ERR(handle))
> - return PTR_ERR(handle);
> - data.fd.fd = ion_share_dma_buf_fd(client, handle);
> - ion_handle_put(handle);
> - if (data.fd.fd < 0)
> - ret = data.fd.fd;
> - break;
> - }
> - case ION_IOC_IMPORT:
> - {
> - struct ion_handle *handle;
> -
> - handle = ion_import_dma_buf_fd(client, data.fd.fd);
> - if (IS_ERR(handle))
> - ret = PTR_ERR(handle);
> - else
> - data.handle.handle = handle->id;
> - break;
> - }
> - case ION_IOC_SYNC:
> - {
> - ret = ion_sync_for_device(client, data.fd.fd);
> - break;
> - }
> - case ION_IOC_CUSTOM:
> - {
> - if (!dev->custom_ioctl)
> - return -ENOTTY;
> - ret = dev->custom_ioctl(client, data.custom.cmd,
> - data.custom.arg);
> - break;
> - }
> - default:
> - return -ENOTTY;
> - }
> -
> - if (dir & _IOC_READ) {
> - if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
> - if (cleanup_handle)
> - ion_free(client, cleanup_handle);
> - return -EFAULT;
> - }
> - }
> - return ret;
> -}
> -
> static int ion_release(struct inode *inode, struct file *file)
> {
> struct ion_client *client = file->private_data;
> diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
> index 35726ae..b09d751 100644
> --- a/drivers/staging/android/ion/ion_priv.h
> +++ b/drivers/staging/android/ion/ion_priv.h
> @@ -26,6 +26,7 @@
> #include <linux/sched.h>
> #include <linux/shrinker.h>
> #include <linux/types.h>
> +#include <linux/miscdevice.h>
>
> #include "ion.h"
>
> @@ -88,6 +89,81 @@ struct ion_buffer {
> void ion_buffer_destroy(struct ion_buffer *buffer);
>
> /**
> + * struct ion_device - the metadata of the ion device node
> + * @dev: the actual misc device
> + * @buffers: an rb tree of all the existing buffers
> + * @buffer_lock: lock protecting the tree of buffers
> + * @lock: rwsem protecting the tree of heaps and clients
> + * @heaps: list of all the heaps in the system
> + * @user_clients: list of all the clients created from userspace
> + */
> +struct ion_device {
> + struct miscdevice dev;
> + struct rb_root buffers;
> + struct mutex buffer_lock;
> + struct rw_semaphore lock;
> + long (*custom_ioctl)(struct ion_client *client, unsigned int cmd,
> + unsigned long arg);
> + struct rb_root clients;
> + struct dentry *debug_root;
> + struct dentry *heaps_debug_root;
> + struct dentry *clients_debug_root;
> + struct idr idr;
> + int heap_cnt;
> +};
> +
> +/**
> + * struct ion_client - a process/hw block local address space
> + * @node: node in the tree of all clients
> + * @dev: backpointer to ion device
> + * @handles: an rb tree of all the handles in this client
> + * @idr: an idr space for allocating handle ids
> + * @lock: lock protecting the tree of handles
> + * @name: used for debugging
> + * @display_name: used for debugging (unique version of @name)
> + * @display_serial: used for debugging (to make display_name unique)
> + * @task: used for debugging
> + *
> + * A client represents a list of buffers this client may access.
> + * The mutex stored here is used to protect both handles tree
> + * as well as the handles themselves, and should be held while modifying either.
> + */
> +struct ion_client {
> + struct rb_node node;
> + struct ion_device *dev;
> + struct rb_root handles;
> + struct idr idr;
> + struct mutex lock;
> + const char *name;
> + char *display_name;
> + int display_serial;
> + struct task_struct *task;
> + pid_t pid;
> + struct dentry *debug_root;
> +};
> +
> +/**
> + * ion_handle - a client local reference to a buffer
> + * @ref: reference count
> + * @client: back pointer to the client the buffer resides in
> + * @buffer: pointer to the buffer
> + * @node: node in the client's handle rbtree
> + * @kmap_cnt: count of times this client has mapped to kernel
> + * @id: client-unique id allocated by client->idr
> + *
> + * Modifications to node, map_cnt or mapping should be protected by the
> + * lock in the client. Other fields are never changed after initialization.
> + */
> +struct ion_handle {
> + struct kref ref;
> + struct ion_client *client;
> + struct ion_buffer *buffer;
> + struct rb_node node;
> + unsigned int kmap_cnt;
> + int id;
> +};
> +
> +/**
> * struct ion_heap_ops - ops to operate on a given heap
> * @allocate: allocate memory
> * @free: free memory
> @@ -403,4 +479,20 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
> void ion_pages_sync_for_device(struct device *dev, struct page *page,
> size_t size, enum dma_data_direction dir);
>
> +long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> +
> +int ion_sync_for_device(struct ion_client *client, int fd);
> +
> +struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> + int id);
> +
> +void ion_free_nolock(struct ion_client *client, struct ion_handle *handle);
> +
> +int ion_handle_put_nolock(struct ion_handle *handle);
> +
> +struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
> + int id);
> +
> +int ion_handle_put(struct ion_handle *handle);
> +
> #endif /* _ION_PRIV_H */
> --
> 2.5.5
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
Powered by blists - more mailing lists