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

[Alice Ryhl <aliceryhl@google.com>]

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?

Alice

> Maybe I should add a comment to the check, so that is easier to read?
> 
> 	/* 
> 	 * Ignore temporary vma copies from aborted operations (e.g.
> 	 * mremap). Only tear-down the original VMA with the expected
> 	 * starting address.
> 	 */
> 	 if (vma->vm_start != proc->alloc.vm_start)
> 	 	return;
> 
> Would that work?