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.