[PATCH 0/2] Fix VMA confusion in Rust Binder

[PATCH 0/2] Fix VMA confusion in Rust Binder

[Alice Ryhl]

This series contains two bugfixes for Rust Binder. I'd like to follow
them up with better solutions by changing the VMA api, but as an
immediate fix this should work.

See the first commit for an explanation of the actual bug.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Alice Ryhl (2):
      rust_binder: check ownership before using vma
      rust_binder: avoid reading the written value in offsets array

 drivers/android/binder/page_range.rs | 78 +++++++++++++++++++++++++++---------
 drivers/android/binder/thread.rs     | 17 +++-----
 2 files changed, 64 insertions(+), 31 deletions(-)
---
base-commit: 0f2acd3148e0ef42bdacbd477f90e8533f96b2ac
change-id: 20260217-binder-vma-check-b6fca42e986c

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

[PATCH 1/2] rust_binder: check ownership before using vma

[Alice Ryhl]

When installing missing pages (or zapping them), Rust Binder will look
up the vma in the mm by address, and then call vm_insert_page (or
zap_page_range_single). However, if the vma is closed and replaced with
a different vma at the same address, this can lead to Rust Binder
installing pages into the wrong vma.

By installing the page into a writable vma, it becomes possible to write
to your own binder pages, which are normally read-only. Although you're
not supposed to be able to write to those pages, the intent behind the
design of Rust Binder is that even if you get that ability, it should not
lead to anything bad. Unfortunately, due to another bug, that is not the
case.

To fix this, I will store a pointer in vm_private_data and check that
the vma returned by vma_lookup() has the right vm_ops and
vm_private_data before trying to use the vma. This should ensure that
Rust Binder will refuse to interact with any other VMA. I will follow up
this patch with more vma abstractions to avoid this unsafe access to
vm_ops and vm_private_data, but for now I'd like to start with the
simplest possible fix.

C Binder performs the same check in a slightly different way: it
provides a vm_ops->close that sets a boolean to true, then checks that
boolean after calling vma_lookup(), but I think this is more fragile
than the solution in this patch. (We probably still want to do both, but
I'll add the vm_ops->close callback with the follow-up vma API changes.)

Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/page_range.rs | 78 +++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 20 deletions(-)

diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
index fdd97112ef5c8b2341e498dc3567b659f05e3fd7..90bab18961443c6e59699cb7345e41e0db80f0dd 100644
--- a/drivers/android/binder/page_range.rs
+++ b/drivers/android/binder/page_range.rs
@@ -142,6 +142,27 @@ pub(crate) struct ShrinkablePageRange {
     _pin: PhantomPinned,
 }
 
+// We do not define any ops. For now, used only to check identity of vmas.
+static BINDER_VM_OPS: bindings::vm_operations_struct = pin_init::zeroed();
+
+// To ensure that we do not accidentally install pages into or zap pages from the wrong vma, we
+// check its vm_ops and private data before using it.
+fn check_vma(vma: &virt::VmaRef, owner: *const ShrinkablePageRange) -> Option<&virt::VmaMixedMap> {
+    // SAFETY: Just reading the vm_ops pointer of any active vma is safe.
+    let vm_ops = unsafe { (*vma.as_ptr()).vm_ops };
+    if !ptr::eq(vm_ops, &BINDER_VM_OPS) {
+        return None;
+    }
+
+    // SAFETY: Reading the vm_private_data pointer of a binder-owned vma is safe.
+    let vm_private_data = unsafe { (*vma.as_ptr()).vm_private_data };
+    if !ptr::eq(vm_private_data, owner.cast()) {
+        return None;
+    }
+
+    vma.as_mixedmap_vma()
+}
+
 struct Inner {
     /// Array of pages.
     ///
@@ -308,6 +329,16 @@ pub(crate) fn register_with_vma(&self, vma: &virt::VmaNew) -> Result<usize> {
         inner.size = num_pages;
         inner.vma_addr = vma.start();
 
+        // This pointer is only used for comparison - it's not dereferenced.
+        //
+        // SAFETY: We own the vma, and we don't use any methods on VmaNew that rely on
+        // `vm_private_data`.
+        unsafe { (*vma.as_ptr()).vm_private_data = self as *const Self as *mut c_void };
+
+        // SAFETY: We own the vma, and we don't use any methods on VmaNew that rely on
+        // `vm_ops`.
+        unsafe { (*vma.as_ptr()).vm_ops = &BINDER_VM_OPS };
+
         Ok(num_pages)
     }
 
@@ -399,22 +430,24 @@ unsafe fn use_page_slow(&self, i: usize) -> Result<()> {
         //
         // Using `mmput_async` avoids this, because then the `mm` cleanup is instead queued to a
         // workqueue.
-        MmWithUser::into_mmput_async(self.mm.mmget_not_zero().ok_or(ESRCH)?)
-            .mmap_read_lock()
-            .vma_lookup(vma_addr)
-            .ok_or(ESRCH)?
-            .as_mixedmap_vma()
-            .ok_or(ESRCH)?
-            .vm_insert_page(user_page_addr, &new_page)
-            .inspect_err(|err| {
-                pr_warn!(
-                    "Failed to vm_insert_page({}): vma_addr:{} i:{} err:{:?}",
-                    user_page_addr,
-                    vma_addr,
-                    i,
-                    err
-                )
-            })?;
+        check_vma(
+            MmWithUser::into_mmput_async(self.mm.mmget_not_zero().ok_or(ESRCH)?)
+                .mmap_read_lock()
+                .vma_lookup(vma_addr)
+                .ok_or(ESRCH)?,
+            self,
+        )
+        .ok_or(ESRCH)?
+        .vm_insert_page(user_page_addr, &new_page)
+        .inspect_err(|err| {
+            pr_warn!(
+                "Failed to vm_insert_page({}): vma_addr:{} i:{} err:{:?}",
+                user_page_addr,
+                vma_addr,
+                i,
+                err
+            )
+        })?;
 
         let inner = self.lock.lock();
 
@@ -667,12 +700,15 @@ fn drop(self: Pin<&mut Self>) {
     let mmap_read;
     let mm_mutex;
     let vma_addr;
+    let range_ptr;
 
     {
         // CAST: The `list_head` field is first in `PageInfo`.
         let info = item as *mut PageInfo;
         // SAFETY: The `range` field of `PageInfo` is immutable.
-        let range = unsafe { &*((*info).range) };
+        range_ptr = unsafe { (*info).range };
+        // SAFETY: The `range` outlives its `PageInfo` values.
+        let range = unsafe { &*range_ptr };
 
         mm = match range.mm.mmget_not_zero() {
             Some(mm) => MmWithUser::into_mmput_async(mm),
@@ -717,9 +753,11 @@ fn drop(self: Pin<&mut Self>) {
     // SAFETY: The lru lock is locked when this method is called.
     unsafe { bindings::spin_unlock(&raw mut (*lru).lock) };
 
-    if let Some(vma) = mmap_read.vma_lookup(vma_addr) {
-        let user_page_addr = vma_addr + (page_index << PAGE_SHIFT);
-        vma.zap_page_range_single(user_page_addr, PAGE_SIZE);
+    if let Some(unchecked_vma) = mmap_read.vma_lookup(vma_addr) {
+        if let Some(vma) = check_vma(unchecked_vma, range_ptr) {
+            let user_page_addr = vma_addr + (page_index << PAGE_SHIFT);
+            vma.zap_page_range_single(user_page_addr, PAGE_SIZE);
+        }
     }
 
     drop(mmap_read);

-- 
2.53.0.273.g2a3d683680-goog

[PATCH 2/2] rust_binder: avoid reading the written value in offsets array

[Alice Ryhl]

When sending a transaction, its offsets array is first copied into the
target proc's vma, and then the values are read back from there. This is
normally fine because the vma is a read-only mapping, so the target
process cannot change the value under us.

However, if the target process somehow gains the ability to write to its
own vma, it could change the offset before it's read back, causing the
kernel to misinterpret what the sender meant. If the sender happens to
send a payload with a specific shape, this could in the worst case lead
to the receiver being able to privilege escalate into the sender.

The intent is that gaining the ability to change the read-only vma of
your own process should not be exploitable, so remove this TOCTOU read
even though it's unexploitable without another Binder bug.

Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/thread.rs | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
index 1f1709a6a77abc1c865cc9387e7ba7493448c71d..f58ecccf5bb10a4b916d14a38dbb3bdfdda24ff8 100644
--- a/drivers/android/binder/thread.rs
+++ b/drivers/android/binder/thread.rs
@@ -1016,12 +1016,9 @@ pub(crate) fn copy_transaction_data(
 
         // Copy offsets if there are any.
         if offsets_size > 0 {
-            {
-                let mut reader =
-                    UserSlice::new(UserPtr::from_addr(trd_data_ptr.offsets as _), offsets_size)
-                        .reader();
-                alloc.copy_into(&mut reader, aligned_data_size, offsets_size)?;
-            }
+            let mut offsets_reader =
+                UserSlice::new(UserPtr::from_addr(trd_data_ptr.offsets as _), offsets_size)
+                .reader();
 
             let offsets_start = aligned_data_size;
             let offsets_end = aligned_data_size + offsets_size;
@@ -1042,11 +1039,9 @@ pub(crate) fn copy_transaction_data(
                 .step_by(size_of::<u64>())
                 .enumerate()
             {
-                let offset: usize = view
-                    .alloc
-                    .read::<u64>(index_offset)?
-                    .try_into()
-                    .map_err(|_| EINVAL)?;
+                let offset = offsets_reader.read::<u64>()?;
+                view.alloc.write(index_offset, &offset)?;
+                let offset: usize = offset.try_into().map_err(|_| EINVAL)?;
 
                 if offset < end_of_previous_object || !is_aligned(offset, size_of::<u32>()) {
                     pr_warn!("Got transaction with invalid offset.");

-- 
2.53.0.273.g2a3d683680-goog

Re: [PATCH 1/2] rust_binder: check ownership before using vma

[Danilo Krummrich]

On Tue Feb 17, 2026 at 3:22 PM CET, Alice Ryhl wrote:
> When installing missing pages (or zapping them), Rust Binder will look
> up the vma in the mm by address, and then call vm_insert_page (or
> zap_page_range_single). However, if the vma is closed and replaced with
> a different vma at the same address, this can lead to Rust Binder
> installing pages into the wrong vma.
>
> By installing the page into a writable vma, it becomes possible to write
> to your own binder pages, which are normally read-only. Although you're
> not supposed to be able to write to those pages, the intent behind the
> design of Rust Binder is that even if you get that ability, it should not
> lead to anything bad. Unfortunately, due to another bug, that is not the
> case.
>
> To fix this, I will store a pointer in vm_private_data and check that
> the vma returned by vma_lookup() has the right vm_ops and
> vm_private_data before trying to use the vma. This should ensure that
> Rust Binder will refuse to interact with any other VMA. I will follow up
> this patch with more vma abstractions to avoid this unsafe access to
> vm_ops and vm_private_data, but for now I'd like to start with the
> simplest possible fix.

I suggest to use imperative mood instead.

> C Binder performs the same check in a slightly different way: it
> provides a vm_ops->close that sets a boolean to true, then checks that
> boolean after calling vma_lookup(), but I think this is more fragile
> than the solution in this patch. (We probably still want to do both, but
> I'll add the vm_ops->close callback with the follow-up vma API changes.)
>
> Cc: stable@vger.kernel.org
> Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> Reported-by: Jann Horn <jannh@google.com>

If you have a link, please add Closes: after Reported-by:.

> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  drivers/android/binder/page_range.rs | 78 +++++++++++++++++++++++++++---------
>  1 file changed, 58 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
> index fdd97112ef5c8b2341e498dc3567b659f05e3fd7..90bab18961443c6e59699cb7345e41e0db80f0dd 100644
> --- a/drivers/android/binder/page_range.rs
> +++ b/drivers/android/binder/page_range.rs
> @@ -142,6 +142,27 @@ pub(crate) struct ShrinkablePageRange {
>      _pin: PhantomPinned,
>  }
>  
> +// We do not define any ops. For now, used only to check identity of vmas.
> +static BINDER_VM_OPS: bindings::vm_operations_struct = pin_init::zeroed();
> +
> +// To ensure that we do not accidentally install pages into or zap pages from the wrong vma, we
> +// check its vm_ops and private data before using it.
> +fn check_vma(vma: &virt::VmaRef, owner: *const ShrinkablePageRange) -> Option<&virt::VmaMixedMap> {
> +    // SAFETY: Just reading the vm_ops pointer of any active vma is safe.

Here and in a few other places, missing markdown.

> +    let vm_ops = unsafe { (*vma.as_ptr()).vm_ops };
> +    if !ptr::eq(vm_ops, &BINDER_VM_OPS) {
> +        return None;
> +    }
> +
> +    // SAFETY: Reading the vm_private_data pointer of a binder-owned vma is safe.
> +    let vm_private_data = unsafe { (*vma.as_ptr()).vm_private_data };
> +    if !ptr::eq(vm_private_data, owner.cast()) {
> +        return None;
> +    }
> +
> +    vma.as_mixedmap_vma()
> +}
> +
>  struct Inner {
>      /// Array of pages.
>      ///
> @@ -308,6 +329,16 @@ pub(crate) fn register_with_vma(&self, vma: &virt::VmaNew) -> Result<usize> {
>          inner.size = num_pages;
>          inner.vma_addr = vma.start();
>  
> +        // This pointer is only used for comparison - it's not dereferenced.
> +        //
> +        // SAFETY: We own the vma, and we don't use any methods on VmaNew that rely on
> +        // `vm_private_data`.
> +        unsafe { (*vma.as_ptr()).vm_private_data = self as *const Self as *mut c_void };

Maybe use from_ref(self).cast_mut().cast::<c_void>() instead?

Please don't consider any of those NITs a blocker. :)

Re: [PATCH 2/2] rust_binder: avoid reading the written value in offsets array

[Jann Horn]

On Tue, Feb 17, 2026 at 3:22 PM Alice Ryhl <aliceryhl@google.com> wrote:
> When sending a transaction, its offsets array is first copied into the
> target proc's vma, and then the values are read back from there. This is
> normally fine because the vma is a read-only mapping, so the target
> process cannot change the value under us.
>
> However, if the target process somehow gains the ability to write to its
> own vma, it could change the offset before it's read back, causing the
> kernel to misinterpret what the sender meant. If the sender happens to
> send a payload with a specific shape, this could in the worst case lead
> to the receiver being able to privilege escalate into the sender.
>
> The intent is that gaining the ability to change the read-only vma of
> your own process should not be exploitable, so remove this TOCTOU read
> even though it's unexploitable without another Binder bug.

With this, the only remaining read from the ShrinkablePageRange is in
AllocationView::cleanup_object(), correct? If I understand correctly,
that is fine because it can only drop references on handles (which
userspace could equivalently do via BC_RELEASE/BC_DECREFS) and on
binders (which would probably also have its influence limited to the
process)?

> Cc: stable@vger.kernel.org
> Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Jann Horn <jannh@google.com>

Re: [PATCH 1/2] rust_binder: check ownership before using vma

[Jann Horn]

On Tue, Feb 17, 2026 at 3:22 PM Alice Ryhl <aliceryhl@google.com> wrote:
> When installing missing pages (or zapping them), Rust Binder will look
> up the vma in the mm by address, and then call vm_insert_page (or
> zap_page_range_single). However, if the vma is closed and replaced with
> a different vma at the same address, this can lead to Rust Binder
> installing pages into the wrong vma.
>
> By installing the page into a writable vma, it becomes possible to write
> to your own binder pages, which are normally read-only. Although you're
> not supposed to be able to write to those pages, the intent behind the
> design of Rust Binder is that even if you get that ability, it should not
> lead to anything bad. Unfortunately, due to another bug, that is not the
> case.
>
> To fix this, I will store a pointer in vm_private_data and check that
> the vma returned by vma_lookup() has the right vm_ops and
> vm_private_data before trying to use the vma. This should ensure that
> Rust Binder will refuse to interact with any other VMA. I will follow up
> this patch with more vma abstractions to avoid this unsafe access to
> vm_ops and vm_private_data, but for now I'd like to start with the
> simplest possible fix.

This sounds good to me.
(Userspace could still trick Rust Binder into accessing the VMA at the
wrong offset, but nothing will go wrong in that case.)

> C Binder performs the same check in a slightly different way: it
> provides a vm_ops->close that sets a boolean to true, then checks that
> boolean after calling vma_lookup(), but I think this is more fragile
> than the solution in this patch. (We probably still want to do both, but
> I'll add the vm_ops->close callback with the follow-up vma API changes.)
>
> Cc: stable@vger.kernel.org
> Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Jann Horn <jannh@google.com>

> ---
>  drivers/android/binder/page_range.rs | 78 +++++++++++++++++++++++++++---------
>  1 file changed, 58 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
> index fdd97112ef5c8b2341e498dc3567b659f05e3fd7..90bab18961443c6e59699cb7345e41e0db80f0dd 100644
> --- a/drivers/android/binder/page_range.rs
> +++ b/drivers/android/binder/page_range.rs
> @@ -142,6 +142,27 @@ pub(crate) struct ShrinkablePageRange {
>      _pin: PhantomPinned,
>  }
>
> +// We do not define any ops. For now, used only to check identity of vmas.
> +static BINDER_VM_OPS: bindings::vm_operations_struct = pin_init::zeroed();
> +
> +// To ensure that we do not accidentally install pages into or zap pages from the wrong vma, we
> +// check its vm_ops and private data before using it.
> +fn check_vma(vma: &virt::VmaRef, owner: *const ShrinkablePageRange) -> Option<&virt::VmaMixedMap> {
> +    // SAFETY: Just reading the vm_ops pointer of any active vma is safe.
> +    let vm_ops = unsafe { (*vma.as_ptr()).vm_ops };
> +    if !ptr::eq(vm_ops, &BINDER_VM_OPS) {
> +        return None;
> +    }
> +
> +    // SAFETY: Reading the vm_private_data pointer of a binder-owned vma is safe.
> +    let vm_private_data = unsafe { (*vma.as_ptr()).vm_private_data };
> +    if !ptr::eq(vm_private_data, owner.cast()) {
> +        return None;
> +    }

(And the ShrinkablePageRange is only dropped when the Process is
dropped, which only happens once the file's ->release handler is
invoked, which means the ShrinkablePageRange outlives any VMA
associated with it, so there can't be any false positives due to
pointer reuse here.)

Re: [PATCH 2/2] rust_binder: avoid reading the written value in offsets array

[Alice Ryhl]

On Tue, Feb 17, 2026 at 5:35 PM Jann Horn <jannh@google.com> wrote:
>
> On Tue, Feb 17, 2026 at 3:22 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > When sending a transaction, its offsets array is first copied into the
> > target proc's vma, and then the values are read back from there. This is
> > normally fine because the vma is a read-only mapping, so the target
> > process cannot change the value under us.
> >
> > However, if the target process somehow gains the ability to write to its
> > own vma, it could change the offset before it's read back, causing the
> > kernel to misinterpret what the sender meant. If the sender happens to
> > send a payload with a specific shape, this could in the worst case lead
> > to the receiver being able to privilege escalate into the sender.
> >
> > The intent is that gaining the ability to change the read-only vma of
> > your own process should not be exploitable, so remove this TOCTOU read
> > even though it's unexploitable without another Binder bug.
>
> With this, the only remaining read from the ShrinkablePageRange is in
> AllocationView::cleanup_object(), correct? If I understand correctly,
> that is fine because it can only drop references on handles (which
> userspace could equivalently do via BC_RELEASE/BC_DECREFS) and on
> binders (which would probably also have its influence limited to the
> process)?

Yeah, that's the idea.

Alice

Re: [PATCH 1/2] rust_binder: check ownership before using vma

[Alice Ryhl]

On Tue, Feb 17, 2026 at 4:13 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue Feb 17, 2026 at 3:22 PM CET, Alice Ryhl wrote:
> > When installing missing pages (or zapping them), Rust Binder will look
> > up the vma in the mm by address, and then call vm_insert_page (or
> > zap_page_range_single). However, if the vma is closed and replaced with
> > a different vma at the same address, this can lead to Rust Binder
> > installing pages into the wrong vma.
> >
> > By installing the page into a writable vma, it becomes possible to write
> > to your own binder pages, which are normally read-only. Although you're
> > not supposed to be able to write to those pages, the intent behind the
> > design of Rust Binder is that even if you get that ability, it should not
> > lead to anything bad. Unfortunately, due to another bug, that is not the
> > case.
> >
> > To fix this, I will store a pointer in vm_private_data and check that
> > the vma returned by vma_lookup() has the right vm_ops and
> > vm_private_data before trying to use the vma. This should ensure that
> > Rust Binder will refuse to interact with any other VMA. I will follow up
> > this patch with more vma abstractions to avoid this unsafe access to
> > vm_ops and vm_private_data, but for now I'd like to start with the
> > simplest possible fix.
>
> I suggest to use imperative mood instead.

How do you propose to reword "I will follow up this patch with"?

> > C Binder performs the same check in a slightly different way: it
> > provides a vm_ops->close that sets a boolean to true, then checks that
> > boolean after calling vma_lookup(), but I think this is more fragile
> > than the solution in this patch. (We probably still want to do both, but
> > I'll add the vm_ops->close callback with the follow-up vma API changes.)
> >
> > Cc: stable@vger.kernel.org
> > Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> > Reported-by: Jann Horn <jannh@google.com>
>
> If you have a link, please add Closes: after Reported-by:.

There is no publicly accessible link.

> > +    let vm_ops = unsafe { (*vma.as_ptr()).vm_ops };
> > +    if !ptr::eq(vm_ops, &BINDER_VM_OPS) {
> > +        return None;
> > +    }
> > +
> > +    // SAFETY: Reading the vm_private_data pointer of a binder-owned vma is safe.
> > +    let vm_private_data = unsafe { (*vma.as_ptr()).vm_private_data };
> > +    if !ptr::eq(vm_private_data, owner.cast()) {
> > +        return None;
> > +    }
> > +
> > +    vma.as_mixedmap_vma()
> > +}
> > +
> >  struct Inner {
> >      /// Array of pages.
> >      ///
> > @@ -308,6 +329,16 @@ pub(crate) fn register_with_vma(&self, vma: &virt::VmaNew) -> Result<usize> {
> >          inner.size = num_pages;
> >          inner.vma_addr = vma.start();
> >
> > +        // This pointer is only used for comparison - it's not dereferenced.
> > +        //
> > +        // SAFETY: We own the vma, and we don't use any methods on VmaNew that rely on
> > +        // `vm_private_data`.
> > +        unsafe { (*vma.as_ptr()).vm_private_data = self as *const Self as *mut c_void };
>
> Maybe use from_ref(self).cast_mut().cast::<c_void>() instead?

Honestly I think this one is easier to read as-is.

Alice

Re: [PATCH 1/2] rust_binder: check ownership before using vma

[Alice Ryhl]

On Tue, Feb 17, 2026 at 5:55 PM Jann Horn <jannh@google.com> wrote:
>
> On Tue, Feb 17, 2026 at 3:22 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > When installing missing pages (or zapping them), Rust Binder will look
> > up the vma in the mm by address, and then call vm_insert_page (or
> > zap_page_range_single). However, if the vma is closed and replaced with
> > a different vma at the same address, this can lead to Rust Binder
> > installing pages into the wrong vma.
> >
> > By installing the page into a writable vma, it becomes possible to write
> > to your own binder pages, which are normally read-only. Although you're
> > not supposed to be able to write to those pages, the intent behind the
> > design of Rust Binder is that even if you get that ability, it should not
> > lead to anything bad. Unfortunately, due to another bug, that is not the
> > case.
> >
> > To fix this, I will store a pointer in vm_private_data and check that
> > the vma returned by vma_lookup() has the right vm_ops and
> > vm_private_data before trying to use the vma. This should ensure that
> > Rust Binder will refuse to interact with any other VMA. I will follow up
> > this patch with more vma abstractions to avoid this unsafe access to
> > vm_ops and vm_private_data, but for now I'd like to start with the
> > simplest possible fix.
>
> This sounds good to me.
> (Userspace could still trick Rust Binder into accessing the VMA at the
> wrong offset, but nothing will go wrong in that case.)

Vma is tricky stuff.

I think if I add the vm_ops->close callback this one isn't possible anymore?

> > C Binder performs the same check in a slightly different way: it
> > provides a vm_ops->close that sets a boolean to true, then checks that
> > boolean after calling vma_lookup(), but I think this is more fragile
> > than the solution in this patch. (We probably still want to do both, but
> > I'll add the vm_ops->close callback with the follow-up vma API changes.)
> >
> > Cc: stable@vger.kernel.org
> > Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> > Reported-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> Reviewed-by: Jann Horn <jannh@google.com>
>
> > ---
> >  drivers/android/binder/page_range.rs | 78 +++++++++++++++++++++++++++---------
> >  1 file changed, 58 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
> > index fdd97112ef5c8b2341e498dc3567b659f05e3fd7..90bab18961443c6e59699cb7345e41e0db80f0dd 100644
> > --- a/drivers/android/binder/page_range.rs
> > +++ b/drivers/android/binder/page_range.rs
> > @@ -142,6 +142,27 @@ pub(crate) struct ShrinkablePageRange {
> >      _pin: PhantomPinned,
> >  }
> >
> > +// We do not define any ops. For now, used only to check identity of vmas.
> > +static BINDER_VM_OPS: bindings::vm_operations_struct = pin_init::zeroed();
> > +
> > +// To ensure that we do not accidentally install pages into or zap pages from the wrong vma, we
> > +// check its vm_ops and private data before using it.
> > +fn check_vma(vma: &virt::VmaRef, owner: *const ShrinkablePageRange) -> Option<&virt::VmaMixedMap> {
> > +    // SAFETY: Just reading the vm_ops pointer of any active vma is safe.
> > +    let vm_ops = unsafe { (*vma.as_ptr()).vm_ops };
> > +    if !ptr::eq(vm_ops, &BINDER_VM_OPS) {
> > +        return None;
> > +    }
> > +
> > +    // SAFETY: Reading the vm_private_data pointer of a binder-owned vma is safe.
> > +    let vm_private_data = unsafe { (*vma.as_ptr()).vm_private_data };
> > +    if !ptr::eq(vm_private_data, owner.cast()) {
> > +        return None;
> > +    }
>
> (And the ShrinkablePageRange is only dropped when the Process is
> dropped, which only happens once the file's ->release handler is
> invoked, which means the ShrinkablePageRange outlives any VMA
> associated with it, so there can't be any false positives due to
> pointer reuse here.)

Yeah.

Alice

Re: [PATCH 1/2] rust_binder: check ownership before using vma

[Miguel Ojeda]

On Tue, Feb 17, 2026 at 9:13 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> How do you propose to reword "I will follow up this patch with"?

What I would have written is perhaps something like:

  "The plan is to introduce more vma abstractions to ..., but for now
let's start with ..."

> Honestly I think this one is easier to read as-is.

Cc'ing Tamir for that -- he may be interested.

Cheers,
Miguel

Re: [PATCH 1/2] rust_binder: check ownership before using vma

[Jann Horn]

On Tue, Feb 17, 2026 at 9:15 PM Alice Ryhl <aliceryhl@google.com> wrote:
> On Tue, Feb 17, 2026 at 5:55 PM Jann Horn <jannh@google.com> wrote:
> > On Tue, Feb 17, 2026 at 3:22 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > > When installing missing pages (or zapping them), Rust Binder will look
> > > up the vma in the mm by address, and then call vm_insert_page (or
> > > zap_page_range_single). However, if the vma is closed and replaced with
> > > a different vma at the same address, this can lead to Rust Binder
> > > installing pages into the wrong vma.
> > >
> > > By installing the page into a writable vma, it becomes possible to write
> > > to your own binder pages, which are normally read-only. Although you're
> > > not supposed to be able to write to those pages, the intent behind the
> > > design of Rust Binder is that even if you get that ability, it should not
> > > lead to anything bad. Unfortunately, due to another bug, that is not the
> > > case.
> > >
> > > To fix this, I will store a pointer in vm_private_data and check that
> > > the vma returned by vma_lookup() has the right vm_ops and
> > > vm_private_data before trying to use the vma. This should ensure that
> > > Rust Binder will refuse to interact with any other VMA. I will follow up
> > > this patch with more vma abstractions to avoid this unsafe access to
> > > vm_ops and vm_private_data, but for now I'd like to start with the
> > > simplest possible fix.
> >
> > This sounds good to me.
> > (Userspace could still trick Rust Binder into accessing the VMA at the
> > wrong offset, but nothing will go wrong in that case.)
>
> Vma is tricky stuff.

Well, they try to give userspace a lot of flexibility, and then things
like the rmap are supposed to abstract away this complexity so that
normal drivers don't have to deal with this complexity...

> I think if I add the vm_ops->close callback this one isn't possible anymore?

Yeah. (Or you could explicitly check that vma_pgoff_offset(vma,
virtual_address) returns the expected index. But either way, from a
security perspective it shouldn't really matter.)

Re: [PATCH 1/2] rust_binder: check ownership before using vma

[Danilo Krummrich]

On Tue Feb 17, 2026 at 9:12 PM CET, Alice Ryhl wrote:
> On Tue, Feb 17, 2026 at 4:13 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Tue Feb 17, 2026 at 3:22 PM CET, Alice Ryhl wrote:
>> > When installing missing pages (or zapping them), Rust Binder will look
>> > up the vma in the mm by address, and then call vm_insert_page (or
>> > zap_page_range_single). However, if the vma is closed and replaced with
>> > a different vma at the same address, this can lead to Rust Binder
>> > installing pages into the wrong vma.
>> >
>> > By installing the page into a writable vma, it becomes possible to write
>> > to your own binder pages, which are normally read-only. Although you're
>> > not supposed to be able to write to those pages, the intent behind the
>> > design of Rust Binder is that even if you get that ability, it should not
>> > lead to anything bad. Unfortunately, due to another bug, that is not the
>> > case.
>> >
>> > To fix this, I will store a pointer in vm_private_data and check that
>> > the vma returned by vma_lookup() has the right vm_ops and
>> > vm_private_data before trying to use the vma. This should ensure that
>> > Rust Binder will refuse to interact with any other VMA. I will follow up
>> > this patch with more vma abstractions to avoid this unsafe access to
>> > vm_ops and vm_private_data, but for now I'd like to start with the
>> > simplest possible fix.
>>
>> I suggest to use imperative mood instead.
>
> How do you propose to reword "I will follow up this patch with"?

To fix this, store a pointer in vm_private_data and check [...]. Subsequent work
will follow-up this patch with [...], but for now start with the simplest
possible fix.

>> > +        // This pointer is only used for comparison - it's not dereferenced.
>> > +        //
>> > +        // SAFETY: We own the vma, and we don't use any methods on VmaNew that rely on
>> > +        // `vm_private_data`.
>> > +        unsafe { (*vma.as_ptr()).vm_private_data = self as *const Self as *mut c_void };
>>
>> Maybe use from_ref(self).cast_mut().cast::<c_void>() instead?
>
> Honestly I think this one is easier to read as-is.

I remember this series: https://lore.kernel.org/all/20250615-ptr-as-ptr-v12-0-f43b024581e8@gmail.com/

It talks about enabling clippy::ref_as_ptr and I think we have it enabled, does
this not apply here?

Re: [PATCH 2/2] rust_binder: avoid reading the written value in offsets array

[kernel test robot]

Hi Alice,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0f2acd3148e0ef42bdacbd477f90e8533f96b2ac]

url:    https://github.com/intel-lab-lkp/linux/commits/Alice-Ryhl/rust_binder-check-ownership-before-using-vma/20260217-222439
base:   0f2acd3148e0ef42bdacbd477f90e8533f96b2ac
patch link:    https://lore.kernel.org/r/20260217-binder-vma-check-v1-2-1a2b37f7b762%40google.com
patch subject: [PATCH 2/2] rust_binder: avoid reading the written value in offsets array
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20260217/202602172222.mGDpJK77-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260217/202602172222.mGDpJK77-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602172222.mGDpJK77-lkp@intel.com/

All errors (new ones prefixed by >>):

   PATH=/opt/cross/clang-20/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   INFO PATH=/opt/cross/rustc-1.88.0-bindgen-0.72.1/cargo/bin:/opt/cross/clang-20/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   /usr/bin/timeout -k 100 12h /usr/bin/make KCFLAGS=\ -fno-crash-diagnostics\ -Wno-error=return-type\ -Wreturn-type\ -funsigned-char\ -Wundef\ -falign-functions=64 W=1 --keep-going LLVM=1 -j32 -C source O=/kbuild/obj/consumer/x86_64-rhel-9.4-rust ARCH=x86_64 SHELL=/bin/bash rustfmtcheck 
   make: Entering directory '/kbuild/src/consumer'
   make[1]: Entering directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'
>> Diff in drivers/android/binder/thread.rs:1018:
            if offsets_size > 0 {
                let mut offsets_reader =
                    UserSlice::new(UserPtr::from_addr(trd_data_ptr.offsets as _), offsets_size)
   -                .reader();
   +                    .reader();
    
                let offsets_start = aligned_data_size;
                let offsets_end = aligned_data_size + offsets_size;
>> Diff in drivers/android/binder/thread.rs:1018:
            if offsets_size > 0 {
                let mut offsets_reader =
                    UserSlice::new(UserPtr::from_addr(trd_data_ptr.offsets as _), offsets_size)
   -                .reader();
   +                    .reader();
    
                let offsets_start = aligned_data_size;
                let offsets_end = aligned_data_size + offsets_size;
   make[2]: *** [Makefile:1903: rustfmt] Error 123
   make[2]: Target 'rustfmtcheck' not remade because of errors.
   make[1]: Leaving directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'
   make[1]: *** [Makefile:248: __sub-make] Error 2
   make[1]: Target 'rustfmtcheck' not remade because of errors.
   make: *** [Makefile:248: __sub-make] Error 2
   make: Target 'rustfmtcheck' not remade because of errors.
   make: Leaving directory '/kbuild/src/consumer'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki