From: Jann Horn <jannh@google.com> To: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Arve Hjønnevåg" <arve@android.com>, "Todd Kjos" <tkjos@android.com>, "Martijn Coenen" <maco@android.com>, "Joel Fernandes" <joel@joelfernandes.org>, "Christian Brauner" <christian@brauner.io>, jannh@google.com Cc: devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: [PATCH 1/2] binder: Don't modify VMA bounds in ->mmap handler Date: Wed, 16 Oct 2019 17:01:18 +0200 [thread overview] Message-ID: <20191016150119.154756-1-jannh@google.com> (raw) 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: Jann Horn <jannh@google.com> --- drivers/android/binder.c | 7 ------- drivers/android/binder_alloc.c | 6 ++++-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 5b9ac2122e89..265d9dd46a5e 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -97,10 +97,6 @@ DEFINE_SHOW_ATTRIBUTE(proc); #define SZ_1K 0x400 #endif -#ifndef SZ_4M -#define SZ_4M 0x400000 -#endif - #define FORBIDDEN_MMAP_FLAGS (VM_WRITE) enum { @@ -5177,9 +5173,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, diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index d42a8b2f636a..eb76a823fbb2 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -22,6 +22,7 @@ #include <asm/cacheflush.h> #include <linux/uaccess.h> #include <linux/highmem.h> +#include <linux/sizes.h> #include "binder_alloc.h" #include "binder_trace.h" @@ -689,7 +690,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, alloc->buffer = (void __user *)vma->vm_start; mutex_unlock(&binder_alloc_mmap_lock); - alloc->pages = kcalloc((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) { @@ -697,7 +700,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, 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) { -- 2.23.0.700.g56cf767bdb-goog
WARNING: multiple messages have this Message-ID (diff)
From: Jann Horn <jannh@google.com> To: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Arve Hjønnevåg" <arve@android.com>, "Todd Kjos" <tkjos@android.com>, "Martijn Coenen" <maco@android.com>, "Joel Fernandes" <joel@joelfernandes.org>, "Christian Brauner" <christian@brauner.io>, jannh@google.com Cc: devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: [PATCH 1/2] binder: Don't modify VMA bounds in ->mmap handler Date: Wed, 16 Oct 2019 17:01:18 +0200 [thread overview] Message-ID: <20191016150119.154756-1-jannh@google.com> (raw) 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: Jann Horn <jannh@google.com> --- drivers/android/binder.c | 7 ------- drivers/android/binder_alloc.c | 6 ++++-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 5b9ac2122e89..265d9dd46a5e 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -97,10 +97,6 @@ DEFINE_SHOW_ATTRIBUTE(proc); #define SZ_1K 0x400 #endif -#ifndef SZ_4M -#define SZ_4M 0x400000 -#endif - #define FORBIDDEN_MMAP_FLAGS (VM_WRITE) enum { @@ -5177,9 +5173,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, diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index d42a8b2f636a..eb76a823fbb2 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -22,6 +22,7 @@ #include <asm/cacheflush.h> #include <linux/uaccess.h> #include <linux/highmem.h> +#include <linux/sizes.h> #include "binder_alloc.h" #include "binder_trace.h" @@ -689,7 +690,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, alloc->buffer = (void __user *)vma->vm_start; mutex_unlock(&binder_alloc_mmap_lock); - alloc->pages = kcalloc((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) { @@ -697,7 +700,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, 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) { -- 2.23.0.700.g56cf767bdb-goog _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next reply other threads:[~2019-10-16 15:01 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-16 15:01 Jann Horn [this message] 2019-10-16 15:01 ` [PATCH 1/2] binder: Don't modify VMA bounds in ->mmap handler Jann Horn 2019-10-16 15:01 ` [PATCH 2/2] binder: Use common definition of SZ_1K Jann Horn 2019-10-16 15:01 ` Jann Horn 2019-10-16 15:16 ` Christian Brauner 2019-10-16 15:16 ` Christian Brauner 2019-10-16 15:42 ` [PATCH 1/2] binder: Don't modify VMA bounds in ->mmap handler Todd Kjos 2019-10-16 15:42 ` Todd Kjos 2019-10-16 15:46 ` Christian Brauner 2019-10-16 15:46 ` Christian Brauner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191016150119.154756-1-jannh@google.com \ --to=jannh@google.com \ --cc=arve@android.com \ --cc=christian@brauner.io \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=joel@joelfernandes.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maco@android.com \ --cc=tkjos@android.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.