Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181205211601.75856-1-tkjos@google.com> <20181205220035.GX2217@ZenIV.linux.org.uk>
In-Reply-To: <20181205220035.GX2217@ZenIV.linux.org.uk>
From:   Todd Kjos <tkjos@google.com>
Date:   Wed, 5 Dec 2018 16:21:55 -0800
Message-ID: <CAHRSSEwpYw312kBU4WGHs_P+xqx8cP+iripYVHxpV6pJBbH9Mw@mail.gmail.com>
Subject: Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Todd Kjos <tkjos@android.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Martijn Coenen <maco@google.com>, joel@joelfernandes.org,
        Android Kernel Team <kernel-team@android.com>,
        Jann Horn <jannh@google.com>, Martijn Coenen <maco@android.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Dec 5, 2018 at 2:00 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Dec 05, 2018 at 01:16:01PM -0800, Todd Kjos wrote:
> > 44d8047f1d87a ("binder: use standard functions to allocate fds")
> > exposed a pre-existing issue in the binder driver.
> >
> > fdget() is used in ksys_ioctl() as a performance optimization.
> > One of the rules associated with fdget() is that ksys_close() must
> > not be called between the fdget() and the fdput(). There is a case
> > where this requirement is not met in the binder driver (and possibly
> > other drivers) which results in the reference count dropping to 0
> > when the device is still in use. This can result in use-after-free
> > or other issues.
> >
> > This was observed with the following sequence of events:
> >
> > Task A and task B are connected via binder; task A has /dev/binder open at
> > file descriptor number X. Both tasks are single-threaded.
> >
> > 1. task B sends a binder message with a file descriptor array
> >    (BINDER_TYPE_FDA) containing one file descriptor to task A
> > 2. task A reads the binder message with the translated file
> >    descriptor number Y
> > 3. task A uses dup2(X, Y) to overwrite file descriptor Y with
> >    the /dev/binder file
> > 4. task A unmaps the userspace binder memory mapping; the reference
> >    count on task A's /dev/binder is now 2
> > 5. task A closes file descriptor X; the reference count on task
> >    A's /dev/binder is now 1
> > 6. task A forks off a child, task C, duplicating the file descriptor
> >    table; the reference count on task A's /dev/binder is now 2
> > 7. task A invokes the BC_FREE_BUFFER command on file descriptor X
> >    to release the incoming binder message
> > 8. fdget() in ksys_ioctl() suppresses the reference count increment,
> >    since the file descriptor table is not shared
> > 9. the BC_FREE_BUFFER handler removes the file descriptor table
> >    entry for X and decrements the reference count of task A's
> >    /dev/binder file to 1
> > 10.task C calls close(X), which drops the reference count of
> >    task A's /dev/binder to 0 and frees it
> > 11.task A continues processing of the ioctl and accesses some
> >    property of e.g. the binder_proc => KASAN-detectable UAF
> >
> > Fixed by using get_file() / fput() in binder_ioctl().
>
> Note that this patch does *not* remove the nasty trap caused by the garbage
> in question - struct file can be freed before we even return from
> ->unlocked_ioctl().  Could you describe in details the desired behaviour
> of this interface?

The ioctl(BC_FREE_BUFFER) frees the buffer memory associated with a
transaction that has completed processing in userspace. If the buffer
contains an FDA object (file-descriptor array), then it closes all of the
fds passed in the transaction using ksys_close(). In the case with the
issue, the fd associated with the binder driver has been passed in the
array. Since the fdget() optimization didn't increment the reference, this
makes us vulnerable to the UAF described above since the rules for
fdget() are being violated (ksys_close()). This change did prevent the
final close during the handling of BC_FREE_BUFFER, but as you point
out, may still result in the final close being processed prematurely after
the new fput() (no observed negative side-effects right now, but agreed
this could be an issue).

>
> How about grabbing the references to all victims (*before* screwing with
> ksys_close()), sticking them into a structure with embedded callback_head
> and using task_work_add() on it, the callback doing those fput()?
>
> The callback would trigger before the return to userland, so observable
> timing of the final close wouldn't be changed.  And it would avoid the
> kludges like this.

I'll rework it according to your suggestion. I had hoped to do this in a way
that doesn't require adding calls to non-exported functions since we are
trying to clean up binder (I hear you snickering) to be a better citizen and
not rely on internal functions that drivers shouldn't be using. I presume
there are no plans to export task_work_add()...

>
> Of course, the proper fix would require TARDIS and set of instruments for
> treating severe case of retrocranial inversion, so that this "ABI" would've
> never existed, but...

There are indeed many things about the binder interface we'd do differently
if we had the chance to start over...

-Todd