staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free

This patch is 4.9.y only.  Kernels 4.12 and later are unaffected, since
all the underlying ion_handle infrastructure has been ripped out.

The ION_IOC_{MAP,SHARE} ioctls drop and reacquire client->lock several
times while operating on one of the client's ion_handles.  This creates
windows where userspace can call ION_IOC_FREE on the same client with
the same handle, and effectively make the kernel drop its own reference.
For example:

- thread A: ION_IOC_ALLOC creates an ion_handle with refcount 1
- thread A: starts ION_IOC_MAP and increments the refcount to 2
- thread B: ION_IOC_FREE decrements the refcount to 1
- thread B: ION_IOC_FREE decrements the refcount to 0 and frees the
            handle
- thread A: continues ION_IOC_MAP with a dangling ion_handle * to
            freed memory

Fix this by holding client->lock for the duration of
ION_IOC_{MAP,SHARE}, preventing the concurrent ION_IOC_FREE.  Also
remove ion_handle_get_by_id(), since there's literally no way to use it
safely.

Cc: stable@vger.kernel.org # v4.11-
Signed-off-by: Greg Hackmann <ghackmann@google.com>
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 2b700e8..e359685 100644 (file)
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -128,11 +128,15 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
        {
                struct ion_handle *handle;
 
-               handle = ion_handle_get_by_id(client, data.handle.handle);
-               if (IS_ERR(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);
-               data.fd.fd = ion_share_dma_buf_fd(client, handle);
-               ion_handle_put(handle);
+               }
+               data.fd.fd = ion_share_dma_buf_fd_nolock(client, handle);
+               ion_handle_put_nolock(handle);
+               mutex_unlock(&client->lock);
                if (data.fd.fd < 0)
                        ret = data.fd.fd;
                break;

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 6f9974c..403df8b 100644 (file)
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -352,18 +352,6 @@ struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
        return handle ? handle : ERR_PTR(-EINVAL);
 }
 
-struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
-                                              int id)
-{
-       struct ion_handle *handle;
-
-       mutex_lock(&client->lock);
-       handle = ion_handle_get_by_id_nolock(client, id);
-       mutex_unlock(&client->lock);
-
-       return handle;
-}
-
 static bool ion_handle_validate(struct ion_client *client,
                                struct ion_handle *handle)
 {
@@ -1029,24 +1017,28 @@ static struct dma_buf_ops dma_buf_ops = {
        .kunmap = ion_dma_buf_kunmap,
 };
 
-struct dma_buf *ion_share_dma_buf(struct ion_client *client,
-                                 struct ion_handle *handle)
+static struct dma_buf *__ion_share_dma_buf(struct ion_client *client,
+                                          struct ion_handle *handle,
+                                          bool lock_client)
 {
        DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
        struct ion_buffer *buffer;
        struct dma_buf *dmabuf;
        bool valid_handle;
 
-       mutex_lock(&client->lock);
+       if (lock_client)
+               mutex_lock(&client->lock);
        valid_handle = ion_handle_validate(client, handle);
        if (!valid_handle) {
                WARN(1, "%s: invalid handle passed to share.\n", __func__);
-               mutex_unlock(&client->lock);
+               if (lock_client)
+                       mutex_unlock(&client->lock);
                return ERR_PTR(-EINVAL);
        }
        buffer = handle->buffer;
        ion_buffer_get(buffer);
-       mutex_unlock(&client->lock);
+       if (lock_client)
+               mutex_unlock(&client->lock);
 
        exp_info.ops = &dma_buf_ops;
        exp_info.size = buffer->size;
@@ -1061,14 +1053,21 @@ struct dma_buf *ion_share_dma_buf(struct ion_client *client,
 
        return dmabuf;
 }
+
+struct dma_buf *ion_share_dma_buf(struct ion_client *client,
+                                 struct ion_handle *handle)
+{
+       return __ion_share_dma_buf(client, handle, true);
+}
 EXPORT_SYMBOL(ion_share_dma_buf);
 
-int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle)
+static int __ion_share_dma_buf_fd(struct ion_client *client,
+                                 struct ion_handle *handle, bool lock_client)
 {
        struct dma_buf *dmabuf;
        int fd;
 
-       dmabuf = ion_share_dma_buf(client, handle);
+       dmabuf = __ion_share_dma_buf(client, handle, lock_client);
        if (IS_ERR(dmabuf))
                return PTR_ERR(dmabuf);
 
@@ -1078,8 +1077,19 @@ int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle)
 
        return fd;
 }
+
+int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle)
+{
+       return __ion_share_dma_buf_fd(client, handle, true);
+}
 EXPORT_SYMBOL(ion_share_dma_buf_fd);
 
+int ion_share_dma_buf_fd_nolock(struct ion_client *client,
+                               struct ion_handle *handle)
+{
+       return __ion_share_dma_buf_fd(client, handle, false);
+}
+
 struct ion_handle *ion_import_dma_buf(struct ion_client *client,
                                      struct dma_buf *dmabuf)
 {

diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
index 3c3b324..760e418 100644 (file)
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -463,11 +463,11 @@ 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);
 
 int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query);
 
+int ion_share_dma_buf_fd_nolock(struct ion_client *client,
+                               struct ion_handle *handle);
+
 #endif /* _ION_PRIV_H */