[PATCH] staging: android: ashmem: get_name,set_name not to hold ashmem_mutex

[PATCH] staging: android: ashmem: get_name,set_name not to hold ashmem_mutex

[Shankar Brahadeeswaran]

[-- Attachment #1: Type: text/plain, Size: 2362 bytes --]

Hi,
I'm working on Android Linux Kernel (version 3.4) and seeing a
"deadlock" in the ashmem driver, while handling mmap request.
I seek your support to fix the same. The locks that involved in the
dead lock are
1) mm->mmap_sem
2) ashmem_mutex

The following is the sequence of events that leads to the deadlock.
There are two threads A and B that belong to the same process
(system_server) and hence share the mm struct.
A1) In the A's context an mmap system call is made with an fd of ashmem
A2) The system call sys_mmap_pgoff acquires the mmap_sem of the "mm"
and sleeps before calling the .mmap of ashmem i.e before calling
ashmem_mmap and acquiring the ashmem_mutex from this function.

Now the thread B runs and proceeds to do the following
B1) In the B's context ashmem ioctl with option ASHMEM_GET_NAME is called.
B2) Now the code proceeds to acquire the ashmem_mutex and performs a
"copy_to_user"
B3) copy_to_user raises a valid exception to copy the data from user
space and proceeds to handle it gracefully,
do_DataAbort --> do_page_fault (This condition can happen if the
physical page for the give user address is not mapped yet).
B4) In do_page_fault it finds that the mm->mmap_sem is not available
(Note A & B share the mm) since A has it and sleeps

Now the thread A runs again
A3) It proceeds to call ashmem_mmap and tries to acquired
ashmem_mutex, which is not available (is with B) and sleeps.

Now A has acquired mmap_sem and waits for B to release ashmem_mutex
B has acquired ashmem_mutex and waits for the mmap_sem to be
available, which is held by A.
This creates a dead lock in the system.

Proposed Solution:
Do not hold the ashmem lock while making copy_to_user/copy_from_user calls.
At the same ensure that data integrity is maintained (two threads
calling SET_NAME/GET_NAME should not lead to issues).
I have attached a patch to fix this problem.

How to Reproduce the problem:
In normal circumstances it is very hard to see this. The problem was
seen during regression tests, but was very rare.
My Kernel Version is 3.4.0 and I managed to write a unit test case to
create this issue almost 100% of the time.

Testing:
Used the same unit test case to ensure that the attached patch fixes
the deadlock in Kernel Version 3.4
Ported the same patch to 3.8 for submission.

Please provide your feedback on the patch.

Warm Regards,
Shankar

[-- Attachment #2: 0001-staging-android-ashmem-get_name-set_name-not-to-hold.patch --]
[-- Type: application/octet-stream, Size: 4399 bytes --]

From 836a896fc18eb27c6aec2b320ef01c17dab7c96d Mon Sep 17 00:00:00 2001
From: Shankar Brahadeeswaran <shankoo77@gmail.com>
Date: Mon, 18 Feb 2013 10:21:56 +0530
Subject: [PATCH] staging: android: ashmem: get_name,set_name not to hold ashmem_mutex

Problem:
There exists a path in ashmem driver that could lead to acquistion
of mm->mmap_sem, ashmem_mutex in reverse order. This could lead
to deadlock in the system.
For Example, assume that mmap is called on a ashmem region
in the context of a thread say T1.
 sys_mmap_pgoff (1. acquires mm->mmap_sem)
  |
   --> mmap_region
 	|
         ----> ashmem_mmap (2. acquires asmem_mutex)
 Now if there is a context switch after 1 and before 2,
 and if another thread T2 (that shares the mm struct) invokes an
 ioctl say ASHMEM_GET_NAME, this can lead to the following path

ashmem_ioctl
  |
  -->get_name (3. acquires ashmem_mutex)
	|
	---> copy_to_user (4. acquires the mm->mmap_sem)
Note that the copy_to_user could lead to a valid fault if no
physical page is allocated yet for the user address passed.
Now T1 has mmap_sem and is waiting for ashmem_mutex.
and T2 has the ashmem_mutex and is waiting for mmap_sem
Thus leading to deadlock.

Solution:
Do not call copy_to_user or copy_from_user while holding the
ahsmem_mutex. Instead copy this to a local buffer that lives
in the stack while holding this lock. This will maintain data
integrity as well never reverse the lock order.
---
 drivers/staging/android/ashmem.c |   48 ++++++++++++++++++++++++++++++-------
 1 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 634b9ae..9ddde92 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -414,6 +414,11 @@ out:
 static int set_name(struct ashmem_area *asma, void __user *name)
 {
 	int ret = 0;
+	/*
+	 * Local variable to hold the name copied from user space
+	 * This variable is approx of size 270 bytes, not huge
+	 */
+	char local_name[ASHMEM_NAME_LEN];
 
 	mutex_lock(&ashmem_mutex);
 
@@ -423,9 +428,24 @@ static int set_name(struct ashmem_area *asma, void __user *name)
 		goto out;
 	}
 
-	if (unlikely(copy_from_user(asma->name + ASHMEM_NAME_PREFIX_LEN,
-				    name, ASHMEM_NAME_LEN)))
+	/*
+	 * Holding the ashmem_mutex while doing a copy_from_user might cause
+	 * an data abourt which would try to access mmap_sem. If another
+	 * thread has invoked ashmem_mmap then it will be holding the
+	 * semaphore and will be waiting for ashmem_mutex, there by leading to
+	 * deadlock. We'll release the mutex  and take the name to a local
+	 * variable that does not need protection and later copy the local
+	 * variable to the structure member with lock held.
+	 */
+	mutex_unlock(&ashmem_mutex);
+	if (unlikely(copy_from_user(local_name, name, ASHMEM_NAME_LEN))) {
+		/* Lock is already dropped, just return */
 		ret = -EFAULT;
+		return ret;
+	}
+	mutex_lock(&ashmem_mutex);
+	memcpy(asma->name + ASHMEM_NAME_PREFIX_LEN,
+		local_name, ASHMEM_NAME_LEN);
 	asma->name[ASHMEM_FULL_NAME_LEN-1] = '\0';
 
 out:
@@ -437,26 +457,36 @@ out:
 static int get_name(struct ashmem_area *asma, void __user *name)
 {
 	int ret = 0;
+	size_t len;
+	/*
+	 * Have a local variable to which we'll copy the content
+	 * from asma with the lock held. Later we can copy this to the user
+	 * space safely without holding any locks. So even if we proceed to
+	 * wait for mmap_sem, it won't lead to deadlock.
+	 */
+	char local_name[ASHMEM_NAME_LEN];
 
 	mutex_lock(&ashmem_mutex);
 	if (asma->name[ASHMEM_NAME_PREFIX_LEN] != '\0') {
-		size_t len;
 
 		/*
 		 * Copying only `len', instead of ASHMEM_NAME_LEN, bytes
 		 * prevents us from revealing one user's stack to another.
 		 */
 		len = strlen(asma->name + ASHMEM_NAME_PREFIX_LEN) + 1;
-		if (unlikely(copy_to_user(name,
-				asma->name + ASHMEM_NAME_PREFIX_LEN, len)))
-			ret = -EFAULT;
+		memcpy(local_name, asma->name + ASHMEM_NAME_PREFIX_LEN, len);
 	} else {
-		if (unlikely(copy_to_user(name, ASHMEM_NAME_DEF,
-					  sizeof(ASHMEM_NAME_DEF))))
-			ret = -EFAULT;
+		len = sizeof(ASHMEM_NAME_DEF);
+		memcpy(local_name, ASHMEM_NAME_DEF, len);
 	}
 	mutex_unlock(&ashmem_mutex);
 
+	/*
+	 * Now we are just copying from the stack variable to userland
+	 * No lock held
+	 */
+	if (unlikely(copy_to_user(name, local_name, len)))
+		ret = -EFAULT;
 	return ret;
 }
 
-- 
1.7.0.4

Re: [PATCH] staging: android: ashmem: get_name, set_name not to hold ashmem_mutex

[Dan Carpenter]

Good job fixing the bug.  :)

My one concern would be that in set_name() there is a race caused
by dropping the lock.  It would be better to do that
copy_from_user() first, before taking the lock.  I don't expect this
to actually be a problem in real life.

+       /*
+        * Local variable to hold the name copied from user space
+        * This variable is approx of size 270 bytes, not huge
+        */
+       char local_name[ASHMEM_NAME_LEN];

These obvious comments are not needed.  We trust you do not overflow
the stack.  ;)  Also it's checked automatically during build.

Looks good generally.

regards,
dan carpenter

Re: [PATCH] staging: android: ashmem: get_name, set_name not to hold ashmem_mutex

[Shankar Brahadeeswaran]

Hi Dan,
Thanks for the feedback. I have incorporated both your review comments
and have re-tested the patch.
I'll submit the patch for approval.

Warm Regards,
Shankar


On Tue, Feb 19, 2013 at 7:08 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Good job fixing the bug.  :)
>
> My one concern would be that in set_name() there is a race caused
> by dropping the lock.  It would be better to do that
> copy_from_user() first, before taking the lock.  I don't expect this
> to actually be a problem in real life.
>
> +       /*
> +        * Local variable to hold the name copied from user space
> +        * This variable is approx of size 270 bytes, not huge
> +        */
> +       char local_name[ASHMEM_NAME_LEN];
>
> These obvious comments are not needed.  We trust you do not overflow
> the stack.  ;)  Also it's checked automatically during build.
>
> Looks good generally.
>
> regards,
> dan carpenter
>