This is a note to let you know that I've just added the patch titled
staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
staging-android-ion-fix-ion_ioc_-map-share-use-after-free.patch
and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <sta...@vger.kernel.org> know about it.
>From ghackm...@android.com Fri Aug 31 13:10:56 2018
From: Greg Hackmann <ghackm...@android.com>
Date: Fri, 31 Aug 2018 13:06:27 -0700
Subject: staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free
To: linux-ker...@vger.kernel.org
Cc: Laura Abbott <labb...@redhat.com>, Sumit Semwal <sumit.sem...@linaro.org>,
Greg Kroah-Hartman <gre...@linuxfoundation.org>, de...@driverdev.osuosl.org,
kernel-t...@android.com, Greg Hackmann <ghackm...@google.com>,
sta...@vger.kernel.org
Message-ID: <20180831200627.59712-1-ghackm...@google.com>
From: Greg Hackmann <ghackm...@android.com>
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: sta...@vger.kernel.org # v4.11-
Signed-off-by: Greg Hackmann <ghackm...@google.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
drivers/staging/android/ion/ion-ioctl.c | 12 +++++---
drivers/staging/android/ion/ion.c | 48 +++++++++++++++++++-------------
drivers/staging/android/ion/ion_priv.h | 6 ++--
3 files changed, 40 insertions(+), 26 deletions(-)
--- 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, unsign
{
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;
--- 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_
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
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_clie
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)
{
--- 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 *
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 */
Patches currently in stable-queue which might be from ghackm...@android.com are
queue-4.9/staging-android-ion-fix-ion_ioc_-map-share-use-after-free.patch
queue-4.9/arm64-mm-check-for-upper-page_shift-bits-in-pfn_valid.patch
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
