staging/android/ion : fix a race condition in the ion driver

From: Laura Abbott [mailto:[email protected]]
Sent: Friday, February 26, 2016 7:12 AM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Rohit Kumar <[email protected]>; [email protected]; shawn.lin@rock- chips.com; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3] staging/android/ion : fix a race condition in the ion driver

Quoted 34 lines · EunTaik Lee

There is a use-after-free problem in the ion driver. This is caused by a race condition in the ion_ioctl() function.

A handle has ref count of 1 and two tasks on different cpus calls ION_IOC_FREE simultaneously.

cpu 0 cpu 1
-------------------------------------------------------
ion_handle_get_by_id()
(ref == 2)

                             ion_handle_get_by_id()
                             (ref == 3)

ion_free()
(ref == 2)

ion_handle_put()
(ref == 1)

ion_free()
(ref == 0 so ion_handle_destroy() is called
and the handle is freed.)

ion_handle_put() is called and it decreases the slub's next free pointer

The problem is detected as an unaligned access in the spin lock functions since it uses load exclusive
instruction. In some cases it corrupts the slub's free pointer which
causes a mis-aligned access to the next free pointer.(kmalloc returns a pointer like ffffc0745b4580aa). And it causes lots of other hard-to-debug problems.

Quoted 8 lines

This symptom is caused since the first member in the ion_handle structure is the reference count and the ion driver decrements the reference after it has been freed.

To fix this problem client->lock mutex is extended to protect all the codes that uses the handle.

Signed-off-by Eun Taik Lee <[email protected]>

Reviewed-by Laura Abbott <[email protected]>

Quoted 9 lines

---
changes in v3:
1. remove ion_handle_put in ion_free
2. remove unnecessary protection in IOC_ION_SHARE/IOC_ION_MAP
changes in v2 :
1. add problem description in the comment
2. fix un-matching mutex_lock/unlock pair in ion_share_dma_buf()

  drivers/staging/android/ion/ion.c | 55 ++++++++++++++++++++++++++++++-

--------

drivers/staging/android/ion/ion.c static void ion_handle_get(struct ion_handle

Quoted 11 lines

  1 file changed, 42 insertions(+), 13 deletions(-)

mode change 100644 => 100755 drivers/staging/android/ion/ion.c

drivers/staging/android/ion/ion.c +0 -0

old mode 100644
new mode 100755
index e237e9f..1958d58
@@ -385,13 +385,22 @@ static void ion_handle_get(struct ion_handle

*handle)

drivers/staging/android/ion/ion.c static struct ion_handle *ion_handle_lookup(struct

Quoted 24 lines

drivers/staging/android/ion/ion.c +10 -2

  	kref_get(&handle->ref);
  }
 
-static int ion_handle_put(struct ion_handle *handle)
+static int ion_handle_put_nolock(struct ion_handle *handle) {
+	int ret;
+
+	ret = kref_put(&handle->ref, ion_handle_destroy);
+
+	return ret;
+}
+
+int ion_handle_put(struct ion_handle *handle)
  {
  	struct ion_client *client = handle->client;
  	int ret;
 
  	mutex_lock(&client->lock);
-	ret = kref_put(&handle->ref, ion_handle_destroy);
+	ret = ion_handle_put_nolock(handle);
  	mutex_unlock(&client->lock);
 
  	return ret;

@@ -415,20 +424,30 @@ static struct ion_handle *ion_handle_lookup(struct

ion_client *client,

drivers/staging/android/ion/ion.c struct ion_handle *ion_alloc(struct ion_client

Quoted 36 lines

drivers/staging/android/ion/ion.c +14 -3

  	return ERR_PTR(-EINVAL);
  }
 
-static struct ion_handle *ion_handle_get_by_id(struct ion_client
 *client,
+static struct ion_handle *ion_handle_get_by_id_nolock(struct
+ion_client *client,
  						int id)
  {
  	struct ion_handle *handle;
 
-	mutex_lock(&client->lock);
  	handle = idr_find(&client->idr, id);
  	if (handle)
  		ion_handle_get(handle);
-	mutex_unlock(&client->lock);
 
  	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)
  {

@@ -530,22 +549,28 @@ struct ion_handle *ion_alloc(struct ion_client

*client, size_t len,

drivers/staging/android/ion/ion.c static long ion_ioctl(struct file *filp,

Quoted 33 lines

drivers/staging/android/ion/ion.c +10 -4

  }
  EXPORT_SYMBOL(ion_alloc);
 
-void ion_free(struct ion_client *client, struct ion_handle *handle)
+static void ion_free_nolock(struct ion_client *client, struct
+ion_handle *handle)
  {
  	bool valid_handle;
 
  	BUG_ON(client != handle->client);
 
-	mutex_lock(&client->lock);
  	valid_handle = ion_handle_validate(client, handle);
 
  	if (!valid_handle) {
  		WARN(1, "%s: invalid handle passed to free.\n", __func__);
-		mutex_unlock(&client->lock);
  		return;
  	}
+	ion_handle_put_nolock(handle);
+}
+
+void ion_free(struct ion_client *client, struct ion_handle *handle) {
+	BUG_ON(client != handle->client);
+
+	mutex_lock(&client->lock);
+	ion_free_nolock(client, handle);
  	mutex_unlock(&client->lock);
-	ion_handle_put(handle);
  }
  EXPORT_SYMBOL(ion_free);
 

@@ -1281,11 +1306,15 @@ static long ion_ioctl(struct file *filp,

unsigned int cmd, unsigned long arg)

drivers/staging/android/ion/ion.c static long ion_ioctl(struct file *filp, · line 1310

Quoted 7 lines

drivers/staging/android/ion/ion.c +2 -2

  	{
  		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);

drivers/staging/android/ion/ion.c static long ion_ioctl(struct file *filp, · line 1320

Quoted 13 lines

drivers/staging/android/ion/ion.c +6 -2

+		if (IS_ERR(handle)) {
+			mutex_unlock(&client->lock);
  			return PTR_ERR(handle);
-		ion_free(client, handle);
-		ion_handle_put(handle);
+		}
+		ion_free_nolock(client, handle);
+		ion_handle_put_nolock(handle);
+		mutex_unlock(&client->lock);
  		break;
  	}
  	case ION_IOC_SHARE: