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

[Carlos Llamas <cmllamas@google.com>]

On Mon, Mar 02, 2026 at 06:28:17PM +0100, Jann Horn wrote:
> On Mon, Mar 2, 2026 at 6:18 PM Carlos Llamas <cmllamas@google.com> wrote:
> > On Wed, Feb 18, 2026 at 11:53:26AM +0000, 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.
> >
> > This all makes sense to me. What I'm missing though is why not reject
> > VM_WRITE mappings all together? Is there a downside or something that
> > prevents us from setting this check?
> 
> You could, and it would probably do the job (assuming that you check
> for VM_MAYWRITE instead of VM_WRITE), but I think it'd be more of a
> surface-level mitigation than a robust safety check - in my opinion, a
> robust check should, at a minimum, confirm that the VMA being accessed
> belongs to the right driver, because other drivers might do random
> things you don't expect in their own VMAs. (For example, it wouldn't
> protect against interaction with a driver like C binder which reads
> PTEs back out of the VMA in binder_page_lookup(), makes assumptions
> about what kinds of pages that yields, and writes into those pages.) A
> driver should not be touching VMAs it doesn't own.

Alice just informed me the VM_MAYWRITE check is already in-place:

	rust_binder_mmap() ->
		Process::mmap() ->
			try_clear_maywrite()

I just wasn't aware of this, sorry for the noise.

I also agree with your comments. The following two safety checks should
be addressed by binder:

  1. mappings should be read-only.
  2. don't operate on unrelated VMAs.

(1) Was already covered and (2) is addressed by this patchset. So we are
all good here. Thanks!

--
Carlos Llamas