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

[Carlos Llamas]

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@xxxxxxxxxxxxxxx
> > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Closes: https://sashiko.dev/#/patchset/20260831224145.169403-1-cmllamas@xxxxxxxxxx?part=1
> > Signed-off-by: Carlos Llamas <cmllamas@xxxxxxxxxx>
> > ---
> >  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.

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?

--
Carlos Llamas