summaryrefslogtreecommitdiff
path: root/drivers/staging
diff options
context:
space:
mode:
authorShankar Brahadeeswaran <shankoo77@gmail.com>2013-02-20 18:11:26 (GMT)
committerGreg Kroah-Hartman <gregkh@linuxfoundation.org>2013-03-05 08:35:22 (GMT)
commite5834d620d61d01444c41958f1816d688a96433c (patch)
tree88cf0b0836f60eff19298af6152c33c4fe32baa7 /drivers/staging
parent573632c2eaf87429a89490173f34682bb71f6883 (diff)
downloadlinux-fsl-qoriq-e5834d620d61d01444c41958f1816d688a96433c.tar.xz
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: Shankar Brahadeeswaran <shankoo77@gmail.com> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com> Acked-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Diffstat (limited to 'drivers/staging')
-rw-r--r--drivers/staging/android/ashmem.c45
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;
}