Skip to contentSkip to conversation

binder: fix issues with mremap()

PATCH In review Carlos Llamas 2 participants 13 messages Sep 1 – Sep 4

Subsystems (3)
lore
Share & tools
lore

Carlos Llamas

author maintainer
#0 lore raw

v4 cover binder: fix issues with mremap()

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/[email protected]

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 <[email protected]>
Cc Alice Ryhl <[email protected]>
Cc Suren Baghdasaryan <[email protected]>

--
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/[email protected]/

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/[email protected]/

v1:
https://lore.kernel.org/all/[email protected]/

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(-)
Signature · 1 line
2.55.0.966.g6673acef38-goog
author maintainer
#1 lore raw

PATCH v4 1/7 binder: set VM_DONTEXPAND

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().

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/android/binder.c +2 -1
index 8f2ef1bd539f..1b492ed48ea6 100644
@@ -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;
Signature · 1 line
2.55.0.966.g6673acef38-goog
author maintainer
#2 lore raw

PATCH v4 2/7 rust_binder: set VM_DONTEXPAND

In reply to 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().

Fixes eafedbc7c050 ("rust_binder: add Rust Binder driver")
---
 drivers/android/binder/process.rs | 1 +
 1 file changed, 1 insertion(+)
drivers/android/binder/process.rs +1 -0
index 5372bfbd93b3..9038f3d13cfd 100644
@@ -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)
Signature · 1 line
2.55.0.966.g6673acef38-goog
author maintainer
#3 lore raw

PATCH v4 3/7 binder: forbid vma splitting

In reply to 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.

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/android/binder.c +6 -0
index 1b492ed48ea6..185128577829 100644
@@ -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)
Signature · 1 line
2.55.0.966.g6673acef38-goog
author maintainer
#4 lore raw

PATCH v4 4/7 rust_binder: forbid vma splitting

In reply to 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.

Fixes eafedbc7c050 ("rust_binder: add Rust Binder driver")
---
 drivers/android/binder/page_range.rs | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/android/binder/page_range.rs +12 -3
index 52ffbf3504e7..d260b009c184 100644
@@ -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.
Signature · 1 line
2.55.0.966.g6673acef38-goog
author maintainer
#5 lore raw

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

In reply to 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.

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/android/binder.c +3 -0
index 185128577829..3d359490436e 100644
@@ -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,
Signature · 1 line
2.55.0.966.g6673acef38-goog
maintainer
#6 lore raw

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

Quoted 18 lines · 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.

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/android/binder.c +0 -0
index 185128577829..3d359490436e 100644

drivers/android/binder.c static void binder_vma_close(struct vm_area_struct *vma) · line 6022

Quoted 5 lines
drivers/android/binder.c +2 -0
@@ -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

author maintainer
#7 lore raw

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

Quoted 28 lines · Alice Ryhl
Quoted 18 lines · 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.

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/android/binder.c +0 -0
index 185128577829..3d359490436e 100644

drivers/android/binder.c static void binder_vma_close(struct vm_area_struct *vma) · line 6022

Quoted 5 lines
drivers/android/binder.c +2 -0
@@ -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?

Quoted 8 lines

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

maintainer
#8 lore raw

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

Quoted 35 lines · Carlos Llamas
Quoted 34 lines · Alice Ryhl
Quoted 18 lines · 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.

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/android/binder.c +0 -0
index 185128577829..3d359490436e 100644

drivers/android/binder.c static void binder_vma_close(struct vm_area_struct *vma) · line 6022

Quoted 5 lines
drivers/android/binder.c +2 -0
@@ -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;
}
Quoted 6 lines
Quoted 2 lines

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

Quoted 11 lines

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?

author maintainer
#9 lore raw

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

Quoted 36 lines · Alice Ryhl
Quoted 35 lines · Carlos Llamas
Quoted 34 lines · Alice Ryhl
Quoted 18 lines · 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.

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/android/binder.c +0 -0
index 185128577829..3d359490436e 100644

drivers/android/binder.c static void binder_vma_close(struct vm_area_struct *vma) · line 6022

Quoted 5 lines
drivers/android/binder.c +2 -0
@@ -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;
}
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

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

maintainer
#10 lore raw

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

Quoted 59 lines · Carlos Llamas
Quoted 36 lines · Alice Ryhl
Quoted 35 lines · Carlos Llamas
Quoted 34 lines · Alice Ryhl
Quoted 18 lines · 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.

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/android/binder.c +0 -0
index 185128577829..3d359490436e 100644

drivers/android/binder.c static void binder_vma_close(struct vm_area_struct *vma) · line 6022

Quoted 5 lines
drivers/android/binder.c +2 -0
@@ -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;
}
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

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;
Quoted 7 lines

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

author maintainer
#11 lore raw

PATCH v4 6/7 binder: reject mremap()

In reply to 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.

Fixes 457b9a6f09f0 ("Staging: android: add binder driver")
---
 drivers/android/binder.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/android/binder.c +6 -0
index 3d359490436e..290ccfde624d 100644
@@ -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)
Signature · 1 line
2.55.0.966.g6673acef38-goog
author maintainer
#12 lore raw

PATCH v4 7/7 rust_binder: reject mremap()

In reply to 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.

Fixes eafedbc7c050 ("rust_binder: add Rust Binder driver")
---
 drivers/android/binder/page_range.rs | 5 +++++
 1 file changed, 5 insertions(+)
drivers/android/binder/page_range.rs +5 -0
index d260b009c184..c6247ba03313 100644
@@ -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)
Signature · 1 line
2.55.0.966.g6673acef38-goog