Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751831AbdLKVXb (ORCPT ); Mon, 11 Dec 2017 16:23:31 -0500 Received: from mail-qt0-f195.google.com ([209.85.216.195]:38029 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750886AbdLKVX3 (ORCPT ); Mon, 11 Dec 2017 16:23:29 -0500 X-Google-Smtp-Source: ACJfBovVqHNPYqn6VRzvrQYj0ITft4PLVlCvxBsg6gd1+tvtIV7w5EeUknGyZspWP/fHy07v0AU047Oyax9P67l85gE= MIME-Version: 1.0 In-Reply-To: <20171127173233.21742-1-tkjos@google.com> References: <20171127173233.21742-1-tkjos@google.com> From: Todd Kjos Date: Mon, 11 Dec 2017 13:23:28 -0800 Message-ID: Subject: Re: [PATCH v3] binder: fix proc->files use-after-free To: Todd Kjos , Greg KH , "Arve Hj??nnev??g" , devel@driverdev.osuosl.org, LKML , Martijn Coenen , viro@zeniv.linux.org.uk Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5682 Lines: 150 Greg- when this is in, we'll want it in 4.14 as well. On Mon, Nov 27, 2017 at 9:32 AM, Todd Kjos wrote: > 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 protect proc->files with a mutex to prevent cleanup > while in use. > > Signed-off-by: Todd Kjos > --- > v2: declare binder_get_files_struct as static > v3: rework to protect proc->files with a mutex instead of using get_files_struct > > Also needed in 4.14 > > drivers/android/binder.c | 44 +++++++++++++++++++++++++++++++------------- > 1 file changed, 31 insertions(+), 13 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index a73596a4f804..7c027ee61375 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -482,7 +482,8 @@ enum binder_deferred_state { > * @tsk task_struct for group_leader of process > * (invariant after initialized) > * @files files_struct for process > - * (invariant after initialized) > + * (protected by @files_lock) > + * @files_lock mutex to protect @files > * @deferred_work_node: element for binder_deferred_list > * (protected by binder_deferred_lock) > * @deferred_work: bitmap of deferred work to perform > @@ -530,6 +531,7 @@ struct binder_proc { > int pid; > struct task_struct *tsk; > struct files_struct *files; > + struct mutex files_lock; > struct hlist_node deferred_work_node; > int deferred_work; > bool is_dead; > @@ -877,20 +879,26 @@ static void binder_inc_node_tmpref_ilocked(struct binder_node *node); > > static int task_get_unused_fd_flags(struct binder_proc *proc, int flags) > { > - struct files_struct *files = proc->files; > unsigned long rlim_cur; > unsigned long irqs; > + int ret; > > - if (files == NULL) > - return -ESRCH; > - > - if (!lock_task_sighand(proc->tsk, &irqs)) > - return -EMFILE; > - > + mutex_lock(&proc->files_lock); > + if (proc->files == NULL) { > + ret = -ESRCH; > + goto err; > + } > + 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(proc->files, 0, rlim_cur, flags); > +err: > + mutex_unlock(&proc->files_lock); > + return ret; > } > > /* > @@ -899,8 +907,10 @@ 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) > { > + mutex_lock(&proc->files_lock); > if (proc->files) > __fd_install(proc->files, fd, file); > + mutex_unlock(&proc->files_lock); > } > > /* > @@ -910,9 +920,11 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd) > { > int retval; > > - if (proc->files == NULL) > - return -ESRCH; > - > + mutex_lock(&proc->files_lock); > + if (proc->files == NULL) { > + retval = -ESRCH; > + goto err; > + } > retval = __close_fd(proc->files, fd); > /* can't restart close syscall because file table entry was cleared */ > if (unlikely(retval == -ERESTARTSYS || > @@ -920,7 +932,8 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd) > retval == -ERESTARTNOHAND || > retval == -ERESTART_RESTARTBLOCK)) > retval = -EINTR; > - > +err: > + mutex_unlock(&proc->files_lock); > return retval; > } > > @@ -4605,7 +4618,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma) > ret = binder_alloc_mmap_handler(&proc->alloc, vma); > if (ret) > return ret; > + mutex_lock(&proc->files_lock); > proc->files = get_files_struct(current); > + mutex_unlock(&proc->files_lock); > return 0; > > err_bad_arg: > @@ -4629,6 +4644,7 @@ static int binder_open(struct inode *nodp, struct file *filp) > spin_lock_init(&proc->outer_lock); > get_task_struct(current->group_leader); > proc->tsk = current->group_leader; > + mutex_init(&proc->files_lock); > INIT_LIST_HEAD(&proc->todo); > proc->default_priority = task_nice(current); > binder_dev = container_of(filp->private_data, struct binder_device, > @@ -4881,9 +4897,11 @@ static void binder_deferred_func(struct work_struct *work) > > files = NULL; > if (defer & BINDER_DEFERRED_PUT_FILES) { > + mutex_lock(&proc->files_lock); > files = proc->files; > if (files) > proc->files = NULL; > + mutex_unlock(&proc->files_lock); > } > > if (defer & BINDER_DEFERRED_FLUSH) > -- > 2.15.0.417.g466bffb3ac-goog >