ion: Protect kref from userspace manipulation

This separates the kref for ion handles into two components.
Userspace requests through the ioctl will hold at most one
reference to the internally used kref. All additional requests
will increment a separate counter, and the original reference is
only put once that counter hits 0. This protects the kernel from
a poorly behaving userspace.

Signed-off-by: Daniel Rosenberg <drosen@google.com>
[d-cagle@codeaurora.org: Resolve style issues]
Signed-off-by: Dennis Cagle <d-cagle@codeaurora.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index f260e0e70488ffdf3c84acab667e13977539fe8c..d47e9b4171e28ae0fb0d6ea78173d77952824b4f 100644 (file)
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -30,6 +30,69 @@ union ion_ioctl_arg {
        struct ion_heap_query query;
 };
 
+/* Must hold the client lock */
+static void user_ion_handle_get(struct ion_handle *handle)
+{
+       if (handle->user_ref_count++ == 0)
+               kref_get(&handle->ref);
+}
+
+/* Must hold the client lock */
+static struct ion_handle *user_ion_handle_get_check_overflow(
+       struct ion_handle *handle)
+{
+       if (handle->user_ref_count + 1 == 0)
+               return ERR_PTR(-EOVERFLOW);
+       user_ion_handle_get(handle);
+       return handle;
+}
+
+/* passes a kref to the user ref count.
+ * We know we're holding a kref to the object before and
+ * after this call, so no need to reverify handle.
+ */
+static struct ion_handle *pass_to_user(struct ion_handle *handle)
+{
+       struct ion_client *client = handle->client;
+       struct ion_handle *ret;
+
+       mutex_lock(&client->lock);
+       ret = user_ion_handle_get_check_overflow(handle);
+       ion_handle_put_nolock(handle);
+       mutex_unlock(&client->lock);
+       return ret;
+}
+
+/* Must hold the client lock */
+static int user_ion_handle_put_nolock(struct ion_handle *handle)
+{
+       int ret;
+
+       if (--handle->user_ref_count == 0)
+               ret = ion_handle_put_nolock(handle);
+
+       return ret;
+}
+
+static void user_ion_free_nolock(struct ion_client *client,
+                                struct ion_handle *handle)
+{
+       bool valid_handle;
+
+       WARN_ON(client != handle->client);
+
+       valid_handle = ion_handle_validate(client, handle);
+       if (!valid_handle) {
+               WARN(1, "%s: invalid handle passed to free.\n", __func__);
+               return;
+       }
+       if (handle->user_ref_count == 0) {
+               WARN(1, "%s: User does not have access!\n", __func__);
+               return;
+       }
+       user_ion_handle_put_nolock(handle);
+}
+
 static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
 {
        int ret = 0;
@@ -102,7 +165,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
                                     data.allocation.flags, true);
                if (IS_ERR(handle))
                        return PTR_ERR(handle);
-
+               pass_to_user(handle);
                data.allocation.handle = handle->id;
 
                cleanup_handle = handle;
@@ -118,7 +181,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
                        mutex_unlock(&client->lock);
                        return PTR_ERR(handle);
                }
-               ion_free_nolock(client, handle);
+               user_ion_free_nolock(client, handle);
                ion_handle_put_nolock(handle);
                mutex_unlock(&client->lock);
                break;
@@ -146,10 +209,15 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
                struct ion_handle *handle;
 
                handle = ion_import_dma_buf_fd(client, data.fd.fd);
-               if (IS_ERR(handle))
+               if (IS_ERR(handle)) {
                        ret = PTR_ERR(handle);
-               else
-                       data.handle.handle = handle->id;
+               } else {
+                       handle = pass_to_user(handle);
+                       if (IS_ERR(handle))
+                               ret = PTR_ERR(handle);
+                       else
+                               data.handle.handle = handle->id;
+               }
                break;
        }
        case ION_IOC_SYNC:
@@ -175,8 +243,10 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
        if (dir & _IOC_READ) {
                if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
                        if (cleanup_handle) {
-                               ion_free(client, cleanup_handle);
-                               ion_handle_put(cleanup_handle);
+                               mutex_lock(&client->lock);
+                               user_ion_free_nolock(client, cleanup_handle);
+                               ion_handle_put_nolock(cleanup_handle);
+                               mutex_unlock(&client->lock);
                        }
                        return -EFAULT;
                }

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 4f769213be1b745b74adb6763c2bc1ae09901a99..b272f2ab87e8f29ddb132815bcf5755add3d7816 100644 (file)
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -363,8 +363,8 @@ struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
        return ERR_PTR(-EINVAL);
 }
 
-static bool ion_handle_validate(struct ion_client *client,
-                               struct ion_handle *handle)
+bool ion_handle_validate(struct ion_client *client,
+                        struct ion_handle *handle)
 {
        WARN_ON(!mutex_is_locked(&client->lock));
        return idr_find(&client->idr, handle->id) == handle;

diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
index 760e41885448a2b195c5e52e179204acce274d77..e1dd25eab1dbdc10ed3ac68b85be26acaa742b73 100644 (file)
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -149,6 +149,7 @@ struct ion_client {
  */
 struct ion_handle {
        struct kref ref;
+       unsigned int user_ref_count;
        struct ion_client *client;
        struct ion_buffer *buffer;
        struct rb_node node;
@@ -459,6 +460,9 @@ 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);
 
+bool ion_handle_validate(struct ion_client *client,
+                        struct ion_handle *handle);
+
 void ion_free_nolock(struct ion_client *client, struct ion_handle *handle);
 
 int ion_handle_put_nolock(struct ion_handle *handle);