All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alice Ryhl <aliceryhl@google.com>
To: Gary Guo <gary@garyguo.net>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Carlos Llamas" <cmllamas@google.com>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Boqun Feng" <boqun@kernel.org>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Danilo Krummrich" <dakr@kernel.org>,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Jann Horn" <jannh@google.com>
Subject: Re: [PATCH v2 4/4] rust_binder: check current before closing fds
Date: Fri, 27 Feb 2026 13:02:28 +0000	[thread overview]
Message-ID: <aaGV5NR9qo4Iah5C@google.com> (raw)
In-Reply-To: <DGPQSQCRYS7E.2443Q5A151I7H@garyguo.net>

On Fri, Feb 27, 2026 at 12:28:04PM +0000, Gary Guo wrote:
> On Fri Feb 27, 2026 at 9:34 AM GMT, Alice Ryhl wrote:
> > This list gets populated once the transaction is delivered to the target
> > process, at which point it's not touched again except in BC_FREE_BUFFER
> > and process exit, so if the list has been populated then this code
> > should not run in the context of the wrong userspace process.
> >
> > However, why tempt fate? The function itself can run in the context of
> > both the sender and receiver, and if someone can engineer a scenario
> > where it runs in the sender and this list is non-empty (or future Rust
> > Binder changes make such a scenario possible), then that'd be a problem
> > because we'd be closing random unrelated fds in the wrong process.
> >
> > Suggested-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  drivers/android/binder/allocation.rs | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/android/binder/allocation.rs b/drivers/android/binder/allocation.rs
> > index 7f65a9c3a0e5..31a42738a99d 100644
> > --- a/drivers/android/binder/allocation.rs
> > +++ b/drivers/android/binder/allocation.rs
> > @@ -260,6 +260,10 @@ fn drop(&mut self) {
> >                  }
> >              }
> >  
> > +            if self.process.task != kernel::current!().group_leader() {
> > +                // Called from wrong task, so do not free fds.
> > +                info.file_list.close_on_free.clear();
> > +            }
> 
> If you're sure that this won't actually happen, perhaps print a warning if it's
> called from a different task but the list is not empty?
> 
> Also, I think this can be
> 
> if ... {
>     for &fd in ... {
>     }
> }
> 
> rather than `.clear()` and then iterate.

Actually, I guess there is one case. When the binder fd is closed, this
code may run from deferred_release() in workqueue context. Now, the fd
close logic is a no-op from workqueue context, so this patch still
doesn't change behavior, but it means the fds wont get closed.

That said, this actually matches C Binder behavior. It also does not
close BINDER_TYPE_FDA fds when you close the Binder fd without first
issuing the cleanup command for each live BINDER_TYPE_FDA object.
Probably not intentional, though.

Alice

      reply	other threads:[~2026-02-27 13:02 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-27  9:34 [PATCH v2 0/4] rust_binder: check current before closing fds Alice Ryhl
2026-02-27  9:34 ` [PATCH v2 1/4] rust: sync: implement == operator for ARef Alice Ryhl
2026-02-27 12:23   ` Gary Guo
2026-02-27  9:34 ` [PATCH v2 2/4] rust: task: implement == operator for Task Alice Ryhl
2026-02-27 12:24   ` Gary Guo
2026-02-27  9:34 ` [PATCH v2 3/4] rust_binder: make use of == " Alice Ryhl
2026-02-27 12:24   ` Gary Guo
2026-02-27  9:34 ` [PATCH v2 4/4] rust_binder: check current before closing fds Alice Ryhl
2026-02-27 12:28   ` Gary Guo
2026-02-27 13:02     ` Alice Ryhl [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aaGV5NR9qo4Iah5C@google.com \
    --to=aliceryhl@google.com \
    --cc=a.hindborg@kernel.org \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun@kernel.org \
    --cc=cmllamas@google.com \
    --cc=dakr@kernel.org \
    --cc=gary@garyguo.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=jannh@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tmgross@umich.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.