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: <CAN5Drs0Gw6=xYVi0Naxm+A86mqckG4xjbEuvO9+UbAYHoEqYCw@mail.gmail.com>
Date: Mon, 24 Jun 2024 08:50:43 -0700
From: Yu-Ting Tseng <yutingtseng@...gle.com>
To: Alice Ryhl <aliceryhl@...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 Mon, Jun 24, 2024 at 7:25 AM Alice Ryhl <aliceryhl@...gle.com> wrote:
>
> 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.
[resending as plain text]

Thanks for looking at the change!

The code here seems correct to me. Could you please elaborate why this
looks wrong?

This part of code gets executed if freeze->work.entry is in a list.
There are two possibilities:
1. The freeze work of type BINDER_WORK_FROZEN_BINDER is in the work
queue, waiting to be picked up by binder_thread_read. Since it hasn't
been picked up yet, this code rewrites the work type to
BINDER_WORK_CLEAR_DEATH_NOTIFICATION, effectively canceling the state
change notification and instead making binder_thread_read send a
BR_CLEAR_FREEZE_NOTIFICATION_DONE to userspace. The API contract
allows coalescing of events. I can explicitly mention this case if it
helps.
2. The freeze work of type BINDER_WORK_FROZEN_BINDER is in the
proc->delivered_freeze queue. This means a state change notification
was just sent to userspace and the kernel is waiting for an ack.
freeze->sent is true. In this case we set freeze->resend to true. Once
the kernel receives the ack, it would queue up the work again, whose
type was already set to BINDER_WORK_CLEAR_DEATH_NOTIFICATION.

Yu-Ting

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