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

[Alice Ryhl <aliceryhl@google.com>]

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