binder: fix proc->files use-after-free
Todd Kjos
maintainerPATCH 1/1 binder: fix proc->files use-after-free
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.
--- drivers/android/binder.c | 63 +++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 33 deletions(-)
drivers/android/binder.c +30 -33
@@ -458,9 +458,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 , };/**
@@ -481,8 +480,6 @@ enum binder_deferred_state {* (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
@@ -529,7 +526,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;
@@ -875,22 +871,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; }/*
@@ -899,8 +907,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); + } }/*
@@ -908,18 +920,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; }
@@ -4561,7 +4575,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_fault *vmf)
@@ -4603,10 +4616,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",
@@ -4778,8 +4789,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);
@@ -4861,8 +4870,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 {
@@ -4879,21 +4886,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);
Signature · 1 line
2.15.0.448.gf294e3d99a-goog
Todd Kjos
maintainerdrivers/android/binder.c static void binder_free_thread(struct binder_thread *thread); · line 874
Quoted 5 lines
drivers/android/binder.c +1
-0@@ -875,22 +871,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)
This should be declared static:
static struct files_struct *binder_get_files_struct(struct binder_proc *proc)
Greg- should I send a "v2" for this?
Quoted 4 lines
drivers/android/binder.c +4
-0+{ +return get_files_struct (proc->tsk); +} +
Greg KH
maintainerQuoted 10 lines · Todd Kjos
drivers/android/binder.c static void binder_free_thread(struct binder_thread *thread); · line 874
Quoted 5 lines
drivers/android/binder.c +1
-0@@ -875,22 +871,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)This should be declared static:
static structfiles_struct*binder_get_files_struct(structbinder_proc*proc)Greg- should I send a "v2" for this?
Please do, I can't do anything with these until after 4.15-rc1 is out, so you have time to do it :)
thanks,
greg k-h