linux-kernel - Re: [PATCH v2] binder: frozen notification

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: <CAH5fLgjR7iLHaWGy0NAiKnOUKro7_fBLoqFFzkdjn926jUdyAA@mail.gmail.com>
Date: Mon, 24 Jun 2024 16:24:46 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Yu-Ting Tseng <yutingtseng@...gle.com>
Cc: cmllamas@...gle.com, tkjos@...gle.com, gregkh@...uxfoundation.org, 
	arve@...roid.com, maco@...roid.com, joel@...lfernandes.org, 
	brauner@...nel.org, surenb@...gle.com, kernel-team@...roid.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] binder: frozen notification

On Sat, Jun 22, 2024 at 5:22 AM Yu-Ting Tseng <yutingtseng@...gle.com> wrote:
>
> Frozen processes present a significant challenge in binder transactions.
> When a process is frozen, it cannot, by design, accept and/or respond to
> binder transactions. As a result, the sender needs to adjust its
> behavior, such as postponing transactions until the peer process
> unfreezes. However, there is currently no way to subscribe to these
> state change events, making it impossible to implement frozen-aware
> behaviors efficiently.
>
> Introduce a binder API for subscribing to frozen state change events.
> This allows programs to react to changes in peer process state,
> mitigating issues related to binder transactions sent to frozen
> processes.
>
> Implementation details:
> For a given binder_ref, the state of frozen notification can be one of
> the followings:
> 1. Userspace doesn't want a notification. binder_ref->freeze is null.
> 2. Userspace wants a notification but none is in flight.
>    list_empty(&binder_ref->freeze->work.entry) = true
> 3. A notification is in flight and waiting to be read by userspace.
>    binder_ref_freeze.sent is false.
> 4. A notification was read by userspace and kernel is waiting for an ack.
>    binder_ref_freeze.sent is true.
>
> When a notification is in flight, new state change events are coalesced into
> the existing binder_ref_freeze struct. If userspace hasn't picked up the
> notification yet, the driver simply rewrites the state. Otherwise, the
> notification is flagged as requiring a resend, which will be performed
> once userspace acks the original notification that's inflight.
>
> See https://android-review.googlesource.com/c/platform/frameworks/native/+/3070045
> for how userspace is going to use this feature.
>
> Signed-off-by: Yu-Ting Tseng <yutingtseng@...gle.com>

[...]

> +               /*
> +                * There is already a freeze notification. Take it over and rewrite
> +                * the work type. If it was already sent, flag it for re-sending;
> +                * Otherwise it's pending and will be sent soon.
> +                */
> +               freeze->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;

I have not done a comprehensive review yet, but this looks wrong.

Is there any chance that we could have a test in aosp that would have
caught this?

Alice