staging: ashmem: Avoid deadlock between read and mmap calls

Avoid holding ashmem_mutex across code that can page fault.  Page faults
grab the mmap_sem for the process, which are also held by mmap calls
prior to calling ashmem_mmap, which locks ashmem_mutex.  The reversed
order of locking between the two can deadlock.

The calls that can page fault are read() and the ASHMEM_SET_NAME and
ASHMEM_GET_NAME ioctls.  Move the code that accesses userspace pages
outside the ashmem_mutex.

Cc: Colin Cross <ccross@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
[jstultz: minor commit message tweaks]
Signed-off-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 23948f16701206926ec9a76ce347641316d692c5..713a9722678746f24363e3b9f3f4a0f89cc145ad 100644 (file)
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -295,21 +295,29 @@ static ssize_t ashmem_read(struct file *file, char __user *buf,
 
        /* If size is not set, or set to 0, always return EOF. */
        if (asma->size == 0)
-               goto out;
+               goto out_unlock;
 
        if (!asma->file) {
                ret = -EBADF;
-               goto out;
+               goto out_unlock;
        }
 
-       ret = asma->file->f_op->read(asma->file, buf, len, pos);
-       if (ret < 0)
-               goto out;
+       mutex_unlock(&ashmem_mutex);
 
-       /** Update backing file pos, since f_ops->read() doesn't */
-       asma->file->f_pos = *pos;
+       /*
+        * asma and asma->file are used outside the lock here.  We assume
+        * once asma->file is set it will never be changed, and will not
+        * be destroyed until all references to the file are dropped and
+        * ashmem_release is called.
+        */
+       ret = asma->file->f_op->read(asma->file, buf, len, pos);
+       if (ret >= 0) {
+               /** Update backing file pos, since f_ops->read() doesn't */
+               asma->file->f_pos = *pos;
+       }
+       return ret;
 
-out:
+out_unlock:
        mutex_unlock(&ashmem_mutex);
        return ret;
 }
@@ -498,6 +506,7 @@ out:
 
 static int set_name(struct ashmem_area *asma, void __user *name)
 {
+       int len;
        int ret = 0;
        char local_name[ASHMEM_NAME_LEN];
 
@@ -510,21 +519,19 @@ static int set_name(struct ashmem_area *asma, void __user *name)
         * 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;
-
+       len = strncpy_from_user(local_name, name, ASHMEM_NAME_LEN);
+       if (len < 0)
+               return len;
+       if (len == ASHMEM_NAME_LEN)
+               local_name[ASHMEM_NAME_LEN - 1] = '\0';
        mutex_lock(&ashmem_mutex);
        /* cannot change an existing mapping's name */
-       if (unlikely(asma->file)) {
+       if (unlikely(asma->file))
                ret = -EINVAL;
-               goto out;
-       }
-       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);
+       else
+               strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
 
+       mutex_unlock(&ashmem_mutex);
        return ret;
 }