Skip to contentSkip to conversation

binder: fix proc->files use-after-free

PATCH Superseded Todd Kjos 4 participants 6 messages Nov 16, 2017 – Nov 20, 2017 Android drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Todd Kjos

author maintainer
#0 lore raw

PATCH v2 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.

Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c | 63 +++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 33 deletions(-)
drivers/android/binder.c +30 -33
index fddf76ef5bd6..ea07b35fb533 100644
@@ -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);
 
+static 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
maintainer
#1 lore raw
Quoted 13 lines · Todd Kjos

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.

Quoted 4 lines
Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c | 63 +++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

For a v2 patch (or v3 or whatever), you need to put below the --- line what changed from the previous version(s). Documentation/SubmittingPatches describes this pretty well :)

thanks,

greg k-h

author maintainer
#2 lore raw

Sorry about that, do you want a v3 with correct annotations?

Quoted 26 lines · Greg KH
Quoted 13 lines · Todd Kjos

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.

Quoted 4 lines
Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c | 63 +++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

For a v2 patch (or v3 or whatever), you need to put below the --- line what changed from the previous version(s). Documentation/SubmittingPatches describes this pretty well :)

thanks,

greg k-h

maintainer
#3 lore raw
Quoted 1 line · Todd Kjos

Sorry about that, do you want a v3 with correct annotations?

Nah, this time is fine :)

#4 lore raw
In reply to Todd Kjos

drivers/android/binder.c static void binder_free_thread(struct binder_thread *thread); · line 877

Quoted 4 lines · Todd Kjos
drivers/android/binder.c +4 -0
+static struct files_struct *binder_get_files_struct(struct binder_proc *proc)
+{
+	return get_files_struct(proc->tsk);
+}

Hell, _no_. You should never, ever use the result of get_files_struct() for write access. It's strictly for "let me look at other process' descriptor table". The whole reason for proc->files is that we want to keep a reference that *could* be used for modifying the damn thing. And such can be obtained only by the process itself.

The rules are:
* you can use current->files both for read and write
* you can use get_files_struct(current) to get a reference that
can be used for modifying the damn thing. Then it can be passed to any other process and used by it.
* you can use get_files_struct(some_other_task) to get a reference
that can be used for examining the descriptor table of that other task.

Violation of those rules means an exploitable race. Here's the logics fdget() and friends are based on: "I'm going to do something to file refered by descriptor N. If my descriptor table is not shared, all struct file references in it will stay around - I'm not changing it, nobody else has it as their ->current, so any additional references to that descriptor table will *not* be used for modifying it. In other words, I don't need to bump the refcount of struct file I'm about to work with - the reference from my descriptor table will keep it alive".

Your patch breaks those assumptions. NAK.

#5 lore raw

Al, thanks for the detailed feedback. I didn't know about these rules (are they written down somewhere?). I'll rework this and post a compliant v3.

Quoted 32 lines · Al Viro

drivers/android/binder.c static void binder_free_thread(struct binder_thread *thread); · line 877

Quoted 4 lines · Todd Kjos
drivers/android/binder.c +4 -0
+static struct files_struct *binder_get_files_struct(struct binder_proc *proc)
+{
+     return get_files_struct(proc->tsk);
+}

Hell, _no_. You should never, ever use the result of get_files_struct() for write access. It's strictly for "let me look at other process' descriptor table". The whole reason for proc->files is that we want to keep a reference that *could* be used for modifying the damn thing. And such can be obtained only by the process itself.

The rules are:
* you can use current->files both for read and write
* you can use get_files_struct(current) to get a reference that
can be used for modifying the damn thing. Then it can be passed to any other process and used by it.
* you can use get_files_struct(some_other_task) to get a reference
that can be used for examining the descriptor table of that other task.

Violation of those rules means an exploitable race. Here's the logics fdget() and friends are based on: "I'm going to do something to file refered by descriptor N. If my descriptor table is not shared, all struct file references in it will stay around - I'm not changing it, nobody else has it as their ->current, so any additional references to that descriptor table will *not* be used for modifying it. In other words, I don't need to bump the refcount of struct file I'm about to work with - the reference from my descriptor table will keep it alive".

Your patch breaks those assumptions. NAK.