[PATCH 1/2] binder: Don't modify VMA bounds in ->mmap handler

[PATCH 1/2] binder: Don't modify VMA bounds in ->mmap handler

[Jann Horn]

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

[PATCH 2/2] binder: Use common definition of SZ_1K

[Jann Horn]

SZ_1K has been defined in include/linux/sizes.h since v3.6. Get rid of the
duplicate definition.

Signed-off-by: Jann Horn <jannh@google.com>
---
 drivers/android/binder.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 265d9dd46a5e..a606262da9d6 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -65,6 +65,7 @@
 #include <linux/ratelimit.h>
 #include <linux/syscalls.h>
 #include <linux/task_work.h>
+#include <linux/sizes.h>
 
 #include <uapi/linux/android/binder.h>
 #include <uapi/linux/android/binderfs.h>
@@ -92,11 +93,6 @@ static atomic_t binder_last_id;
 static int proc_show(struct seq_file *m, void *unused);
 DEFINE_SHOW_ATTRIBUTE(proc);
 
-/* This is only defined in include/asm-arm/sizes.h */
-#ifndef SZ_1K
-#define SZ_1K                               0x400
-#endif
-
 #define FORBIDDEN_MMAP_FLAGS                (VM_WRITE)
 
 enum {
-- 
2.23.0.700.g56cf767bdb-goog

Re: [PATCH 2/2] binder: Use common definition of SZ_1K

[Christian Brauner]

On Wed Oct 16, 2019 at 5:01 PM Jann Horn wrote:
> SZ_1K has been defined in include/linux/sizes.h since v3.6. Get rid of the
> duplicate definition.
> 
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Re: [PATCH 1/2] binder: Don't modify VMA bounds in ->mmap handler

[Todd Kjos]

On Wed, Oct 16, 2019 at 8:01 AM Jann Horn <jannh@google.com> wrote:
>
> 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>

Acked-by: Todd Kjos <tkjos@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
>

Re: [PATCH 1/2] binder: Don't modify VMA bounds in ->mmap handler

[Christian Brauner]

On Wed Oct 16, 2019 at 5:01 PM Jann Horn wrote:
> 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>

Hm, aerc kept crashing for me so I'm not sure whether or not prior
messages made it so sorry if this arrives multiple times.

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>