diff mbox series

[v2,4.9,1/3] ion: Fix use after free during ION_IOC_ALLOC

Message ID 20220125141808.1172511-1-lee.jones@linaro.org
State New
Headers show
Series [v2,4.9,1/3] ion: Fix use after free during ION_IOC_ALLOC | expand

Commit Message

Lee Jones Jan. 25, 2022, 2:18 p.m. UTC 
From: Daniel Rosenberg <drosen@google.com>

If a user happens to call ION_IOC_FREE during an ION_IOC_ALLOC
on the just allocated id, and the copy_to_user fails, the cleanup
code will attempt to free an already freed handle.

This adds a wrapper for ion_alloc that adds an ion_handle_get to
avoid this.

Signed-off-by: Daniel Rosenberg <drosen@google.com>
Signed-off-by: Dennis Cagle <d-cagle@codeaurora.org>
Signed-off-by: Patrick Daly <pdaly@codeaurora.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---

NB: These are Android patches that were not sent to Mainline.

Only v4.9 is affected by these issues due to refactoring.

 drivers/staging/android/ion/ion-ioctl.c | 14 +++++++++-----
 drivers/staging/android/ion/ion.c       | 15 ++++++++++++---
 drivers/staging/android/ion/ion.h       |  4 ++++
 3 files changed, 25 insertions(+), 8 deletions(-)

Comments

Greg KH Jan. 27, 2022, 3:38 p.m. UTC | #1 
On Tue, Jan 25, 2022 at 02:18:06PM +0000, Lee Jones wrote:
> From: Daniel Rosenberg <drosen@google.com>
> 
> If a user happens to call ION_IOC_FREE during an ION_IOC_ALLOC
> on the just allocated id, and the copy_to_user fails, the cleanup
> code will attempt to free an already freed handle.
> 
> This adds a wrapper for ion_alloc that adds an ion_handle_get to
> avoid this.
> 
> Signed-off-by: Daniel Rosenberg <drosen@google.com>
> Signed-off-by: Dennis Cagle <d-cagle@codeaurora.org>
> Signed-off-by: Patrick Daly <pdaly@codeaurora.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> 
> NB: These are Android patches that were not sent to Mainline.
> 
> Only v4.9 is affected by these issues due to refactoring.

All now queued up, thanks.

greg k-h
diff mbox series

Patch

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index e3596855a7031..f260e0e70488f 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -96,10 +96,10 @@  long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	{
 		struct ion_handle *handle;
 
-		handle = ion_alloc(client, data.allocation.len,
-						data.allocation.align,
-						data.allocation.heap_id_mask,
-						data.allocation.flags);
+		handle = __ion_alloc(client, data.allocation.len,
+				     data.allocation.align,
+				     data.allocation.heap_id_mask,
+				     data.allocation.flags, true);
 		if (IS_ERR(handle))
 			return PTR_ERR(handle);
 
@@ -174,10 +174,14 @@  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)
+			if (cleanup_handle) {
 				ion_free(client, cleanup_handle);
+				ion_handle_put(cleanup_handle);
+			}
 			return -EFAULT;
 		}
 	}
+	if (cleanup_handle)
+		ion_handle_put(cleanup_handle);
 	return ret;
 }
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index aac9b38b8c25c..4f769213be1b7 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -401,9 +401,9 @@  static int ion_handle_add(struct ion_client *client, struct ion_handle *handle)
 	return 0;
 }
 
-struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
-			     size_t align, unsigned int heap_id_mask,
-			     unsigned int flags)
+struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
+			       size_t align, unsigned int heap_id_mask,
+			       unsigned int flags, bool grab_handle)
 {
 	struct ion_handle *handle;
 	struct ion_device *dev = client->dev;
@@ -453,6 +453,8 @@  struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
 		return handle;
 
 	mutex_lock(&client->lock);
+	if (grab_handle)
+		ion_handle_get(handle);
 	ret = ion_handle_add(client, handle);
 	mutex_unlock(&client->lock);
 	if (ret) {
@@ -462,6 +464,13 @@  struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
 
 	return handle;
 }
+
+struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
+			     size_t align, unsigned int heap_id_mask,
+			     unsigned int flags)
+{
+	return __ion_alloc(client, len, align, heap_id_mask, flags, false);
+}
 EXPORT_SYMBOL(ion_alloc);
 
 void ion_free_nolock(struct ion_client *client,
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 93dafb4586e43..cfa50dfb46edc 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -109,6 +109,10 @@  struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
 			     size_t align, unsigned int heap_id_mask,
 			     unsigned int flags);
 
+struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
+			       size_t align, unsigned int heap_id_mask,
+			       unsigned int flags, bool grab_handle);
+
 /**
  * ion_free - free a handle
  * @client:	the client