[PATCH v2] binder: fix proc->files use-after-free

[PATCH v2] binder: fix proc->files use-after-free

[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.

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c | 63 +++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index fddf76ef5bd6..ea07b35fb533 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -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);
-- 
2.15.0.448.gf294e3d99a-goog

Re: [PATCH v2] binder: fix proc->files use-after-free

[Greg KH]

On Thu, Nov 16, 2017 at 09:56:50AM -0800, 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 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 <tkjos@google.com>
> ---
>  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

Re: [PATCH v2] binder: fix proc->files use-after-free

[Todd Kjos]

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

On Thu, Nov 16, 2017 at 12:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 16, 2017 at 09:56:50AM -0800, 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 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 <tkjos@google.com>
>> ---
>>  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

Re: [PATCH v2] binder: fix proc->files use-after-free

[Greg KH]

On Thu, Nov 16, 2017 at 12:37:02PM -0800, Todd Kjos wrote:
> Sorry about that, do you want a v3 with correct annotations?

Nah, this time is fine :)

Re: [PATCH v2] binder: fix proc->files use-after-free

[Al Viro]

On Thu, Nov 16, 2017 at 09:56:50AM -0800, Todd Kjos wrote:

> +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.

Re: [PATCH v2] binder: fix proc->files use-after-free

[Todd Kjos]

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.

On Fri, Nov 17, 2017 at 11:31 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Nov 16, 2017 at 09:56:50AM -0800, Todd Kjos wrote:
>
>> +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.