From 1e30c38408628f0d56e6f469910d89556201055d Mon Sep 17 00:00:00 2001 From: Shankar Brahadeeswaran Date: Wed, 20 Feb 2013 23:41:26 +0530 Subject: [PATCH] --- yaml --- r: 363287 b: refs/heads/master c: e5834d620d61d01444c41958f1816d688a96433c h: refs/heads/master i: 363285: 9879f71136fcdda727891ad77f31ddd339dda253 363283: 6fb8cc68f0e1ab47052e6f003944b5c1802ed167 363279: d0bc413474e4d50d4467e427349d8f20701f750f v: v3 --- [refs] | 2 +- trunk/drivers/staging/android/ashmem.c | 45 ++++++++++++++++++-------- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/[refs] b/[refs] index bb77bf68f6d1..14b8130521f2 100644 --- a/[refs] +++ b/[refs] @@ -1,2 +1,2 @@ --- -refs/heads/master: 573632c2eaf87429a89490173f34682bb71f6883 +refs/heads/master: e5834d620d61d01444c41958f1816d688a96433c diff --git a/trunk/drivers/staging/android/ashmem.c b/trunk/drivers/staging/android/ashmem.c index 634b9ae713e0..38a9135a2f6d 100644 --- a/trunk/drivers/staging/android/ashmem.c +++ b/trunk/drivers/staging/android/ashmem.c @@ -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; }