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@xxxxxxxxxxxxxxxxxx> 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.