[PATCH v4 0/7] binder: fix issues with mremap()

[PATCH v4 0/7] binder: fix issues with mremap()

[Carlos Llamas]

This is a follow up series fixing some pre-existing issues found by
sashiko during a review of an unrelated patchset here:
https://sashiko.dev/#/patchset/20260813193433.3318288-1-surenb@google.com

The goal is to safely reject mremap() requests on binder's vma to
prevent pages from temporary leaking and accidental IPC tear-down.

Cc: Todd Kjos <tkjos@android.com>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Suren Baghdasaryan <surenb@google.com> 

--
v4:
 - Also reject vma splitting. This closes loopholes wrt to partial
   range munmap() that Sashiko pointed out.
 - With splitting denied, we can go back to the simpler ->vm_start
   check in op->close().

v3:
 - Switch to a range-check in vm_ops->close() and protect against
   partial munmap() as Sashiko points out.
https://lore.kernel.org/all/20260901163521.1355535-1-cmllamas@google.com/

v2:
 - Set VM_DONTEXPAND in [1/5] and [2/5] per Sashiko's feedback.
 - Check vma->vm_start to prevent incorrect ->close() in [3/5] per
   Sashiko's feedback.
https://lore.kernel.org/all/20260901030419.470246-1-cmllamas@google.com/

v1:
https://lore.kernel.org/all/20260831224145.169403-1-cmllamas@google.com/

Carlos Llamas (7):
  binder: set VM_DONTEXPAND
  rust_binder: set VM_DONTEXPAND
  binder: forbid vma splitting
  rust_binder: forbid vma splitting
  binder: check vma->vm_start in binder_vma_close()
  binder: reject mremap()
  rust_binder: reject mremap()

 drivers/android/binder.c             | 18 +++++++++++++++++-
 drivers/android/binder/page_range.rs | 20 +++++++++++++++++---
 drivers/android/binder/process.rs    |  1 +
 3 files changed, 35 insertions(+), 4 deletions(-)

-- 
2.55.0.966.g6673acef38-goog

[PATCH v4 1/7] binder: set VM_DONTEXPAND

[Carlos Llamas]

Binder does not support expanding VMAs. It caches the original size
under alloc->buffer_size during mmap() and expanding the VMA would only
result in a wasted virtual range. Set the VM_DONTEXPAND flag to prevent
the vma from being expanded via mremap().

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, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 8f2ef1bd539f..1b492ed48ea6 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6056,7 +6056,8 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		       proc->pid, vma->vm_start, vma->vm_end, "bad vm_flags", -EPERM);
 		return -EPERM;
 	}
-	vm_flags_mod(vma, VM_DONTCOPY | VM_MIXEDMAP, VM_MAYWRITE);
+	vm_flags_mod(vma, VM_DONTCOPY | VM_MIXEDMAP | VM_DONTEXPAND,
+		     VM_MAYWRITE);
 
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
-- 
2.55.0.966.g6673acef38-goog

[PATCH v4 2/7] rust_binder: set VM_DONTEXPAND

[Carlos Llamas]

Binder does not support expanding VMAs. It uses the original size to
create its RangeAllocator during mmap() and expanding the VMA would only
result in a wasted virtual range. Set the VM_DONTEXPAND flag to prevent
the vma from being expanded via mremap().

Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust 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/process.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index 5372bfbd93b3..9038f3d13cfd 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -1786,6 +1786,7 @@ pub(crate) fn mmap(
         vma.try_clear_maywrite().map_err(|_| EPERM)?;
         vma.set_dontcopy();
         vma.set_mixedmap();
+        vma.set_dontexpand();
 
         // TODO: Set ops. We need to learn when the user unmaps so that we can stop using it.
         this.create_mapping(vma)
-- 
2.55.0.966.g6673acef38-goog

[PATCH v4 3/7] binder: forbid vma splitting

[Carlos Llamas]

Binder does not support splitting its mappings. Allowing so, leads to
potential attacks that stem from a partial munmap(), such as closing the
tail range and replacing it with a new mapping.

Close the loophole by explicitly rejecting vma splitting.

Cc: stable@vger.kernel.org
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260901163521.1355535-1-cmllamas@google.com?part=2
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 1b492ed48ea6..185128577829 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6032,10 +6032,16 @@ VISIBLE_IF_KUNIT vm_fault_t binder_vm_fault(struct vm_fault *vmf)
 }
 EXPORT_SYMBOL_IF_KUNIT(binder_vm_fault);
 
+static int binder_vma_may_split(struct vm_area_struct *vma, unsigned long addr)
+{
+	return -EINVAL;
+}
+
 static const struct vm_operations_struct binder_vm_ops = {
 	.open = binder_vma_open,
 	.close = binder_vma_close,
 	.fault = binder_vm_fault,
+	.may_split = binder_vma_may_split,
 };
 
 static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
-- 
2.55.0.966.g6673acef38-goog

[PATCH v4 4/7] rust_binder: forbid vma splitting

[Carlos Llamas]

Binder does not support splitting its mappings. Allowing so, leads to
potential attacks that stem from a partial munmap(), such as closing the
tail range and replacing it with a new mapping.

Close the loophole by explicitly rejecting vma splitting.

Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260901163521.1355535-1-cmllamas@google.com?part=2
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder/page_range.rs | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
index 52ffbf3504e7..d260b009c184 100644
--- a/drivers/android/binder/page_range.rs
+++ b/drivers/android/binder/page_range.rs
@@ -24,7 +24,7 @@
 use kernel::{
     bindings,
     error::Result,
-    ffi::{c_ulong, c_void},
+    ffi::{c_int, c_ulong, c_void},
     mm::{virt, Mm, MmWithUser},
     new_mutex, new_spinlock,
     page::{Page, PAGE_SHIFT, PAGE_SIZE},
@@ -144,8 +144,17 @@ pub(crate) struct ShrinkablePageRange {
     _pin: PhantomPinned,
 }
 
-// We do not define any ops. For now, used only to check identity of vmas.
-static BINDER_VM_OPS: AssertSync<bindings::vm_operations_struct> = AssertSync(pin_init::zeroed());
+unsafe extern "C" fn binder_vma_may_split(_: *mut bindings::vm_area_struct, _: c_ulong) -> c_int {
+    EINVAL.to_errno()
+}
+
+static BINDER_VM_OPS: AssertSync<bindings::vm_operations_struct> = {
+    let ops = bindings::vm_operations_struct {
+        may_split: Some(binder_vma_may_split),
+        ..pin_init::zeroed()
+    };
+    AssertSync(ops)
+};
 
 // To ensure that we do not accidentally install pages into or zap pages from the wrong vma, we
 // check its vm_ops and private data before using it.
-- 
2.55.0.966.g6673acef38-goog

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

[Carlos Llamas]

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;
+
 	binder_debug(BINDER_DEBUG_OPEN_CLOSE,
 		     "%d close vm area %lx-%lx (%ld K) vma %lx pagep %lx\n",
 		     proc->pid, vma->vm_start, vma->vm_end,
-- 
2.55.0.966.g6673acef38-goog

[PATCH v4 6/7] binder: reject mremap()

[Carlos Llamas]

Binder does not support mremap() as it caches the mapping address in
alloc->vm_start. Moving the mapping breaks the IPC communication for the
process and can temporarily leak pages during a shrinker reclaim.

Fix this by explicitly rejecting the .mremap() operation.

Cc: stable@vger.kernel.org
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260813193433.3318288-1-surenb@google.com?part=2
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 3d359490436e..290ccfde624d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6040,11 +6040,17 @@ static int binder_vma_may_split(struct vm_area_struct *vma, unsigned long addr)
 	return -EINVAL;
 }
 
+static int binder_mremap(struct vm_area_struct *vma)
+{
+	return -EINVAL;
+}
+
 static const struct vm_operations_struct binder_vm_ops = {
 	.open = binder_vma_open,
 	.close = binder_vma_close,
 	.fault = binder_vm_fault,
 	.may_split = binder_vma_may_split,
+	.mremap = binder_mremap,
 };
 
 static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
-- 
2.55.0.966.g6673acef38-goog

[PATCH v4 7/7] rust_binder: reject mremap()

[Carlos Llamas]

Binder does not support mremap() as it caches the mapping address in
Inner::vma_addr. Moving the mapping breaks the IPC communication for the
process and can temporarily leak pages during a shrinker reclaim.

Fix this by explicitly rejecting the .mremap() operation.

Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260813193433.3318288-1-surenb@google.com?part=2
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder/page_range.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
index d260b009c184..c6247ba03313 100644
--- a/drivers/android/binder/page_range.rs
+++ b/drivers/android/binder/page_range.rs
@@ -148,9 +148,14 @@ pub(crate) struct ShrinkablePageRange {
     EINVAL.to_errno()
 }
 
+unsafe extern "C" fn binder_mremap(_: *mut bindings::vm_area_struct) -> c_int {
+    EINVAL.to_errno()
+}
+
 static BINDER_VM_OPS: AssertSync<bindings::vm_operations_struct> = {
     let ops = bindings::vm_operations_struct {
         may_split: Some(binder_vma_may_split),
+        mremap: Some(binder_mremap),
         ..pin_init::zeroed()
     };
     AssertSync(ops)
-- 
2.55.0.966.g6673acef38-goog

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

[Alice Ryhl]

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.

Alice

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

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

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

[Alice Ryhl]

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?

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

--
Carlos Llamas

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

[Alice Ryhl]

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