[lkml]   [2017]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2] binder: fix proc->files use-after-free
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 <> 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.

 \ /
  Last update: 2017-11-20 17:58    [W:0.064 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site