diff options
author | Todd Kjos <tkjos@google.com> | 2017-11-10 15:30:27 -0800 |
---|---|---|
committer | Todd Kjos <tkjos@google.com> | 2017-11-15 12:18:22 -0800 |
commit | 19ef30ef23a4de3f0bd8979a5bb5216dd381bc75 (patch) | |
tree | 566dfed8416fa25f55be10b5abc9213c6acf7fd8 /drivers/android | |
parent | 03d4af8271682742aa21bbbf3032a839764f591a (diff) |
FROMLIST: binder: fix proc->files use-after-free
(from https://patchwork.kernel.org/patch/10058587/)
proc->files cleanup is initiated by binder_vma_close. Therefore
a reference on the binder_proc is not enough to prevent the
files_struct from being released while the binder_proc still has
a reference. This can lead to an attempt to dereference the
stale pointer obtained from proc->files prior to proc->files
cleanup. This has been seen once in task_get_unused_fd_flags()
when __alloc_fd() is called with a stale "files".
The fix is to always use get_files_struct() to obtain struct_files
so that the refcount on the files_struct is used to prevent
a premature free. proc->files is removed since we get it every
time.
Bug: 69164715
Change-Id: I6431027d3d569e76913935c21885201505627982
Signed-off-by: Todd Kjos <tkjos@google.com>
Diffstat (limited to 'drivers/android')
-rw-r--r-- | drivers/android/binder.c | 63 |
1 files changed, 30 insertions, 33 deletions
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 1177a6617964..008e448536de 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -466,9 +466,8 @@ struct binder_ref { }; enum binder_deferred_state { - BINDER_DEFERRED_PUT_FILES = 0x01, - BINDER_DEFERRED_FLUSH = 0x02, - BINDER_DEFERRED_RELEASE = 0x04, + BINDER_DEFERRED_FLUSH = 0x01, + BINDER_DEFERRED_RELEASE = 0x02, }; /** @@ -505,8 +504,6 @@ struct binder_priority { * (invariant after initialized) * @tsk task_struct for group_leader of process * (invariant after initialized) - * @files files_struct for process - * (invariant after initialized) * @deferred_work_node: element for binder_deferred_list * (protected by binder_deferred_lock) * @deferred_work: bitmap of deferred work to perform @@ -553,7 +550,6 @@ struct binder_proc { struct list_head waiting_threads; int pid; struct task_struct *tsk; - struct files_struct *files; struct hlist_node deferred_work_node; int deferred_work; bool is_dead; @@ -949,22 +945,34 @@ static void binder_free_thread(struct binder_thread *thread); static void binder_free_proc(struct binder_proc *proc); static void binder_inc_node_tmpref_ilocked(struct binder_node *node); +struct files_struct *binder_get_files_struct(struct binder_proc *proc) +{ + return get_files_struct(proc->tsk); +} + static int task_get_unused_fd_flags(struct binder_proc *proc, int flags) { - struct files_struct *files = proc->files; + struct files_struct *files; unsigned long rlim_cur; unsigned long irqs; + int ret; + files = binder_get_files_struct(proc); if (files == NULL) return -ESRCH; - if (!lock_task_sighand(proc->tsk, &irqs)) - return -EMFILE; + if (!lock_task_sighand(proc->tsk, &irqs)) { + ret = -EMFILE; + goto err; + } rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE); unlock_task_sighand(proc->tsk, &irqs); - return __alloc_fd(files, 0, rlim_cur, flags); + ret = __alloc_fd(files, 0, rlim_cur, flags); +err: + put_files_struct(files); + return ret; } /* @@ -973,8 +981,12 @@ static int task_get_unused_fd_flags(struct binder_proc *proc, int flags) static void task_fd_install( struct binder_proc *proc, unsigned int fd, struct file *file) { - if (proc->files) - __fd_install(proc->files, fd, file); + struct files_struct *files = binder_get_files_struct(proc); + + if (files) { + __fd_install(files, fd, file); + put_files_struct(files); + } } /* @@ -982,18 +994,20 @@ static void task_fd_install( */ static long task_close_fd(struct binder_proc *proc, unsigned int fd) { + struct files_struct *files = binder_get_files_struct(proc); int retval; - if (proc->files == NULL) + if (files == NULL) return -ESRCH; - retval = __close_fd(proc->files, fd); + retval = __close_fd(files, fd); /* can't restart close syscall because file table entry was cleared */ if (unlikely(retval == -ERESTARTSYS || retval == -ERESTARTNOINTR || retval == -ERESTARTNOHAND || retval == -ERESTART_RESTARTBLOCK)) retval = -EINTR; + put_files_struct(files); return retval; } @@ -4818,7 +4832,6 @@ static void binder_vma_close(struct vm_area_struct *vma) (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags, (unsigned long)pgprot_val(vma->vm_page_prot)); binder_alloc_vma_close(&proc->alloc); - binder_defer_work(proc, BINDER_DEFERRED_PUT_FILES); } static int binder_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf) @@ -4860,10 +4873,8 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma) vma->vm_private_data = proc; ret = binder_alloc_mmap_handler(&proc->alloc, vma); - if (ret) - return ret; - proc->files = get_files_struct(current); - return 0; + + return ret; err_bad_arg: pr_err("binder_mmap: %d %lx-%lx %s failed %d\n", @@ -5042,8 +5053,6 @@ static void binder_deferred_release(struct binder_proc *proc) struct rb_node *n; int threads, nodes, incoming_refs, outgoing_refs, active_transactions; - BUG_ON(proc->files); - mutex_lock(&binder_procs_lock); hlist_del(&proc->proc_node); mutex_unlock(&binder_procs_lock); @@ -5125,8 +5134,6 @@ static void binder_deferred_release(struct binder_proc *proc) static void binder_deferred_func(struct work_struct *work) { struct binder_proc *proc; - struct files_struct *files; - int defer; do { @@ -5143,21 +5150,11 @@ static void binder_deferred_func(struct work_struct *work) } mutex_unlock(&binder_deferred_lock); - files = NULL; - if (defer & BINDER_DEFERRED_PUT_FILES) { - files = proc->files; - if (files) - proc->files = NULL; - } - if (defer & BINDER_DEFERRED_FLUSH) binder_deferred_flush(proc); if (defer & BINDER_DEFERRED_RELEASE) binder_deferred_release(proc); /* frees proc */ - - if (files) - put_files_struct(files); } while (proc); } static DECLARE_WORK(binder_deferred_work, binder_deferred_func); |