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.

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

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

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

 drivers/android/binder.c             | 12 +++++++++++-
 drivers/android/binder/page_range.rs | 15 ++++++++++++---
 drivers/android/binder/process.rs    |  1 +
 3 files changed, 24 insertions(+), 4 deletions(-)

2.55.0.897.gb25b4bd76c-goog

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

---
 drivers/android/binder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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;

2.55.0.897.gb25b4bd76c-goog

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

---
 drivers/android/binder/process.rs | 1 +
 1 file changed, 1 insertion(+)

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)

2.55.0.897.gb25b4bd76c-goog

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.

---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)

index 1b492ed48ea6..3ec3a77af106 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,

2.55.0.897.gb25b4bd76c-goog

I don't quite understand this. You explicitly made sure mremap() can't happen to begin with, so when is this called?

Alice

The issue is mremap() first clones the old vma and then calls ->mremap() but we return -EINVAL. So it now starts to tear-down the cloned vma and it calls ->close() as part of it.

The idea was to avoid shutting down our IPC when this happens.

However, sashiko pointed out a second issue with a tail split. I need to expand this check to allow the ->close() on anything within the range of the original VMA.

I'll have to send a v3.

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.

---
 drivers/android/binder.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 3ec3a77af106..76fc2edc1937 100644
@@ -6035,10 +6035,16 @@ VISIBLE_IF_KUNIT vm_fault_t binder_vm_fault(struct vm_fault *vmf)
 }
 EXPORT_SYMBOL_IF_KUNIT(binder_vm_fault);
 
+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,
+	.mremap = binder_mremap,
 };
 
 static int binder_mmap(struct file *filp, struct vm_area_struct *vma)

2.55.0.897.gb25b4bd76c-goog

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.

---
 drivers/android/binder/page_range.rs | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

index 52ffbf3504e7..411efcc2116f 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_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 {
+        mremap: Some(binder_mremap),
+        ..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.897.gb25b4bd76c-goog