Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e5834d62 authored by Shankar Brahadeeswaran's avatar Shankar Brahadeeswaran Committed by Greg Kroah-Hartman
Browse files

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.

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: default avatarShankar Brahadeeswaran <shankoo77@gmail.com>
Reviewed-by: default avatarDan Carpenter <dan.carpenter@oracle.com>
Acked-by: default avatarDan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 573632c2

drivers/staging/android/ashmem.c

+32 −13
Original line number Diff line number Diff line
@@ -414,20 +414,29 @@ static int set_prot_mask(struct ashmem_area *asma, unsigned long prot) 
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 @@ static int set_name(struct ashmem_area *asma, void __user *name) 
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;

}