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

[PATCH v3 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 v3:
- Apply 'if' statement around for loop.
- Link to v2: https://lore.kernel.org/r/20260227-close-fd-check-current-v2-0-b3347edbb310@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 | 29 ++++++++++++++++-------------
 drivers/android/binder/process.rs    |  4 ++--
 rust/kernel/sync/aref.rs             | 22 ++++++++++++++++++++++
 rust/kernel/task.rs                  |  9 +++++++++
 4 files changed, 49 insertions(+), 15 deletions(-)
---
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
change-id: 20260219-close-fd-check-current-98e3ee59880a

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

[PATCH v3 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>.

Reviewed-by: Gary Guo <gary@garyguo.net>
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.1018.g2bb0e51243-goog

[PATCH v3 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.

Reviewed-by: Gary Guo <gary@garyguo.net>
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.1018.g2bb0e51243-goog

[PATCH v3 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.

Reviewed-by: Gary Guo <gary@garyguo.net>
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.1018.g2bb0e51243-goog

[PATCH v3 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.

Note that on process exit, the == comparison may actually fail because
it's called from a kthread. The fd closing code is a no-op on kthreads,
so there is no actual behavior different though.

Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/allocation.rs | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/android/binder/allocation.rs b/drivers/android/binder/allocation.rs
index 7f65a9c3a0e5..691e4af5c1d0 100644
--- a/drivers/android/binder/allocation.rs
+++ b/drivers/android/binder/allocation.rs
@@ -260,19 +260,22 @@ fn drop(&mut self) {
                 }
             }
 
-            for &fd in &info.file_list.close_on_free {
-                let closer = match DeferredFdCloser::new(GFP_KERNEL) {
-                    Ok(closer) => closer,
-                    Err(kernel::alloc::AllocError) => {
-                        // Ignore allocation failures.
-                        break;
-                    }
-                };
-
-                // Here, we ignore errors. The operation can fail if the fd is not valid, or if the
-                // method is called from a kthread. However, this is always called from a syscall,
-                // so the latter case cannot happen, and we don't care about the first case.
-                let _ = closer.close_fd(fd);
+            if self.process.task == kernel::current!().group_leader() {
+                for &fd in &info.file_list.close_on_free {
+                    let closer = match DeferredFdCloser::new(GFP_KERNEL) {
+                        Ok(closer) => closer,
+                        Err(kernel::alloc::AllocError) => {
+                            // Ignore allocation failures.
+                            break;
+                        }
+                    };
+
+                    // Here, we ignore errors. The operation can fail if the fd is not valid, or if
+                    // the method is called from a kthread. However, this is always called from a
+                    // syscall, so the latter case cannot happen, and we don't care about the first
+                    // case.
+                    let _ = closer.close_fd(fd);
+                }
             }
 
             if info.clear_on_free {

-- 
2.53.0.1018.g2bb0e51243-goog