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.
This patch is applied on top of 4.4.y, and applies to older kernels
too. 4.9.y was fixed separately. Kernels 4.12 and later are
unaffected, since all the underlying ion_handle infrastructure has been
ripped out.
Cc: [email protected] # v4.4-
Signed-off-by: Greg Hackmann <[email protected]>
---
v2: remove Change-Id line from commit message
drivers/staging/android/ion/ion.c | 60 +++++++++++++++++++------------
1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 47cb163da9a0..4adb1138af09 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -449,18 +449,6 @@ static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
return 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)
{
@@ -1138,24 +1126,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;
@@ -1170,14 +1162,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);
@@ -1187,8 +1186,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);
+static 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, int fd)
{
struct dma_buf *dmabuf;
@@ -1335,11 +1345,15 @@ static 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;
--
2.19.0.rc1.350.ge57e33dbd1-goog
On 09/04/2018 09:33 AM, Greg Hackmann wrote:
> 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.
>
> This patch is applied on top of 4.4.y, and applies to older kernels
> too. 4.9.y was fixed separately. Kernels 4.12 and later are
> unaffected, since all the underlying ion_handle infrastructure has been
> ripped out.
>
Acked-by: Laura Abbott <[email protected]>
> Cc: [email protected] # v4.4-
> Signed-off-by: Greg Hackmann <[email protected]>
> ---
> v2: remove Change-Id line from commit message
>
> drivers/staging/android/ion/ion.c | 60 +++++++++++++++++++------------
> 1 file changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 47cb163da9a0..4adb1138af09 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -449,18 +449,6 @@ static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> return 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)
> {
> @@ -1138,24 +1126,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;
> @@ -1170,14 +1162,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);
>
> @@ -1187,8 +1186,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);
>
> +static 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, int fd)
> {
> struct dma_buf *dmabuf;
> @@ -1335,11 +1345,15 @@ static 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;
>