Commit b39e6c0e authored by Jann Horn's avatar Jann Horn Committed by Bernhard Thoben
Browse files

binder: Don't modify VMA bounds in ->mmap handler



binder_mmap() tries to prevent the creation of overly big binder mappings
by silently truncating the size of the VMA to 4MiB. However, this violates
the API contract of mmap(). If userspace attempts to create a large binder
VMA, and later attempts to unmap that VMA, it will call munmap() on a range
beyond the end of the VMA, which may have been allocated to another VMA in
the meantime. This can lead to userspace memory corruption.

The following sequence of calls leads to a segfault without this commit:

int main(void) {
  int binder_fd = open("/dev/binder", O_RDWR);
  if (binder_fd == -1) err(1, "open binder");
  void *binder_mapping = mmap(NULL, 0x800000UL, PROT_READ, MAP_SHARED,
                              binder_fd, 0);
  if (binder_mapping == MAP_FAILED) err(1, "mmap binder");
  void *data_mapping = mmap(NULL, 0x400000UL, PROT_READ|PROT_WRITE,
                            MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
  if (data_mapping == MAP_FAILED) err(1, "mmap data");
  munmap(binder_mapping, 0x800000UL);
  *(char*)data_mapping = 1;
  return 0;
}

Cc: stable@vger.kernel.org
Signed-off-by: default avatarJann Horn <jannh@google.com>
Acked-by: default avatarTodd Kjos <tkjos@google.com>
Acked-by: default avatarChristian Brauner <christian.brauner@ubuntu.com>
Link: https://lore.kernel.org/r/20191016150119.154756-1-jannh@google.com

Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 45d02f79b539073b76077836871de6b674e36eb4)
parent 1ed959a6
......@@ -103,10 +103,6 @@ DEFINE_SHOW_ATTRIBUTE(proc);
#define SZ_1K 0x400
#endif
#ifndef SZ_4M
#define SZ_4M 0x400000
#endif
#define FORBIDDEN_MMAP_FLAGS (VM_WRITE)
#define BINDER_SMALL_BUF_SIZE (PAGE_SIZE * 64)
......@@ -5297,9 +5293,6 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
if (proc->tsk != current->group_leader)
return -EINVAL;
if ((vma->vm_end - vma->vm_start) > SZ_4M)
vma->vm_end = vma->vm_start + SZ_4M;
binder_debug(BINDER_DEBUG_OPEN_CLOSE,
"%s: %d %lx-%lx (%ld K) vma %lx pagep %lx\n",
__func__, proc->pid, vma->vm_start, vma->vm_end,
......
......@@ -28,6 +28,7 @@
#include <linux/vmalloc.h>
#include <linux/slab.h>
#include <linux/sched.h>
#include <linux/sizes.h>
#include "binder_alloc.h"
#include "binder_trace.h"
......@@ -693,26 +694,16 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
vma->vm_start - (uintptr_t)alloc->buffer;
mutex_unlock(&binder_alloc_mmap_lock);
#ifdef CONFIG_CPU_CACHE_VIPT
if (cache_is_vipt_aliasing()) {
while (CACHE_COLOUR(
(vma->vm_start ^ (uint32_t)alloc->buffer))) {
pr_info("binder_mmap: %d %lx-%lx maps %pK bad alignment\n",
alloc->pid, vma->vm_start, vma->vm_end,
alloc->buffer);
vma->vm_start += PAGE_SIZE;
}
}
#endif
alloc->pages = kzalloc(sizeof(alloc->pages[0]) *
((vma->vm_end - vma->vm_start) / PAGE_SIZE),
alloc->buffer_size = min_t(unsigned long, vma->vm_end - vma->vm_start,
SZ_4M);
alloc->pages = kcalloc(alloc->buffer_size / PAGE_SIZE,
sizeof(alloc->pages[0]),
GFP_KERNEL);
if (alloc->pages == NULL) {
ret = -ENOMEM;
failure_string = "alloc page array";
goto err_alloc_pages_failed;
}
alloc->buffer_size = vma->vm_end - vma->vm_start;
buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
if (!buffer) {
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment