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.
Bug: 9261835
Change-Id: If1322e981d29c889a56cdc9dfcbc6df2729a45e9
Signed-off-by: Todd Poynor <toddpoynor@google.com>
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index bda20bf..c49480e 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -221,21 +221,29 @@
/* 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;
}
+ mutex_unlock(&ashmem_mutex);
+
+ /*
+ * 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)
- goto out;
+ if (ret >= 0) {
+ /** Update backing file pos, since f_ops->read() doesn't */
+ asma->file->f_pos = *pos;
+ }
+ return ret;
- /** Update backing file pos, since f_ops->read() doesn't */
- asma->file->f_pos = *pos;
-
-out:
+out_unlock:
mutex_unlock(&ashmem_mutex);
return ret;
}
@@ -402,50 +410,48 @@
static int set_name(struct ashmem_area *asma, void __user *name)
{
+ char lname[ASHMEM_NAME_LEN];
+ int len;
int ret = 0;
+ len = strncpy_from_user(lname, name, ASHMEM_NAME_LEN);
+ if (len < 0)
+ return len;
+ if (len == ASHMEM_NAME_LEN)
+ lname[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;
- }
+ else
+ strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, lname);
- 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';
-
-out:
mutex_unlock(&ashmem_mutex);
-
return ret;
}
static int get_name(struct ashmem_area *asma, void __user *name)
{
int ret = 0;
+ char lname[ASHMEM_NAME_LEN];
+ size_t 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(lname, asma->name + ASHMEM_NAME_PREFIX_LEN, len);
} else {
- if (unlikely(copy_to_user(name, ASHMEM_NAME_DEF,
- sizeof(ASHMEM_NAME_DEF))))
- ret = -EFAULT;
+ len = strlen(ASHMEM_NAME_DEF) + 1;
+ memcpy(lname, ASHMEM_NAME_DEF, len);
}
mutex_unlock(&ashmem_mutex);
-
+ if (unlikely(copy_to_user(name, lname, len)))
+ ret = -EFAULT;
return ret;
}
