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

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

[Shankar Brahadeeswaran]

Hi Greg,
This patch is to fix a dead lock scenario that is created in ashmem driver.
The inital patch for review was posted and I got some comments from Dan
Carpeneter.I have incorporated the changes and re-tested the
same. Submitting the final patch for your pursual.

Warm Regards,
Shankar

Shankar Brahadeeswaran (1):
  staging: android: ashmem: get_name,set_name not to hold      
    ashmem_mutex

 drivers/staging/android/ashmem.c |   44 ++++++++++++++++++++++++++++---------
 1 files changed, 33 insertions(+), 11 deletions(-)

-- 
1.7.6

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

[Shankar Brahadeeswaran]

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.

Testing:
Created a unit test case to reproduce the problem.
Used the same to test this fix on kernel version 3.4.0
Ported the same patch to 3.8

Signed-off-by: Shankar Brahadeeswaran <shankoo77@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/staging/android/ashmem.c |   44 ++++++++++++++++++++++++++++---------
 1 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 634b9ae..9c30ead 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -414,8 +414,7 @@ out:
 static int set_name(struct ashmem_area *asma, void __user *name)
 {
 	int ret = 0;
-
-	mutex_lock(&ashmem_mutex);
+	char local_name[ASHMEM_NAME_LEN];
 
 	/* cannot change an existing mapping's name */
 	if (unlikely(asma->file)) {
@@ -423,9 +422,22 @@ 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.
+	 */
+	if (unlikely(copy_from_user(local_name, name, ASHMEM_NAME_LEN))) {
 		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 +449,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.6

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

[Dan Carpenter]

On Wed, Feb 20, 2013 at 05:53:59PM +0530, Shankar Brahadeeswaran wrote:
>  static int set_name(struct ashmem_area *asma, void __user *name)
>  {
>  	int ret = 0;
> -
> -	mutex_lock(&ashmem_mutex);
> +	char local_name[ASHMEM_NAME_LEN];
>  
>  	/* cannot change an existing mapping's name */
>  	if (unlikely(asma->file)) {
> @@ -423,9 +422,22 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>  		goto out;

You're not holding the lock here so you can return directly.
Otherwise it's a double unlock.

>  	}
>  
> -	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.
> +	 */
> +	if (unlikely(copy_from_user(local_name, name, ASHMEM_NAME_LEN))) {
>  		ret = -EFAULT;
> +		return ret;

		return -EFAULT;

These weren't there in the original, only when you redid it at my
request.  :/  Sorry for that.

regards,
dan carpenter

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

[Shankar Brahadeeswaran]

Hi Dan,
Fixed the review comments and re-tested again.
Hope the patch is clean atleast this time.

Warm regards,
Shankar

Shankar Brahadeeswaran (1):
  staging: android: ashmem: get_name,set_name not to hold     
    ashmem_mutex

 drivers/staging/android/ashmem.c |   56 ++++++++++++++++++++++++-------------
 1 files changed, 36 insertions(+), 20 deletions(-)

-- 
1.7.6

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

[Shankar Brahadeeswaran]

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.

Testing:
Created a unit test case to reproduce the problem.
Used the same to test this fix on kernel version 3.4.0
Ported the same patch to 3.8

Signed-off-by: Shankar Brahadeeswaran <shankoo77@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/staging/android/ashmem.c |   56 ++++++++++++++++++++++++-------------
 1 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 634b9ae..497d044 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -413,50 +413,66 @@ out:
 
 static int set_name(struct ashmem_area *asma, void __user *name)
 {
-	int ret = 0;
-
-	mutex_lock(&ashmem_mutex);
+	char local_name[ASHMEM_NAME_LEN];
 
 	/* cannot change an existing mapping's name */
-	if (unlikely(asma->file)) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (unlikely(asma->file))
+		return -EINVAL;
 
-	if (unlikely(copy_from_user(asma->name + ASHMEM_NAME_PREFIX_LEN,
-				    name, ASHMEM_NAME_LEN)))
-		ret = -EFAULT;
-	asma->name[ASHMEM_FULL_NAME_LEN-1] = '\0';
+	/*
+	 * 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.
+	 */
+	if (unlikely(copy_from_user(local_name, name, ASHMEM_NAME_LEN)))
+		return -EFAULT;
 
-out:
+	mutex_lock(&ashmem_mutex);
+	memcpy(asma->name + ASHMEM_NAME_PREFIX_LEN,
+		local_name, ASHMEM_NAME_LEN);
+	asma->name[ASHMEM_FULL_NAME_LEN-1] = '\0';
 	mutex_unlock(&ashmem_mutex);
 
-	return ret;
+	return 0;
 }
 
 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.6

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

[Dan Carpenter]

On Wed, Feb 20, 2013 at 07:57:08PM +0530, Shankar Brahadeeswaran wrote:
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -413,50 +413,66 @@ out:
>  
>  static int set_name(struct ashmem_area *asma, void __user *name)
>  {
> -	int ret = 0;
> -
> -	mutex_lock(&ashmem_mutex);
> +	char local_name[ASHMEM_NAME_LEN];
>  
>  	/* cannot change an existing mapping's name */
> -	if (unlikely(asma->file)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (unlikely(asma->file))
> +		return -EINVAL;

Gar.  No.  Sorry I wasn't paying attention properly last time.  This
isn't right but I didn't explain things properly from the beginning.
When you drop a lock, obviously the first thing I'm going to look at
is if it introduces race conditions.  The problem is that checking
asma->file has to be done under lock and also we can't drop the lock
before we set the name.  Otherwise someone could set asma->file
while we were waiting for the copy to complete.

It should be something like this:

	char local_name[ASHMEM_NAME_LEN];
	int ret = 0;

	if (copy_from_user(local_name, name, ASHMEM_NAME_LEN))
		return -EFAULT;
	local_name[ASHMEM_FULL_NAME_LEN - 1] = '\0';

	mutex_lock(&ashmem_mutex);
	if (asma->file) {
		ret = -EINVAL;
		goto out;
	}
	memcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
	       ASHMEM_NAME_LEN);
out:
	mutex_unlock(&ashmem_mutex);

	return ret;

(I removed some calls to likely/unlikely() because putting those
around copy_from_user() is probably not going to speed anything up.)

Sorry, again for the miscommunication.

regards,
dan carpenter

[PATCH V3 1/1] staging: android: ashmem: get_name,set_name not to

[Shankar Brahadeeswaran]

Hi Dan,

Oh, now I got what you say. Thanks for your patience. 
I have done the necessary modifications.
Re-submitting the patch here

Shankar Brahadeeswaran (1):
  staging: android: ashmem: get_name,set_name      not to hold
    ashmem_mutex

 drivers/staging/android/ashmem.c |   45 +++++++++++++++++++++++++++-----------
 1 files changed, 32 insertions(+), 13 deletions(-)

-- 
1.7.6

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

[Shankar Brahadeeswaran]

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.

Testing:
Created a unit test case to reproduce the problem.
Used the same to test this fix on kernel version 3.4.0
Ported the same patch to 3.8

Signed-off-by: Shankar Brahadeeswaran <shankoo77@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/staging/android/ashmem.c |   45 +++++++++++++++++++++++++++-----------
 1 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 634b9ae..38a9135 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -414,20 +414,29 @@ out:
 static int set_name(struct ashmem_area *asma, void __user *name)
 {
 	int ret = 0;
+	char local_name[ASHMEM_NAME_LEN];
 
-	mutex_lock(&ashmem_mutex);
+	/*
+	 * Holding the ashmem_mutex while doing a copy_from_user might cause
+	 * an data abort 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.
+	 */
+	if (copy_from_user(local_name, name, ASHMEM_NAME_LEN))
+		return -EFAULT;
 
+	mutex_lock(&ashmem_mutex);
 	/* cannot change an existing mapping's name */
 	if (unlikely(asma->file)) {
 		ret = -EINVAL;
 		goto out;
 	}
-
-	if (unlikely(copy_from_user(asma->name + ASHMEM_NAME_PREFIX_LEN,
-				    name, ASHMEM_NAME_LEN)))
-		ret = -EFAULT;
+	memcpy(asma->name + ASHMEM_NAME_PREFIX_LEN,
+		local_name, ASHMEM_NAME_LEN);
 	asma->name[ASHMEM_FULL_NAME_LEN-1] = '\0';
-
 out:
 	mutex_unlock(&ashmem_mutex);
 
@@ -437,26 +446,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.6

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

[Dan Carpenter]

Look good.

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter