Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 20e0cbdf authored by Todd Kjos's avatar Todd Kjos
Browse files

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: default avatarTodd Kjos <tkjos@google.com>
parent bc94b7ac

drivers/staging/android/binder.c

+30 −33
Original line number Diff line number Diff line
@@ -462,9 +462,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,

};



/**
@@ -501,8 +500,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
@@ -549,7 +546,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;
@@ -945,22 +941,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;

}



/*
@@ -969,8 +977,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);

	}

}



/*
@@ -978,18 +990,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;

}
@@ -4814,7 +4828,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)
@@ -4856,10 +4869,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;



err_bad_arg:

	pr_err("binder_mmap: %d %lx-%lx %s failed %d\n",
@@ -5038,8 +5049,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);
@@ -5121,8 +5130,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 {
@@ -5139,21 +5146,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);