Re: [PATCH v4 5/7] binder: check vma->vm_start in binder_vma_close()

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Sep 03, 2026 at 10:12:55PM +0000, Carlos Llamas wrote:
> On Thu, Sep 03, 2026 at 07:57:47AM +0000, Alice Ryhl wrote:
> > On Wed, Sep 02, 2026 at 04:25:39PM +0000, Carlos Llamas wrote:
> > > On Wed, Sep 02, 2026 at 12:50:20PM +0000, Alice Ryhl wrote:
> > > > On Tue, Sep 01, 2026 at 08:52:45PM +0000, Carlos Llamas wrote:
> > > > > Certain operations like a failed mremap() might trigger vm_ops->close()
> > > > > on temporary mappings. To avoid tearing-down the main binder mapping on
> > > > > these, let's verify that the VMA matches the expected starting address.
> > > > > 
> > > > > Cc: stable@vger.kernel.org
> > > > > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > > > > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > > > Closes: https://sashiko.dev/#/patchset/20260831224145.169403-1-cmllamas@google.com?part=1
> > > > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > > > ---
> > > > >  drivers/android/binder.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > index 185128577829..3d359490436e 100644
> > > > > --- a/drivers/android/binder.c
> > > > > +++ b/drivers/android/binder.c
> > > > > @@ -6018,6 +6018,9 @@ static void binder_vma_close(struct vm_area_struct *vma)
> > > > >  {
> > > > >  	struct binder_proc *proc = vma->vm_private_data;
> > > > >  
> > > > > +	if (vma->vm_start != proc->alloc.vm_start)
> > > > > +		return;
> > > > 
> > > > So .. this does work in the case of mremap, but how about instead doing
> > > > this?
> > > > 
> > > > static int binder_mremap(struct vm_area_struct *vma)
> > > > {
> > > > 	vma->vm_private_data = NULL;
> > > > 	return -EINVAL;
> > > > }
> > > > 
> > > > and then check for NULL in binder_vma_close() instead? I think that
> > > > logic would be a bit easier to understand.
> > > 
> > > Yeah, I agree that is easier to read. However, not all exit paths that
> > > close a copied vma actually call op->mremap(). We would miss those and
> > > accidentally brick binder.
> > 
> > What about setting it to NULL in op->open(), then?
> 
> I suppose that would technically work because ->open() would only be
> called for subsequent operations after the initial ->mmap(). However,
> that might be more complex to understand without this "mm-specific"
> context no? A comment would again be needed to explain why we clear
> vma->vm_private_data for the common readers...
> 
> 	/*
> 	 * Subsequent ->open() calls after the initial ->mmap() are
> 	 * considered invalid ops and as such we mark ->vm_private_data
> 	 * invalid and avoid IPC tear-down upon its ->close().
> 	 */
> 	 vma->vm_private_data = NULL;
> 
> I don't hate this idea, but I don't see the easier-to-read argument
> either. I'll switch to this if you really think is better.
> 
> Ultimately, we are trying to find a way to identify the "original"
> mapping and avoid shutting down the IPC on invalid clones. Do you
> believe using vma->vm_start is not a reliable way? Or perhaps not
> straight-forward?

The reason I find vma->vm_start to be non-obvious is ... how do you know
that the second vma can't have the same value for vm_start?

Does mremap() work for splitting a vma into two? Then the first half of
the resulting two vmas will have the same vm_start. Or does it support
resizing it, which results in a new vma with the same vm_start? Or can
we create a vma of length zero at that address?

After some verification, I found that these do not apply because the vma
created by mremap() can't overlap with the old one. But it was not
obvious to me.

And in fact I do think we *can* create a new vma with the same vm_start
like this:

1. mremap() the original VMA to a second VMA at a different address
2. Close the original VMA
3. mremap() the second VMA to the original address, creating a third VMA
   at that location

and the third VMA would get past the vm_start check when you close it.

Or perhaps:

1. unmap the first half of the original vma
2. mremap() the remainder back to vm_start, which is no longer overlap

Now, in the current code that's actually harmless because we already set
mapped to false in this scenario ... will it be harmless in all future
versions of this code? Maybe not? Future authors may see the vm_start
check and conclude "after this check I know for sure this code runs only
once" and do something that's illegal if called twice.

Alice