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

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

[Greg Hackmann]

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.9.y.  Kernels 4.12 and later are
unaffected, since all the underlying ion_handle infrastructure has been
ripped out.

Cc: stable@vger.kernel.org # v4.11-
Signed-off-by: Greg Hackmann <ghackmann@google.com>
---
 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(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index 2b700e8455c6..e3596855a703 100644
--- 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 6f9974cb0e15..403df8bf4b48 100644
--- 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 3c3b3245275d..760e41885448 100644
--- 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 */
-- 
2.19.0.rc1.350.ge57e33dbd1-goog

Re: [PATCH] staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free

[Greg Kroah-Hartman]

On Fri, Aug 31, 2018 at 01:06:27PM -0700, 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.9.y.  Kernels 4.12 and later are
> unaffected, since all the underlying ion_handle infrastructure has been
> ripped out.

Does 4.4.y or older also need this?  If so, can you send backports, as
this one does not apply there.

thanks,

greg k-h

Re: [PATCH] staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free

[Greg Hackmann]

On 08/31/2018 01:12 PM, Greg Kroah-Hartman wrote:
> On Fri, Aug 31, 2018 at 01:06:27PM -0700, 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.9.y.  Kernels 4.12 and later are
>> unaffected, since all the underlying ion_handle infrastructure has been
>> ripped out.
> 
> Does 4.4.y or older also need this?  If so, can you send backports, as
> this one does not apply there.
> 
> thanks,
> 
> greg k-h
> 

Yes, 4.4.y and older will need this.  If there are no objections to this
patch, I'll send backports ASAP.

Re: [PATCH] staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free

[Greg Kroah-Hartman]

On Fri, Aug 31, 2018 at 01:17:20PM -0700, Greg Hackmann wrote:
> On 08/31/2018 01:12 PM, Greg Kroah-Hartman wrote:
> > On Fri, Aug 31, 2018 at 01:06:27PM -0700, 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.9.y.  Kernels 4.12 and later are
> >> unaffected, since all the underlying ion_handle infrastructure has been
> >> ripped out.
> > 
> > Does 4.4.y or older also need this?  If so, can you send backports, as
> > this one does not apply there.
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> Yes, 4.4.y and older will need this.  If there are no objections to this
> patch, I'll send backports ASAP.

None from me, thanks!

greg k-h

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

[Greg Hackmann]

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.

Change-Id: Ia0542dd8134e81cd5e1412e126545303c766f738
Cc: stable@vger.kernel.org # v4.4-
Signed-off-by: Greg Hackmann <ghackmann@google.com>
---
 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

Re: [PATCH] staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free

[Greg Hackmann]

On 08/31/2018 01:27 PM, Greg Hackmann wrote:
> Change-Id: Ia0542dd8134e81cd5e1412e126545303c766f738

Sorry, please disregard the Change-Id line.  This is what I get for
forgetting to re-run checkpatch after amending my commit message.  :/

Re: [PATCH] staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free

[Greg Kroah-Hartman]

On Fri, Aug 31, 2018 at 01:30:01PM -0700, Greg Hackmann wrote:
> On 08/31/2018 01:27 PM, Greg Hackmann wrote:
> > Change-Id: Ia0542dd8134e81cd5e1412e126545303c766f738
> 
> Sorry, please disregard the Change-Id line.  This is what I get for
> forgetting to re-run checkpatch after amending my commit message.  :/

Can you please resend with that fixed up.  Having to hand-edit patches
on my end is a royal pain...

thanks,

greg k-h