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

[Carlos Llamas]

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@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.
> 
> 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?

--
Carlos Llamas