[PATCH 1/3] binder: Fix race between mmap() and binder_alloc_print_pages()

[PATCH 1/3] binder: Fix race between mmap() and binder_alloc_print_pages()

[Jann Horn]

binder_alloc_print_pages() iterates over
alloc->pages[0..alloc->buffer_size-1] under alloc->mutex.
binder_alloc_mmap_handler() writes alloc->pages and alloc->buffer_size
without holding that lock, and even writes them before the last bailout
point.

Unfortunately we can't take the alloc->mutex in the ->mmap() handler
because mmap_sem can be taken while alloc->mutex is held.
So instead, we have to locklessly check whether the binder_alloc has been
fully initialized with binder_alloc_get_vma(), like in
binder_alloc_new_buf_locked().

Fixes: 8ef4665aa129 ("android: binder: Add page usage in binder stats")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
 drivers/android/binder_alloc.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index eb76a823fbb2..21952dfa147d 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -841,14 +841,20 @@ void binder_alloc_print_pages(struct seq_file *m,
 	int free = 0;
 
 	mutex_lock(&alloc->mutex);
-	for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
-		page = &alloc->pages[i];
-		if (!page->page_ptr)
-			free++;
-		else if (list_empty(&page->lru))
-			active++;
-		else
-			lru++;
+	/*
+	 * Make sure the binder_alloc is fully initialized, otherwise we might
+	 * read inconsistent state.
+	 */
+	if (binder_alloc_get_vma(alloc) != NULL) {
+		for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
+			page = &alloc->pages[i];
+			if (!page->page_ptr)
+				free++;
+			else if (list_empty(&page->lru))
+				active++;
+			else
+				lru++;
+		}
 	}
 	mutex_unlock(&alloc->mutex);
 	seq_printf(m, "  pages: %d:%d:%d\n", active, lru, free);
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH 2/3] binder: Prevent repeated use of ->mmap() via NULL mapping

[Jann Horn]

binder_alloc_mmap_handler() attempts to detect the use of ->mmap() on a
binder_proc whose binder_alloc has already been initialized by checking
whether alloc->buffer is non-zero.

Before commit 880211667b20 ("binder: remove kernel vm_area for buffer
space"), alloc->buffer was a kernel mapping address, which is always
non-zero, but since that commit, it is a userspace mapping address.

A sufficiently privileged user can map /dev/binder at NULL, tricking
binder_alloc_mmap_handler() into assuming that the binder_proc has not been
mapped yet. This leads to memory unsafety.
Luckily, no context on Android has such privileges, and on a typical Linux
desktop system, you need to be root to do that.

Fix it by using the mapping size instead of the mapping address to
distinguish the mapped case. A valid VMA can't have size zero.

Fixes: 880211667b20 ("binder: remove kernel vm_area for buffer space")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
 drivers/android/binder_alloc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 21952dfa147d..539385634151 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -681,17 +681,17 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
 	struct binder_buffer *buffer;
 
 	mutex_lock(&binder_alloc_mmap_lock);
-	if (alloc->buffer) {
+	if (alloc->buffer_size) {
 		ret = -EBUSY;
 		failure_string = "already mapped";
 		goto err_already_mapped;
 	}
+	alloc->buffer_size = min_t(unsigned long, vma->vm_end - vma->vm_start,
+				   SZ_4M);
+	mutex_unlock(&binder_alloc_mmap_lock);
 
 	alloc->buffer = (void __user *)vma->vm_start;
-	mutex_unlock(&binder_alloc_mmap_lock);
 
-	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);
@@ -722,8 +722,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
 	kfree(alloc->pages);
 	alloc->pages = NULL;
 err_alloc_pages_failed:
-	mutex_lock(&binder_alloc_mmap_lock);
 	alloc->buffer = NULL;
+	mutex_lock(&binder_alloc_mmap_lock);
+	alloc->buffer_size = 0;
 err_already_mapped:
 	mutex_unlock(&binder_alloc_mmap_lock);
 	binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH 3/3] binder: Handle start==NULL in binder_update_page_range()

[Jann Horn]

The old loop wouldn't stop when reaching `start` if `start==NULL`, instead
continuing backwards to index -1 and crashing.

Luckily you need to be highly privileged to map things at NULL, so it's not
a big problem.

Fix it by adjusting the loop so that the loop variable is always in bounds.

This patch is deliberately minimal to simplify backporting, but IMO this
function could use a refactor. The jump labels in the second loop body are
horrible (the error gotos should be jumping to free_range instead), and
both loops would look nicer if they just iterated upwards through indices.
And the up_read()+mmput() shouldn't be duplicated like that.

Cc: stable@vger.kernel.org
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Signed-off-by: Jann Horn <jannh@google.com>
---
 drivers/android/binder_alloc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 539385634151..7067d5542a82 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -277,8 +277,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 	return 0;
 
 free_range:
-	for (page_addr = end - PAGE_SIZE; page_addr >= start;
-	     page_addr -= PAGE_SIZE) {
+	for (page_addr = end - PAGE_SIZE; 1; page_addr -= PAGE_SIZE) {
 		bool ret;
 		size_t index;
 
@@ -291,6 +290,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		WARN_ON(!ret);
 
 		trace_binder_free_lru_end(alloc, index);
+		if (page_addr == start)
+			break;
 		continue;
 
 err_vm_insert_page_failed:
@@ -298,7 +299,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		page->page_ptr = NULL;
 err_alloc_page_failed:
 err_page_ptr_cleared:
-		;
+		if (page_addr == start)
+			break;
 	}
 err_no_vma:
 	if (mm) {
-- 
2.23.0.866.gb869b98d4c-goog

Re: [PATCH 1/3] binder: Fix race between mmap() and binder_alloc_print_pages()

[Christian Brauner]

On Fri Oct 18, 2019 at 10:56 PM Jann Horn wrote:
> binder_alloc_print_pages() iterates over
> alloc->pages[0..alloc->buffer_size-1] under alloc->mutex.
> binder_alloc_mmap_handler() writes alloc->pages and alloc->buffer_size
> without holding that lock, and even writes them before the last bailout
> point.
> 
> Unfortunately we can't take the alloc->mutex in the ->mmap() handler
> because mmap_sem can be taken while alloc->mutex is held.
> So instead, we have to locklessly check whether the binder_alloc has been
> fully initialized with binder_alloc_get_vma(), like in
> binder_alloc_new_buf_locked().
> 
> Fixes: 8ef4665aa129 ("android: binder: Add page usage in binder stats")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Ok, I see a smp_wmb() in binder_alloc_set_vma() which is called in
binder_alloc_mmap_handler() paired with a smp_rmb() in
binder_alloc_get_vma(). That makes sense to me.

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

Re: [PATCH 3/3] binder: Handle start==NULL in binder_update_page_range()

[Christian Brauner]

On Fri Oct 18, 2019 at 10:56 PM Jann Horn wrote:
> The old loop wouldn't stop when reaching `start` if `start==NULL`, instead
> continuing backwards to index -1 and crashing.
> 
> Luckily you need to be highly privileged to map things at NULL, so it's not
> a big problem.
> 
> Fix it by adjusting the loop so that the loop variable is always in bounds.
> 
> This patch is deliberately minimal to simplify backporting, but IMO this
> function could use a refactor. The jump labels in the second loop body are
> horrible (the error gotos should be jumping to free_range instead), and
> both loops would look nicer if they just iterated upwards through indices.
> And the up_read()+mmput() shouldn't be duplicated like that.
> 
> Cc: stable@vger.kernel.org
> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> Signed-off-by: Jann Horn <jannh@google.com>

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

Re: [PATCH 2/3] binder: Prevent repeated use of ->mmap() via NULL mapping

[Christian Brauner]

On Fri Oct 18, 2019 at 10:56 PM Jann Horn wrote:
> binder_alloc_mmap_handler() attempts to detect the use of ->mmap() on a
> binder_proc whose binder_alloc has already been initialized by checking
> whether alloc->buffer is non-zero.
> 
> Before commit 880211667b20 ("binder: remove kernel vm_area for buffer
> space"), alloc->buffer was a kernel mapping address, which is always
> non-zero, but since that commit, it is a userspace mapping address.
> 
> A sufficiently privileged user can map /dev/binder at NULL, tricking
> binder_alloc_mmap_handler() into assuming that the binder_proc has not been
> mapped yet. This leads to memory unsafety.
> Luckily, no context on Android has such privileges, and on a typical Linux
> desktop system, you need to be root to do that.
> 
> Fix it by using the mapping size instead of the mapping address to
> distinguish the mapped case. A valid VMA can't have size zero.
> 
> Fixes: 880211667b20 ("binder: remove kernel vm_area for buffer space")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

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