[PATCH v2 0/4] rust_binder: check current before closing fds

[PATCH v2 0/4] rust_binder: check current before closing fds

[Alice Ryhl]

Please see the last patch in this series for details.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v2:
- Note that ARef<U> == U can't be implemented due to impl overlap.
- Add #[inline] in == for Task.
- Add a patch that makes use of the new == impl for Task.
- Link to v1: https://lore.kernel.org/r/20260219-close-fd-check-current-v1-0-fcab8d8469f5@google.com

---
Alice Ryhl (4):
      rust: sync: implement == operator for ARef
      rust: task: implement == operator for Task
      rust_binder: make use of == for Task
      rust_binder: check current before closing fds

 drivers/android/binder/allocation.rs |  4 ++++
 drivers/android/binder/process.rs    |  4 ++--
 rust/kernel/sync/aref.rs             | 22 ++++++++++++++++++++++
 rust/kernel/task.rs                  |  9 +++++++++
 4 files changed, 37 insertions(+), 2 deletions(-)
---
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
change-id: 20260219-close-fd-check-current-98e3ee59880a

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH v2 1/4] rust: sync: implement == operator for ARef

[Alice Ryhl]

Rust Binder wants to perform a comparison between ARef<Task> and &Task,
so define the == operator for ARef<_> when compared with another ARef<_>
or just a reference. The operator is implemented in terms of the same
operator applied to the inner type.

Note that PartialEq<U> cannot be implemented because it would overlap
with the impl for ARef<U>.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/aref.rs | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
index 0616c0353c2b..9989f56d0605 100644
--- a/rust/kernel/sync/aref.rs
+++ b/rust/kernel/sync/aref.rs
@@ -170,3 +170,25 @@ fn drop(&mut self) {
         unsafe { T::dec_ref(self.ptr) };
     }
 }
+
+impl<T, U> PartialEq<ARef<U>> for ARef<T>
+where
+    T: AlwaysRefCounted + PartialEq<U>,
+    U: AlwaysRefCounted,
+{
+    #[inline]
+    fn eq(&self, other: &ARef<U>) -> bool {
+        T::eq(&**self, &**other)
+    }
+}
+impl<T: AlwaysRefCounted + Eq> Eq for ARef<T> {}
+
+impl<T, U> PartialEq<&'_ U> for ARef<T>
+where
+    T: AlwaysRefCounted + PartialEq<U>,
+{
+    #[inline]
+    fn eq(&self, other: &&U) -> bool {
+        T::eq(&**self, other)
+    }
+}

-- 
2.53.0.473.g4a7958ca14-goog

Re: [PATCH v2 1/4] rust: sync: implement == operator for ARef

[Gary Guo]

On Fri Feb 27, 2026 at 9:34 AM GMT, Alice Ryhl wrote:
> Rust Binder wants to perform a comparison between ARef<Task> and &Task,
> so define the == operator for ARef<_> when compared with another ARef<_>
> or just a reference. The operator is implemented in terms of the same
> operator applied to the inner type.
> 
> Note that PartialEq<U> cannot be implemented because it would overlap
> with the impl for ARef<U>.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/sync/aref.rs | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

[PATCH v2 2/4] rust: task: implement == operator for Task

[Alice Ryhl]

It's useful to compare if two tasks are the same task or not. Rust
Binder wants this to check if a certain task is equal to the group
leader of current.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/task.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index cc907fb531bc..deb948d99921 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -362,6 +362,15 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
     }
 }
 
+impl PartialEq for Task {
+    #[inline]
+    fn eq(&self, other: &Self) -> bool {
+        ptr::eq(self.as_ptr(), other.as_ptr())
+    }
+}
+
+impl Eq for Task {}
+
 impl Kuid {
     /// Get the current euid.
     #[inline]

-- 
2.53.0.473.g4a7958ca14-goog

Re: [PATCH v2 2/4] rust: task: implement == operator for Task

[Gary Guo]

On Fri Feb 27, 2026 at 9:34 AM GMT, Alice Ryhl wrote:
> It's useful to compare if two tasks are the same task or not. Rust
> Binder wants this to check if a certain task is equal to the group
> leader of current.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/task.rs | 9 +++++++++
>  1 file changed, 9 insertions(+)

[PATCH v2 3/4] rust_binder: make use of == for Task

[Alice Ryhl]

Now that we have implemented the == operator for Task, replace the two
raw pointer comparisons in Binder with the == operator.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/process.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index 41de5593197c..b78c1490e654 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -681,7 +681,7 @@ pub(crate) fn get_work_or_register<'a>(
     fn get_current_thread(self: ArcBorrow<'_, Self>) -> Result<Arc<Thread>> {
         let id = {
             let current = kernel::current!();
-            if !core::ptr::eq(current.group_leader(), &*self.task) {
+            if self.task != current.group_leader() {
                 pr_err!("get_current_thread was called from the wrong process.");
                 return Err(EINVAL);
             }
@@ -1669,7 +1669,7 @@ pub(crate) fn mmap(
         vma: &mm::virt::VmaNew,
     ) -> Result {
         // We don't allow mmap to be used in a different process.
-        if !core::ptr::eq(kernel::current!().group_leader(), &*this.task) {
+        if this.task != kernel::current!().group_leader() {
             return Err(EINVAL);
         }
         if vma.start() == 0 {

-- 
2.53.0.473.g4a7958ca14-goog

Re: [PATCH v2 3/4] rust_binder: make use of == for Task

[Gary Guo]

On Fri Feb 27, 2026 at 9:34 AM GMT, Alice Ryhl wrote:
> Now that we have implemented the == operator for Task, replace the two
> raw pointer comparisons in Binder with the == operator.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  drivers/android/binder/process.rs | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

[PATCH v2 4/4] rust_binder: check current before closing fds

[Alice Ryhl]

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();
+            }
             for &fd in &info.file_list.close_on_free {
                 let closer = match DeferredFdCloser::new(GFP_KERNEL) {
                     Ok(closer) => closer,

-- 
2.53.0.473.g4a7958ca14-goog

Re: [PATCH v2 4/4] rust_binder: check current before closing fds

[Gary Guo]

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.

Best,
Gary

>              for &fd in &info.file_list.close_on_free {
>                  let closer = match DeferredFdCloser::new(GFP_KERNEL) {
>                      Ok(closer) => closer,

Re: [PATCH v2 4/4] rust_binder: check current before closing fds

[Alice Ryhl]

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