linux-kernel - Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAB0TPYGO8Nm_Qz0kzSvX69NApiPwu4xV19F=KhyLe5DO3DoLTw@mail.gmail.com>
Date:   Mon, 7 Oct 2019 11:33:53 +0200
From:   Martijn Coenen <maco@...roid.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, stable@...r.kernel.org,
        syzbot <syzkaller@...glegroups.com>,
        Mattias Nissler <mnissler@...omium.org>
Subject: Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.

On Sun, Oct 6, 2019 at 7:23 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> From: Martijn Coenen <maco@...roid.com>
>
> commit f5cb779ba16334b45ba8946d6bfa6d9834d1527f upstream.
>
> binder_poll() passes the thread->wait waitqueue that
> can be slept on for work. When a thread that uses
> epoll explicitly exits using BINDER_THREAD_EXIT,
> the waitqueue is freed, but it is never removed
> from the corresponding epoll data structure. When
> the process subsequently exits, the epoll cleanup
> code tries to access the waitlist, which results in
> a use-after-free.
>
> Prevent this by using POLLFREE when the thread exits.
>
> Signed-off-by: Martijn Coenen <maco@...roid.com>
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Cc: stable <stable@...r.kernel.org> # 4.14
> [backport BINDER_LOOPER_STATE_POLL logic as well]
> Signed-off-by: Mattias Nissler <mnissler@...omium.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
>  drivers/android/binder.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -334,7 +334,8 @@ enum {
>         BINDER_LOOPER_STATE_EXITED      = 0x04,
>         BINDER_LOOPER_STATE_INVALID     = 0x08,
>         BINDER_LOOPER_STATE_WAITING     = 0x10,
> -       BINDER_LOOPER_STATE_NEED_RETURN = 0x20
> +       BINDER_LOOPER_STATE_NEED_RETURN = 0x20,
> +       BINDER_LOOPER_STATE_POLL        = 0x40,
>  };
>
>  struct binder_thread {
> @@ -2628,6 +2629,18 @@ static int binder_free_thread(struct bin
>                 } else
>                         BUG();
>         }
> +
> +       /*
> +        * If this thread used poll, make sure we remove the waitqueue
> +        * from any epoll data structures holding it with POLLFREE.
> +        * waitqueue_active() is safe to use here because we're holding
> +        * the inner lock.

This should be "global lock" in 4.9 and 4.4 :)

Otherwise LGTM, thanks!

Martijn

> +        */
> +       if ((thread->looper & BINDER_LOOPER_STATE_POLL) &&
> +           waitqueue_active(&thread->wait)) {
> +               wake_up_poll(&thread->wait, POLLHUP | POLLFREE);
> +       }
> +
>         if (send_reply)
>                 binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
>         binder_release_work(&thread->todo);
> @@ -2651,6 +2664,8 @@ static unsigned int binder_poll(struct f
>                 return POLLERR;
>         }
>
> +       thread->looper |= BINDER_LOOPER_STATE_POLL;
> +
>         wait_for_proc_work = thread->transaction_stack == NULL &&
>                 list_empty(&thread->todo) && thread->return_error == BR_OK;
>
>
>