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.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
