linux-kernel - Re: [PATCH v2] binder: fix proc->files use-after-free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHRSSEykc-3Ow-3CAvQcPTWOB7=jN0vdqbdWDSsahV_CCEiKoQ@mail.gmail.com>
Date:   Mon, 20 Nov 2017 08:57:10 -0800
From:   Todd Kjos <tkjos@...gle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Todd Kjos <tkjos@...roid.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Martijn Coenen <maco@...gle.com>
Subject: Re: [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 <viro@...iv.linux.org.uk> 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.