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: <CAN5Drs06fbditeSaVLc6i6wEY+A47HHzQmhCS1rzJgacNs1Tjw@mail.gmail.com>
Date: Mon, 24 Jun 2024 08:56:22 -0700
From: Yu-Ting Tseng <yutingtseng@...gle.com>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: Alice Ryhl <aliceryhl@...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 8:53 AM Carlos Llamas <cmllamas@...gle.com> wrote:
>
> On Mon, Jun 24, 2024 at 08:50:43AM -0700, Yu-Ting Tseng wrote:
> > 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
>
> Alice means you want to use BINDER_WORK_CLEAR_FREEZE_NOTIFICATION and
> not BINDER_WORK_CLEAR_DEATH_NOTIFICATION.
Ah, I see. Thanks! Will get this corrected and also look into adding a
test for it.

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