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

[Alice Ryhl <aliceryhl@google.com>]

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