[PATCH v2] binder: fix use-after-free due to fdget() optimization

[PATCH v2] binder: fix use-after-free due to fdget() optimization

[Todd Kjos]

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().

Fixes: 44d8047f1d87a ("binder: use standard functions to allocate fds")
Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Acked-by: Martijn Coenen <maco@android.com>
---
v2: added "Fixes:" tag
Should be added to 4.20-final if possible

 drivers/android/binder.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9f1000d2a40c7..d6979cf7b2dad 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4733,6 +4733,13 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	unsigned int size = _IOC_SIZE(cmd);
 	void __user *ubuf = (void __user *)arg;
 
+	/*
+	 * Need a reference on filp since ksys_close() could
+	 * be called on binder fd and the fdget() used in
+	 * ksys_ioctl() might have optimized out the reference.
+	 */
+	get_file(filp);
+
 	/*pr_info("binder_ioctl: %d:%d %x %lx\n",
 			proc->pid, current->pid, cmd, arg);*/
 
@@ -4844,6 +4851,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret);
 err_unlocked:
 	trace_binder_ioctl_done(ret);
+	fput(filp);
 	return ret;
 }
 
-- 
2.20.0.rc1.387.gf8505762e3-goog

Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization

[Al Viro]

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?

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.

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...

Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization

[Todd Kjos]

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

Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization

[Al Viro]

On Wed, Dec 05, 2018 at 04:21:55PM -0800, Todd Kjos wrote:

> > 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()...

Er...  Your variant critically depends upon binder being non-modular; if it
*was* built as a module, you could
	* lose the timeslice just after your fput()
	* have another process hit the final fput() *and* close the struct file
	* now that module refcount is not pinned by anything, get rmmod remove
your module
	* have the process in binder_ioctl() regain the timeslice and find the
code under it gone.

That's one of the reasons why such kludges are brittle as hell - normally you
are guaranteed that once fdget() has succeeded, the final fput() won't happen
until fdput().  With everything that guarantees in terms of code/data not going
away under you.  This patch relies upon the lack of accesses to anything
sensitive after that fput() added into binder_ioctl().  Which is actually
true, but only because the driver is not modular...

At least this variant (task_work_add()-based) doesn't depend on anything
subtle - the lack of exports is the only problem there (IOW, it would've
worked in a module if not for that).

Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization

[Todd Kjos]

On Wed, Dec 5, 2018 at 4:40 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Dec 05, 2018 at 04:21:55PM -0800, Todd Kjos wrote:
>
> > > 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()...
>
> Er...  Your variant critically depends upon binder being non-modular; if it
> *was* built as a module, you could
>         * lose the timeslice just after your fput()
>         * have another process hit the final fput() *and* close the struct file
>         * now that module refcount is not pinned by anything, get rmmod remove
> your module
>         * have the process in binder_ioctl() regain the timeslice and find the
> code under it gone.
>
> That's one of the reasons why such kludges are brittle as hell - normally you
> are guaranteed that once fdget() has succeeded, the final fput() won't happen
> until fdput().  With everything that guarantees in terms of code/data not going
> away under you.  This patch relies upon the lack of accesses to anything
> sensitive after that fput() added into binder_ioctl().  Which is actually
> true, but only because the driver is not modular...
>
> At least this variant (task_work_add()-based) doesn't depend on anything
> subtle - the lack of exports is the only problem there (IOW, it would've
> worked in a module if not for that).

Thanks for the detailed responses. I'll rework it for v3.