linux-kernel - Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

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: <CAH5fLggS7C4QdmDFqEy5KARUj+4oNWfstyno3d43joG5haysDw@mail.gmail.com>
Date: Fri, 27 Sep 2024 09:19:12 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Arve Hjønnevåg <arve@...roid.com>, 
	Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>, 
	Joel Fernandes <joel@...lfernandes.org>, Christian Brauner <brauner@...nel.org>, 
	Suren Baghdasaryan <surenb@...gle.com>, Yu-Ting Tseng <yutingtseng@...gle.com>, linux-kernel@...r.kernel.org, 
	kernel-team@...roid.com, stable@...r.kernel.org
Subject: Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

On Fri, Sep 27, 2024 at 1:37 AM Carlos Llamas <cmllamas@...gle.com> wrote:
>
> Alice points out that binder_request_freeze_notification() should not
> return EINVAL when the relevant node is dead [1]. The node can die at
> any point even if the user input is valid. Instead, allow the request
> to be allocated but skip the initial notification for dead nodes. This
> avoids propagating unnecessary errors back to userspace.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@...r.kernel.org
> Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
> Link: https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1]
> Signed-off-by: Carlos Llamas <cmllamas@...gle.com>
> ---
>  drivers/android/binder.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 73dc6cbc1681..415fc9759249 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3856,7 +3856,6 @@ binder_request_freeze_notification(struct binder_proc *proc,
>  {
>         struct binder_ref_freeze *freeze;
>         struct binder_ref *ref;
> -       bool is_frozen;
>
>         freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
>         if (!freeze)
> @@ -3872,32 +3871,31 @@ binder_request_freeze_notification(struct binder_proc *proc,
>         }
>
>         binder_node_lock(ref->node);
> -
> -       if (ref->freeze || !ref->node->proc) {
> -               binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
> -                                 proc->pid, thread->pid,
> -                                 ref->freeze ? "already set" : "dead node");
> +       if (ref->freeze) {
> +               binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION already set\n",
> +                                 proc->pid, thread->pid);
>                 binder_node_unlock(ref->node);
>                 binder_proc_unlock(proc);
>                 kfree(freeze);
>                 return -EINVAL;
>         }
> -       binder_inner_proc_lock(ref->node->proc);
> -       is_frozen = ref->node->proc->is_frozen;
> -       binder_inner_proc_unlock(ref->node->proc);
>
>         binder_stats_created(BINDER_STAT_FREEZE);
>         INIT_LIST_HEAD(&freeze->work.entry);
>         freeze->cookie = handle_cookie->cookie;
>         freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> -       freeze->is_frozen = is_frozen;
> -
>         ref->freeze = freeze;
>
> -       binder_inner_proc_lock(proc);
> -       binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
> -       binder_wakeup_proc_ilocked(proc);
> -       binder_inner_proc_unlock(proc);
> +       if (ref->node->proc) {
> +               binder_inner_proc_lock(ref->node->proc);
> +               freeze->is_frozen = ref->node->proc->is_frozen;
> +               binder_inner_proc_unlock(ref->node->proc);
> +
> +               binder_inner_proc_lock(proc);
> +               binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> +               binder_wakeup_proc_ilocked(proc);
> +               binder_inner_proc_unlock(proc);

This is not a problem with your change ... but, why exactly are we
scheduling the BINDER_WORK_FROZEN_BINDER right after creating it? For
death notications, we only schedule it immediately if the process is
dead. So shouldn't we only schedule it if the process is not frozen?

And if the answer is that frozen notifications are always sent
immediately to notify about the current state, then we should also
send one for a dead process ... maybe. I guess a dead process is not
frozen?

> +       }
>
>         binder_node_unlock(ref->node);
>         binder_proc_unlock(proc);
> --
> 2.46.1.824.gd892dcdcdd-goog
>