From c1d43119ece86ebfa918ed1e979a1ab6a5dcdaeb 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 --- drivers/staging/android/ashmem.c | 60 ++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index bda20bf15948..c49480ebf85f 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -221,21 +221,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; } + 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_prot_mask(struct ashmem_area *asma, unsigned long prot) 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; }