From 0a9dea28170c90a7265250b66cc0df1253b0c501 Mon Sep 17 00:00:00 2001 From: Todd Poynor Date: Tue, 4 Jun 2013 17:29:38 -0700 Subject: [PATCH] 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 Conflicts: drivers/staging/android/ashmem.c --- drivers/staging/android/ashmem.c | 45 ++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 4a7db762a2e1..3511b0840362 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -224,21 +224,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; } @@ -405,6 +413,7 @@ out: static int set_name(struct ashmem_area *asma, void __user *name) { + int len; int ret = 0; char local_name[ASHMEM_NAME_LEN]; @@ -417,21 +426,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; } -- 2.34.1