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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171117193146.GO21978@ZenIV.linux.org.uk>
Date:   Fri, 17 Nov 2017 19:31:46 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Todd Kjos <tkjos@...roid.com>
Cc:     tkjos@...gle.com, gregkh@...uxfoundation.org, arve@...roid.com,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        maco@...gle.com
Subject: Re: [PATCH v2] binder: fix proc->files use-after-free

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.