Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization
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). ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization
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...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] binder: fix use-after-free due to fdget() optimization
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
Signed-off-by: Todd Kjos
Acked-by: Martijn Coenen
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
