binder: fix issues with mremap()
Subsystems (3)
Carlos Llamas
maintainerv4 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.
--
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
Carlos Llamas
maintainerPATCH 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().
--- drivers/android/binder.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/android/binder.c +2 -1
@@ -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
Carlos Llamas
maintainerPATCH v4 2/7 rust_binder: set VM_DONTEXPAND
In reply to Carlos LlamasBinder 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(+)
drivers/android/binder/process.rs +1 -0
@@ -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
Carlos Llamas
maintainerPATCH v4 3/7 binder: forbid vma splitting
In reply to Carlos LlamasBinder 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.
--- drivers/android/binder.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/android/binder.c +6 -0
@@ -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
Carlos Llamas
maintainerPATCH v4 4/7 rust_binder: forbid vma splitting
In reply to Carlos LlamasBinder 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.
--- drivers/android/binder/page_range.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/android/binder/page_range.rs +12 -3
@@ -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
Carlos Llamas
maintainerPATCH v4 5/7 binder: check vma->vm_start in binder_vma_close()
In reply to Carlos LlamasCertain 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(+)
drivers/android/binder.c +3 -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 ; + 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
Alice Ryhl
maintainerRe: [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 triggervm_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")Reported-by Sashiko <[email protected]>Signed-off-by Carlos Llamas <[email protected]>--- drivers/android/binder.c | 3 +++ 1 file changed, 3 insertions(+)drivers/android/binder.c +0
-0
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
Carlos Llamas
maintainerRe: [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 triggervm_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")Reported-by Sashiko <[email protected]>Signed-off-by Carlos Llamas <[email protected]>--- drivers/android/binder.c | 3 +++ 1 file changed, 3 insertions(+)drivers/android/binder.c +0
-0drivers/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
Alice Ryhl
maintainerRe: [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 triggervm_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")Reported-by Sashiko <[email protected]>Signed-off-by Carlos Llamas <[email protected]>--- drivers/android/binder.c | 3 +++ 1 file changed, 3 insertions(+)drivers/android/binder.c +0
-0drivers/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?
Carlos Llamas
maintainerRe: [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 triggervm_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")Reported-by Sashiko <[email protected]>Signed-off-by Carlos Llamas <[email protected]>--- drivers/android/binder.c | 3 +++ 1 file changed, 3 insertions(+)drivers/android/binder.c +0
-0drivers/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
Alice Ryhl
maintainerRe: [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 triggervm_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")Reported-by Sashiko <[email protected]>Signed-off-by Carlos Llamas <[email protected]>--- drivers/android/binder.c | 3 +++ 1 file changed, 3 insertions(+)drivers/android/binder.c +0
-0drivers/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_datafor 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_startis 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
Carlos Llamas
maintainerPATCH v4 6/7 binder: reject mremap()
In reply to Carlos LlamasBinder 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(+)
drivers/android/binder.c +6 -0
@@ -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
Carlos Llamas
maintainerPATCH v4 7/7 rust_binder: reject mremap()
In reply to Carlos LlamasBinder 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 | 5 +++++ 1 file changed, 5 insertions(+)
drivers/android/binder/page_range.rs +5 -0
@@ -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