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

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: <20181205105820.GB16376@kroah.com>
Date:   Wed, 5 Dec 2018 11:58:20 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Todd Kjos <tkjos@...roid.com>
Cc:     tkjos@...gle.com, arve@...roid.com, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, maco@...gle.com,
        joel@...lfernandes.org, Martijn Coenen <maco@...roid.com>,
        kernel-team@...roid.com, Jann Horn <jannh@...gle.com>
Subject: Re: [PATCH] binder: fix use-after-free due to fdget() optimization

On Mon, Dec 03, 2018 at 12:24:57PM -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().
> 
> Suggested-by: Jann Horn <jannh@...gle.com>
> Signed-off-by: Todd Kjos <tkjos@...gle.com>
> Acked-by: Martijn Coenen <maco@...roid.com>

Shouldn't this go to 4.20-final?  And have a stable@ tag?
And a "Fixes:" tag?

thanks,

greg k-h