[PATCH 0/7] fix null-ptr-deref in binder_alloc and others

[PATCH 0/7] fix null-ptr-deref in binder_alloc and others

[Carlos Llamas]

This patch series fixes primarily a null dereference of alloc->vma_vm_mm
reported by syzbot which unfortunately is quite easy to reproduce. Also,
included here are several other patches for more trivial things I found
along the way.

--
Carlos Llamas

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Arve Hjønnevåg" <arve@android.com>
Cc: Todd Kjos <tkjos@android.com>
Cc: Martijn Coenen <maco@android.com>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Liam Howlett <liam.howlett@oracle.com>
Cc: kernel-team@android.com
Cc: linux-kernel@vger.kernel.org

Carlos Llamas (7):
  binder: fix alloc->vma_vm_mm null-ptr dereference
  binder: fix trivial kernel-doc typo
  binder: rename alloc->vma_vm_mm to alloc->mm
  binder: remove binder_alloc_set_vma()
  binder: remove unused binder_alloc->buffer_free
  binder: fix binder_alloc kernel-doc warnings
  binderfs: remove unused INTSTRLEN macro

 drivers/android/binder_alloc.c | 55 +++++++++++-----------------------
 drivers/android/binder_alloc.h | 12 ++++----
 drivers/android/binderfs.c     |  1 -
 3 files changed, 22 insertions(+), 46 deletions(-)

-- 
2.37.2.672.g94769d06f0-goog

[PATCH 1/7] binder: fix alloc->vma_vm_mm null-ptr dereference

[Carlos Llamas]

Syzbot reported a couple issues introduced by commit 44e602b4e52f
("binder_alloc: add missing mmap_lock calls when using the VMA"), in
which we attempt to acquire the mmap_lock when alloc->vma_vm_mm has not
been initialized yet.

This can happen if a binder_proc receives a transaction without having
previously called mmap() to setup the binder_proc->alloc space in [1].
Also, a similar issue occurs via binder_alloc_print_pages() when we try
to dump the debugfs binder stats file in [2].

Sample of syzbot's crash report:
  ==================================================================
  KASAN: null-ptr-deref in range [0x0000000000000128-0x000000000000012f]
  CPU: 0 PID: 3755 Comm: syz-executor229 Not tainted 6.0.0-rc1-next-20220819-syzkaller #0
  syz-executor229[3755] cmdline: ./syz-executor2294415195
  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
  RIP: 0010:__lock_acquire+0xd83/0x56d0 kernel/locking/lockdep.c:4923
  [...]
  Call Trace:
   <TASK>
   lock_acquire kernel/locking/lockdep.c:5666 [inline]
   lock_acquire+0x1ab/0x570 kernel/locking/lockdep.c:5631
   down_read+0x98/0x450 kernel/locking/rwsem.c:1499
   mmap_read_lock include/linux/mmap_lock.h:117 [inline]
   binder_alloc_new_buf_locked drivers/android/binder_alloc.c:405 [inline]
   binder_alloc_new_buf+0xa5/0x19e0 drivers/android/binder_alloc.c:593
   binder_transaction+0x242e/0x9a80 drivers/android/binder.c:3199
   binder_thread_write+0x664/0x3220 drivers/android/binder.c:3986
   binder_ioctl_write_read drivers/android/binder.c:5036 [inline]
   binder_ioctl+0x3470/0x6d00 drivers/android/binder.c:5323
   vfs_ioctl fs/ioctl.c:51 [inline]
   __do_sys_ioctl fs/ioctl.c:870 [inline]
   __se_sys_ioctl fs/ioctl.c:856 [inline]
   __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
   do_syscall_x64 arch/x86/entry/common.c:50 [inline]
   do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
   entry_SYSCALL_64_after_hwframe+0x63/0xcd
   [...]
  ==================================================================

Fix these issues by setting up alloc->vma_vm_mm pointer during open()
and caching directly from current->mm. This guarantees we have a valid
reference to take the mmap_lock during scenarios described above.

[1] https://syzkaller.appspot.com/bug?extid=f7dc54e5be28950ac459
[2] https://syzkaller.appspot.com/bug?extid=a75ebe0452711c9e56d9

Fixes: 44e602b4e52f ("binder_alloc: add missing mmap_lock calls when using the VMA")
Reported-by: syzbot+f7dc54e5be28950ac459@syzkaller.appspotmail.com
Reported-by: syzbot+a75ebe0452711c9e56d9@syzkaller.appspotmail.com
Cc: <stable@vger.kernel.org> # v5.15+
Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 51f4e1c5cd01..9b1778c00610 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -322,7 +322,6 @@ static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
 	 */
 	if (vma) {
 		vm_start = vma->vm_start;
-		alloc->vma_vm_mm = vma->vm_mm;
 		mmap_assert_write_locked(alloc->vma_vm_mm);
 	} else {
 		mmap_assert_locked(alloc->vma_vm_mm);
@@ -795,7 +794,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
 	binder_insert_free_buffer(alloc, buffer);
 	alloc->free_async_space = alloc->buffer_size / 2;
 	binder_alloc_set_vma(alloc, vma);
-	mmgrab(alloc->vma_vm_mm);
 
 	return 0;
 
@@ -1091,6 +1089,8 @@ static struct shrinker binder_shrinker = {
 void binder_alloc_init(struct binder_alloc *alloc)
 {
 	alloc->pid = current->group_leader->pid;
+	alloc->vma_vm_mm = current->mm;
+	mmgrab(alloc->vma_vm_mm);
 	mutex_init(&alloc->mutex);
 	INIT_LIST_HEAD(&alloc->buffers);
 }
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 2/7] binder: fix trivial kernel-doc typo

[Carlos Llamas]

Correct the misspelling of 'invariant' in kernel-doc section.

No functional changes in this patch.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 1e4fd37af5e0..0c37935ff7a2 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -75,10 +75,10 @@ struct binder_lru_page {
 /**
  * struct binder_alloc - per-binder proc state for binder allocator
  * @vma:                vm_area_struct passed to mmap_handler
- *                      (invarient after mmap)
+ *                      (invariant after mmap)
  * @tsk:                tid for task that called init for this proc
  *                      (invariant after init)
- * @vma_vm_mm:          copy of vma->vm_mm (invarient after mmap)
+ * @vma_vm_mm:          copy of vma->vm_mm (invariant after mmap)
  * @buffer:             base of per-proc address space mapped via mmap
  * @buffers:            list of all buffers for this proc
  * @free_buffers:       rb tree of buffers available for allocation
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 3/7] binder: rename alloc->vma_vm_mm to alloc->mm

[Carlos Llamas]

Rename ->vma_vm_mm to ->mm to reflect the fact that we no longer cache
this reference from vma->vm_mm but from current->mm instead.

No functional changes in this patch.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 34 +++++++++++++++++-----------------
 drivers/android/binder_alloc.h |  4 ++--
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 9b1778c00610..749a4cd30a83 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -208,8 +208,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		}
 	}
 
-	if (need_mm && mmget_not_zero(alloc->vma_vm_mm))
-		mm = alloc->vma_vm_mm;
+	if (need_mm && mmget_not_zero(alloc->mm))
+		mm = alloc->mm;
 
 	if (mm) {
 		mmap_read_lock(mm);
@@ -322,9 +322,9 @@ static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
 	 */
 	if (vma) {
 		vm_start = vma->vm_start;
-		mmap_assert_write_locked(alloc->vma_vm_mm);
+		mmap_assert_write_locked(alloc->mm);
 	} else {
-		mmap_assert_locked(alloc->vma_vm_mm);
+		mmap_assert_locked(alloc->mm);
 	}
 
 	alloc->vma_addr = vm_start;
@@ -336,7 +336,7 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
 	struct vm_area_struct *vma = NULL;
 
 	if (alloc->vma_addr)
-		vma = vma_lookup(alloc->vma_vm_mm, alloc->vma_addr);
+		vma = vma_lookup(alloc->mm, alloc->vma_addr);
 
 	return vma;
 }
@@ -401,15 +401,15 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	size_t size, data_offsets_size;
 	int ret;
 
-	mmap_read_lock(alloc->vma_vm_mm);
+	mmap_read_lock(alloc->mm);
 	if (!binder_alloc_get_vma(alloc)) {
-		mmap_read_unlock(alloc->vma_vm_mm);
+		mmap_read_unlock(alloc->mm);
 		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
 				   "%d: binder_alloc_buf, no vma\n",
 				   alloc->pid);
 		return ERR_PTR(-ESRCH);
 	}
-	mmap_read_unlock(alloc->vma_vm_mm);
+	mmap_read_unlock(alloc->mm);
 
 	data_offsets_size = ALIGN(data_size, sizeof(void *)) +
 		ALIGN(offsets_size, sizeof(void *));
@@ -823,7 +823,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 	buffers = 0;
 	mutex_lock(&alloc->mutex);
 	BUG_ON(alloc->vma_addr &&
-	       vma_lookup(alloc->vma_vm_mm, alloc->vma_addr));
+	       vma_lookup(alloc->mm, alloc->vma_addr));
 
 	while ((n = rb_first(&alloc->allocated_buffers))) {
 		buffer = rb_entry(n, struct binder_buffer, rb_node);
@@ -873,8 +873,8 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 		kfree(alloc->pages);
 	}
 	mutex_unlock(&alloc->mutex);
-	if (alloc->vma_vm_mm)
-		mmdrop(alloc->vma_vm_mm);
+	if (alloc->mm)
+		mmdrop(alloc->mm);
 
 	binder_alloc_debug(BINDER_DEBUG_OPEN_CLOSE,
 		     "%s: %d buffers %d, pages %d\n",
@@ -931,13 +931,13 @@ void binder_alloc_print_pages(struct seq_file *m,
 	 * read inconsistent state.
 	 */
 
-	mmap_read_lock(alloc->vma_vm_mm);
+	mmap_read_lock(alloc->mm);
 	if (binder_alloc_get_vma(alloc) == NULL) {
-		mmap_read_unlock(alloc->vma_vm_mm);
+		mmap_read_unlock(alloc->mm);
 		goto uninitialized;
 	}
 
-	mmap_read_unlock(alloc->vma_vm_mm);
+	mmap_read_unlock(alloc->mm);
 	for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
 		page = &alloc->pages[i];
 		if (!page->page_ptr)
@@ -1020,7 +1020,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 	index = page - alloc->pages;
 	page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE;
 
-	mm = alloc->vma_vm_mm;
+	mm = alloc->mm;
 	if (!mmget_not_zero(mm))
 		goto err_mmget;
 	if (!mmap_read_trylock(mm))
@@ -1089,8 +1089,8 @@ static struct shrinker binder_shrinker = {
 void binder_alloc_init(struct binder_alloc *alloc)
 {
 	alloc->pid = current->group_leader->pid;
-	alloc->vma_vm_mm = current->mm;
-	mmgrab(alloc->vma_vm_mm);
+	alloc->mm = current->mm;
+	mmgrab(alloc->mm);
 	mutex_init(&alloc->mutex);
 	INIT_LIST_HEAD(&alloc->buffers);
 }
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 0c37935ff7a2..fe80cc405707 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -78,7 +78,7 @@ struct binder_lru_page {
  *                      (invariant after mmap)
  * @tsk:                tid for task that called init for this proc
  *                      (invariant after init)
- * @vma_vm_mm:          copy of vma->vm_mm (invariant after mmap)
+ * @mm:                 copy of task->mm (invariant after open)
  * @buffer:             base of per-proc address space mapped via mmap
  * @buffers:            list of all buffers for this proc
  * @free_buffers:       rb tree of buffers available for allocation
@@ -101,7 +101,7 @@ struct binder_lru_page {
 struct binder_alloc {
 	struct mutex mutex;
 	unsigned long vma_addr;
-	struct mm_struct *vma_vm_mm;
+	struct mm_struct *mm;
 	void __user *buffer;
 	struct list_head buffers;
 	struct rb_root free_buffers;
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 4/7] binder: remove binder_alloc_set_vma()

[Carlos Llamas]

The mmap_locked asserts here are not needed since this is only called
back from the mmap stack in ->mmap() and ->close() which always acquire
the lock first. Remove these asserts along with binder_alloc_set_vma()
altogether since it's trivial enough to be consumed by callers.

Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 749a4cd30a83..1c39cfce32fa 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -309,27 +309,6 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 	return vma ? -ENOMEM : -ESRCH;
 }
 
-
-static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
-		struct vm_area_struct *vma)
-{
-	unsigned long vm_start = 0;
-
-	/*
-	 * Allow clearing the vma with holding just the read lock to allow
-	 * munmapping downgrade of the write lock before freeing and closing the
-	 * file using binder_alloc_vma_close().
-	 */
-	if (vma) {
-		vm_start = vma->vm_start;
-		mmap_assert_write_locked(alloc->mm);
-	} else {
-		mmap_assert_locked(alloc->mm);
-	}
-
-	alloc->vma_addr = vm_start;
-}
-
 static inline struct vm_area_struct *binder_alloc_get_vma(
 		struct binder_alloc *alloc)
 {
@@ -793,7 +772,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
 	buffer->free = 1;
 	binder_insert_free_buffer(alloc, buffer);
 	alloc->free_async_space = alloc->buffer_size / 2;
-	binder_alloc_set_vma(alloc, vma);
+	alloc->vma_addr = vma->vm_start;
 
 	return 0;
 
@@ -983,7 +962,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc)
  */
 void binder_alloc_vma_close(struct binder_alloc *alloc)
 {
-	binder_alloc_set_vma(alloc, NULL);
+	alloc->vma_addr = 0;
 }
 
 /**
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 5/7] binder: remove unused binder_alloc->buffer_free

[Carlos Llamas]

The ->buffer_free member was introduced in the first revision of the
driver under staging but it appears like it was never actually used
according to git's history. Remove it from binder_alloc.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index fe80cc405707..ab3b027bcd29 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -109,7 +109,6 @@ struct binder_alloc {
 	size_t free_async_space;
 	struct binder_lru_page *pages;
 	size_t buffer_size;
-	uint32_t buffer_free;
 	int pid;
 	size_t pages_high;
 	bool oneway_spam_detected;
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 6/7] binder: fix binder_alloc kernel-doc warnings

[Carlos Llamas]

Update the kernel-doc section of struct binder_alloc to fix the
following warnings reported by ./scripts/kernel-doc:

  warning: Function parameter or member 'mutex' not described in 'binder_alloc'
  warning: Function parameter or member 'vma_addr' not described in 'binder_alloc'

No functional changes in this patch.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index ab3b027bcd29..0f811ac4bcff 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -74,10 +74,9 @@ struct binder_lru_page {
 
 /**
  * struct binder_alloc - per-binder proc state for binder allocator
- * @vma:                vm_area_struct passed to mmap_handler
+ * @mutex:              protects binder_alloc fields
+ * @vma_addr:           vm_area_struct->vm_start passed to mmap_handler
  *                      (invariant after mmap)
- * @tsk:                tid for task that called init for this proc
- *                      (invariant after init)
  * @mm:                 copy of task->mm (invariant after open)
  * @buffer:             base of per-proc address space mapped via mmap
  * @buffers:            list of all buffers for this proc
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 7/7] binderfs: remove unused INTSTRLEN macro

[Carlos Llamas]

Fix the following W=1 build error:

drivers/android/binderfs.c:42: error: macro "INTSTRLEN" is not used [-Werror=unused-macros]
   42 | #define INTSTRLEN 21
      |

No functional changes in this patch.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binderfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 588d753a7a19..44939ea1874f 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -39,7 +39,6 @@
 #define FIRST_INODE 1
 #define SECOND_INODE 2
 #define INODE_OFFSET 3
-#define INTSTRLEN 21
 #define BINDERFS_MAX_MINOR (1U << MINORBITS)
 /* Ensure that the initial ipc namespace always has devices available. */
 #define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4)
-- 
2.37.2.672.g94769d06f0-goog

Re: [PATCH 1/7] binder: fix alloc->vma_vm_mm null-ptr dereference

[Andrew Morton]

On Mon, 29 Aug 2022 20:12:48 +0000 Carlos Llamas <cmllamas@google.com> wrote:

> Syzbot reported a couple issues introduced by commit 44e602b4e52f
> ("binder_alloc: add missing mmap_lock calls when using the VMA"), in
> which we attempt to acquire the mmap_lock when alloc->vma_vm_mm has not
> been initialized yet.
> 
> This can happen if a binder_proc receives a transaction without having
> previously called mmap() to setup the binder_proc->alloc space in [1].
> Also, a similar issue occurs via binder_alloc_print_pages() when we try
> to dump the debugfs binder stats file in [2].

Thanks.  I assume you'll be merging all these into mainline?

> 
> Fixes: 44e602b4e52f ("binder_alloc: add missing mmap_lock calls when using the VMA")
> Reported-by: syzbot+f7dc54e5be28950ac459@syzkaller.appspotmail.com
> Reported-by: syzbot+a75ebe0452711c9e56d9@syzkaller.appspotmail.com
> Cc: <stable@vger.kernel.org> # v5.15+

44e602b4e52f is only present in 6.0-rcX?

Re: [PATCH 1/7] binder: fix alloc->vma_vm_mm null-ptr dereference

[Carlos Llamas]

On Mon, Aug 29, 2022 at 01:34:52PM -0700, Andrew Morton wrote:
> On Mon, 29 Aug 2022 20:12:48 +0000 Carlos Llamas <cmllamas@google.com> wrote:
> 
> > Syzbot reported a couple issues introduced by commit 44e602b4e52f
> > ("binder_alloc: add missing mmap_lock calls when using the VMA"), in
> > which we attempt to acquire the mmap_lock when alloc->vma_vm_mm has not
> > been initialized yet.
> > 
> > This can happen if a binder_proc receives a transaction without having
> > previously called mmap() to setup the binder_proc->alloc space in [1].
> > Also, a similar issue occurs via binder_alloc_print_pages() when we try
> > to dump the debugfs binder stats file in [2].
> 
> Thanks.  I assume you'll be merging all these into mainline?

Yes, I believe Greg will pick up these patches into his char-misc tree.

> 
> > 
> > Fixes: 44e602b4e52f ("binder_alloc: add missing mmap_lock calls when using the VMA")
> > Reported-by: syzbot+f7dc54e5be28950ac459@syzkaller.appspotmail.com
> > Reported-by: syzbot+a75ebe0452711c9e56d9@syzkaller.appspotmail.com
> > Cc: <stable@vger.kernel.org> # v5.15+
> 
> 44e602b4e52f is only present in 6.0-rcX?

Right, it was just added to the stable queue earlier today:
https://lore.kernel.org/all/20220829105814.857786586@linuxfoundation.org/
https://lore.kernel.org/all/20220829105809.855177179@linuxfoundation.org/

Re: [PATCH 7/7] binderfs: remove unused INTSTRLEN macro

[Christian Brauner]

On Mon, Aug 29, 2022 at 08:12:54PM +0000, Carlos Llamas wrote:
> Fix the following W=1 build error:
> 
> drivers/android/binderfs.c:42: error: macro "INTSTRLEN" is not used [-Werror=unused-macros]
>    42 | #define INTSTRLEN 21
>       |
> 
> No functional changes in this patch.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---

Thanks,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Re: [PATCH 6/7] binder: fix binder_alloc kernel-doc warnings

[Christian Brauner]

On Mon, Aug 29, 2022 at 08:12:53PM +0000, Carlos Llamas wrote:
> Update the kernel-doc section of struct binder_alloc to fix the
> following warnings reported by ./scripts/kernel-doc:
> 
>   warning: Function parameter or member 'mutex' not described in 'binder_alloc'
>   warning: Function parameter or member 'vma_addr' not described in 'binder_alloc'
> 
> No functional changes in this patch.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Re: [PATCH 2/7] binder: fix trivial kernel-doc typo

[Christian Brauner]

On Mon, Aug 29, 2022 at 08:12:49PM +0000, Carlos Llamas wrote:
> Correct the misspelling of 'invariant' in kernel-doc section.
> 
> No functional changes in this patch.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Re: [PATCH 3/7] binder: rename alloc->vma_vm_mm to alloc->mm

[Christian Brauner]

On Mon, Aug 29, 2022 at 08:12:50PM +0000, Carlos Llamas wrote:
> Rename ->vma_vm_mm to ->mm to reflect the fact that we no longer cache
> this reference from vma->vm_mm but from current->mm instead.
> 
> No functional changes in this patch.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Re: [PATCH 5/7] binder: remove unused binder_alloc->buffer_free

[Christian Brauner]

On Mon, Aug 29, 2022 at 08:12:52PM +0000, Carlos Llamas wrote:
> The ->buffer_free member was introduced in the first revision of the
> driver under staging but it appears like it was never actually used
> according to git's history. Remove it from binder_alloc.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Re: [PATCH 4/7] binder: remove binder_alloc_set_vma()

[Liam Howlett]

* Carlos Llamas <cmllamas@google.com> [220829 16:13]:
> The mmap_locked asserts here are not needed since this is only called
> back from the mmap stack in ->mmap() and ->close() which always acquire
> the lock first. Remove these asserts along with binder_alloc_set_vma()
> altogether since it's trivial enough to be consumed by callers.

I agree that it is not called anywhere else today.  I think it's still
worth while since these asserts do nothing if you don't build with
lockdep and/or debug_vm enabled.  I was hoping having these here would
avoid future mistakes a lot like what we have in the mm code's
find_vma() (mm/mmap.c ~L2297).


> 
> Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder_alloc.c | 25 ++-----------------------
>  1 file changed, 2 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 749a4cd30a83..1c39cfce32fa 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -309,27 +309,6 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
>  	return vma ? -ENOMEM : -ESRCH;
>  }
>  
> -
> -static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
> -		struct vm_area_struct *vma)
> -{
> -	unsigned long vm_start = 0;
> -
> -	/*
> -	 * Allow clearing the vma with holding just the read lock to allow
> -	 * munmapping downgrade of the write lock before freeing and closing the
> -	 * file using binder_alloc_vma_close().
> -	 */
> -	if (vma) {
> -		vm_start = vma->vm_start;
> -		mmap_assert_write_locked(alloc->mm);
> -	} else {
> -		mmap_assert_locked(alloc->mm);
> -	}
> -
> -	alloc->vma_addr = vm_start;
> -}
> -
>  static inline struct vm_area_struct *binder_alloc_get_vma(
>  		struct binder_alloc *alloc)
>  {
> @@ -793,7 +772,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
>  	buffer->free = 1;
>  	binder_insert_free_buffer(alloc, buffer);
>  	alloc->free_async_space = alloc->buffer_size / 2;
> -	binder_alloc_set_vma(alloc, vma);
> +	alloc->vma_addr = vma->vm_start;
>  
>  	return 0;
>  
> @@ -983,7 +962,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc)
>   */
>  void binder_alloc_vma_close(struct binder_alloc *alloc)
>  {
> -	binder_alloc_set_vma(alloc, NULL);
> +	alloc->vma_addr = 0;
>  }
>  
>  /**
> -- 
> 2.37.2.672.g94769d06f0-goog
> 

Re: [PATCH 1/7] binder: fix alloc->vma_vm_mm null-ptr dereference

[Liam Howlett]

* Carlos Llamas <cmllamas@google.com> [220829 16:13]:
> Syzbot reported a couple issues introduced by commit 44e602b4e52f
> ("binder_alloc: add missing mmap_lock calls when using the VMA"), in
> which we attempt to acquire the mmap_lock when alloc->vma_vm_mm has not
> been initialized yet.
> 
> This can happen if a binder_proc receives a transaction without having
> previously called mmap() to setup the binder_proc->alloc space in [1].
> Also, a similar issue occurs via binder_alloc_print_pages() when we try
> to dump the debugfs binder stats file in [2].
> 
> Sample of syzbot's crash report:
>   ==================================================================
>   KASAN: null-ptr-deref in range [0x0000000000000128-0x000000000000012f]
>   CPU: 0 PID: 3755 Comm: syz-executor229 Not tainted 6.0.0-rc1-next-20220819-syzkaller #0
>   syz-executor229[3755] cmdline: ./syz-executor2294415195
>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
>   RIP: 0010:__lock_acquire+0xd83/0x56d0 kernel/locking/lockdep.c:4923
>   [...]
>   Call Trace:
>    <TASK>
>    lock_acquire kernel/locking/lockdep.c:5666 [inline]
>    lock_acquire+0x1ab/0x570 kernel/locking/lockdep.c:5631
>    down_read+0x98/0x450 kernel/locking/rwsem.c:1499
>    mmap_read_lock include/linux/mmap_lock.h:117 [inline]
>    binder_alloc_new_buf_locked drivers/android/binder_alloc.c:405 [inline]
>    binder_alloc_new_buf+0xa5/0x19e0 drivers/android/binder_alloc.c:593
>    binder_transaction+0x242e/0x9a80 drivers/android/binder.c:3199
>    binder_thread_write+0x664/0x3220 drivers/android/binder.c:3986
>    binder_ioctl_write_read drivers/android/binder.c:5036 [inline]
>    binder_ioctl+0x3470/0x6d00 drivers/android/binder.c:5323
>    vfs_ioctl fs/ioctl.c:51 [inline]
>    __do_sys_ioctl fs/ioctl.c:870 [inline]
>    __se_sys_ioctl fs/ioctl.c:856 [inline]
>    __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
>    do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>    do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>    entry_SYSCALL_64_after_hwframe+0x63/0xcd
>    [...]
>   ==================================================================
> 
> Fix these issues by setting up alloc->vma_vm_mm pointer during open()
> and caching directly from current->mm. This guarantees we have a valid
> reference to take the mmap_lock during scenarios described above.
> 
> [1] https://syzkaller.appspot.com/bug?extid=f7dc54e5be28950ac459
> [2] https://syzkaller.appspot.com/bug?extid=a75ebe0452711c9e56d9
> 
> Fixes: 44e602b4e52f ("binder_alloc: add missing mmap_lock calls when using the VMA")
> Reported-by: syzbot+f7dc54e5be28950ac459@syzkaller.appspotmail.com
> Reported-by: syzbot+a75ebe0452711c9e56d9@syzkaller.appspotmail.com
> Cc: <stable@vger.kernel.org> # v5.15+
> Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder_alloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 51f4e1c5cd01..9b1778c00610 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -322,7 +322,6 @@ static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
>  	 */
>  	if (vma) {
>  		vm_start = vma->vm_start;
> -		alloc->vma_vm_mm = vma->vm_mm;

Is this really the null pointer dereference?  We check for vma above..?

>  		mmap_assert_write_locked(alloc->vma_vm_mm);
>  	} else {
>  		mmap_assert_locked(alloc->vma_vm_mm);
> @@ -795,7 +794,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
>  	binder_insert_free_buffer(alloc, buffer);
>  	alloc->free_async_space = alloc->buffer_size / 2;
>  	binder_alloc_set_vma(alloc, vma);
> -	mmgrab(alloc->vma_vm_mm);
>  
>  	return 0;
>  
> @@ -1091,6 +1089,8 @@ static struct shrinker binder_shrinker = {
>  void binder_alloc_init(struct binder_alloc *alloc)
>  {
>  	alloc->pid = current->group_leader->pid;
> +	alloc->vma_vm_mm = current->mm;
> +	mmgrab(alloc->vma_vm_mm);
>  	mutex_init(&alloc->mutex);
>  	INIT_LIST_HEAD(&alloc->buffers);
>  }
> -- 
> 2.37.2.672.g94769d06f0-goog
> 

Re: [PATCH 1/7] binder: fix alloc->vma_vm_mm null-ptr dereference

[Carlos Llamas]

On Tue, Aug 30, 2022 at 07:06:37PM +0000, Liam Howlett wrote:
> > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> > index 51f4e1c5cd01..9b1778c00610 100644
> > --- a/drivers/android/binder_alloc.c
> > +++ b/drivers/android/binder_alloc.c
> > @@ -322,7 +322,6 @@ static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
> >  	 */
> >  	if (vma) {
> >  		vm_start = vma->vm_start;
> > -		alloc->vma_vm_mm = vma->vm_mm;
> 
> Is this really the null pointer dereference?  We check for vma above..?
> 

Not here. The sequence leading to the null-ptr-deref happens when we try
to take alloc->vma_vm_mm->mmap_lock in binder_alloc_new_buf_locked() and
in binder_alloc_print_pages() without initializing alloc->vma_vm_mm
first (e.g. mmap() was never called). These sequences are described in
the commit message but basically they translate to mmap_read_lock(NULL)
calls.

Re: [PATCH 1/7] binder: fix alloc->vma_vm_mm null-ptr dereference

[Liam Howlett]

* Carlos Llamas <cmllamas@google.com> [220830 15:41]:
> On Tue, Aug 30, 2022 at 07:06:37PM +0000, Liam Howlett wrote:
> > > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> > > index 51f4e1c5cd01..9b1778c00610 100644
> > > --- a/drivers/android/binder_alloc.c
> > > +++ b/drivers/android/binder_alloc.c
> > > @@ -322,7 +322,6 @@ static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
> > >  	 */
> > >  	if (vma) {
> > >  		vm_start = vma->vm_start;
> > > -		alloc->vma_vm_mm = vma->vm_mm;
> > 
> > Is this really the null pointer dereference?  We check for vma above..?
> > 
> 
> Not here. The sequence leading to the null-ptr-deref happens when we try
> to take alloc->vma_vm_mm->mmap_lock in binder_alloc_new_buf_locked() and
> in binder_alloc_print_pages() without initializing alloc->vma_vm_mm
> first (e.g. mmap() was never called). These sequences are described in
> the commit message but basically they translate to mmap_read_lock(NULL)
> calls.

Ah, this is unnecessary with the rest of the change.

Feel free to add my reviewed-by if you want.


Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

Re: [PATCH 4/7] binder: remove binder_alloc_set_vma()

[Carlos Llamas]

On Tue, Aug 30, 2022 at 06:57:59PM +0000, Liam Howlett wrote:
> * Carlos Llamas <cmllamas@google.com> [220829 16:13]:
> > The mmap_locked asserts here are not needed since this is only called
> > back from the mmap stack in ->mmap() and ->close() which always acquire
> > the lock first. Remove these asserts along with binder_alloc_set_vma()
> > altogether since it's trivial enough to be consumed by callers.
> 
> I agree that it is not called anywhere else today.  I think it's still
> worth while since these asserts do nothing if you don't build with
> lockdep and/or debug_vm enabled.  I was hoping having these here would
> avoid future mistakes a lot like what we have in the mm code's
> find_vma() (mm/mmap.c ~L2297).
> 

Yes, the assert in find_vma() is perfectly fine, we need to check that
callers have taken the lock before looking up a vma. However, the
scenario here is different as these are callbacks for vm_ops->close()
and mmap() so we are guaranteed to have the lock at this point. We don't
really want to duplicate checks for each user of these callbacks such as
the binder driver here.

Re: [PATCH 3/7] binder: rename alloc->vma_vm_mm to alloc->mm

[Todd Kjos]

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<kernel-team@android.com> wrote:
>
> Rename ->vma_vm_mm to ->mm to reflect the fact that we no longer cache
> this reference from vma->vm_mm but from current->mm instead.
>
> No functional changes in this patch.
>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Acked-by: Todd Kjos <tkjos@google.com>

> ---
>  drivers/android/binder_alloc.c | 34 +++++++++++++++++-----------------
>  drivers/android/binder_alloc.h |  4 ++--
>  2 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 9b1778c00610..749a4cd30a83 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -208,8 +208,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
>                 }
>         }
>
> -       if (need_mm && mmget_not_zero(alloc->vma_vm_mm))
> -               mm = alloc->vma_vm_mm;
> +       if (need_mm && mmget_not_zero(alloc->mm))
> +               mm = alloc->mm;
>
>         if (mm) {
>                 mmap_read_lock(mm);
> @@ -322,9 +322,9 @@ static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
>          */
>         if (vma) {
>                 vm_start = vma->vm_start;
> -               mmap_assert_write_locked(alloc->vma_vm_mm);
> +               mmap_assert_write_locked(alloc->mm);
>         } else {
> -               mmap_assert_locked(alloc->vma_vm_mm);
> +               mmap_assert_locked(alloc->mm);
>         }
>
>         alloc->vma_addr = vm_start;
> @@ -336,7 +336,7 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
>         struct vm_area_struct *vma = NULL;
>
>         if (alloc->vma_addr)
> -               vma = vma_lookup(alloc->vma_vm_mm, alloc->vma_addr);
> +               vma = vma_lookup(alloc->mm, alloc->vma_addr);
>
>         return vma;
>  }
> @@ -401,15 +401,15 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
>         size_t size, data_offsets_size;
>         int ret;
>
> -       mmap_read_lock(alloc->vma_vm_mm);
> +       mmap_read_lock(alloc->mm);
>         if (!binder_alloc_get_vma(alloc)) {
> -               mmap_read_unlock(alloc->vma_vm_mm);
> +               mmap_read_unlock(alloc->mm);
>                 binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
>                                    "%d: binder_alloc_buf, no vma\n",
>                                    alloc->pid);
>                 return ERR_PTR(-ESRCH);
>         }
> -       mmap_read_unlock(alloc->vma_vm_mm);
> +       mmap_read_unlock(alloc->mm);
>
>         data_offsets_size = ALIGN(data_size, sizeof(void *)) +
>                 ALIGN(offsets_size, sizeof(void *));
> @@ -823,7 +823,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
>         buffers = 0;
>         mutex_lock(&alloc->mutex);
>         BUG_ON(alloc->vma_addr &&
> -              vma_lookup(alloc->vma_vm_mm, alloc->vma_addr));
> +              vma_lookup(alloc->mm, alloc->vma_addr));
>
>         while ((n = rb_first(&alloc->allocated_buffers))) {
>                 buffer = rb_entry(n, struct binder_buffer, rb_node);
> @@ -873,8 +873,8 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
>                 kfree(alloc->pages);
>         }
>         mutex_unlock(&alloc->mutex);
> -       if (alloc->vma_vm_mm)
> -               mmdrop(alloc->vma_vm_mm);
> +       if (alloc->mm)
> +               mmdrop(alloc->mm);
>
>         binder_alloc_debug(BINDER_DEBUG_OPEN_CLOSE,
>                      "%s: %d buffers %d, pages %d\n",
> @@ -931,13 +931,13 @@ void binder_alloc_print_pages(struct seq_file *m,
>          * read inconsistent state.
>          */
>
> -       mmap_read_lock(alloc->vma_vm_mm);
> +       mmap_read_lock(alloc->mm);
>         if (binder_alloc_get_vma(alloc) == NULL) {
> -               mmap_read_unlock(alloc->vma_vm_mm);
> +               mmap_read_unlock(alloc->mm);
>                 goto uninitialized;
>         }
>
> -       mmap_read_unlock(alloc->vma_vm_mm);
> +       mmap_read_unlock(alloc->mm);
>         for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
>                 page = &alloc->pages[i];
>                 if (!page->page_ptr)
> @@ -1020,7 +1020,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
>         index = page - alloc->pages;
>         page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE;
>
> -       mm = alloc->vma_vm_mm;
> +       mm = alloc->mm;
>         if (!mmget_not_zero(mm))
>                 goto err_mmget;
>         if (!mmap_read_trylock(mm))
> @@ -1089,8 +1089,8 @@ static struct shrinker binder_shrinker = {
>  void binder_alloc_init(struct binder_alloc *alloc)
>  {
>         alloc->pid = current->group_leader->pid;
> -       alloc->vma_vm_mm = current->mm;
> -       mmgrab(alloc->vma_vm_mm);
> +       alloc->mm = current->mm;
> +       mmgrab(alloc->mm);
>         mutex_init(&alloc->mutex);
>         INIT_LIST_HEAD(&alloc->buffers);
>  }
> diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
> index 0c37935ff7a2..fe80cc405707 100644
> --- a/drivers/android/binder_alloc.h
> +++ b/drivers/android/binder_alloc.h
> @@ -78,7 +78,7 @@ struct binder_lru_page {
>   *                      (invariant after mmap)
>   * @tsk:                tid for task that called init for this proc
>   *                      (invariant after init)
> - * @vma_vm_mm:          copy of vma->vm_mm (invariant after mmap)
> + * @mm:                 copy of task->mm (invariant after open)
>   * @buffer:             base of per-proc address space mapped via mmap
>   * @buffers:            list of all buffers for this proc
>   * @free_buffers:       rb tree of buffers available for allocation
> @@ -101,7 +101,7 @@ struct binder_lru_page {
>  struct binder_alloc {
>         struct mutex mutex;
>         unsigned long vma_addr;
> -       struct mm_struct *vma_vm_mm;
> +       struct mm_struct *mm;
>         void __user *buffer;
>         struct list_head buffers;
>         struct rb_root free_buffers;
> --
> 2.37.2.672.g94769d06f0-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH 2/7] binder: fix trivial kernel-doc typo

[Todd Kjos]

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<kernel-team@android.com> wrote:
>
> Correct the misspelling of 'invariant' in kernel-doc section.
>
> No functional changes in this patch.
>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Acked-by: Todd Kjos <tkjos@google.com>

> ---
>  drivers/android/binder_alloc.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
> index 1e4fd37af5e0..0c37935ff7a2 100644
> --- a/drivers/android/binder_alloc.h
> +++ b/drivers/android/binder_alloc.h
> @@ -75,10 +75,10 @@ struct binder_lru_page {
>  /**
>   * struct binder_alloc - per-binder proc state for binder allocator
>   * @vma:                vm_area_struct passed to mmap_handler
> - *                      (invarient after mmap)
> + *                      (invariant after mmap)
>   * @tsk:                tid for task that called init for this proc
>   *                      (invariant after init)
> - * @vma_vm_mm:          copy of vma->vm_mm (invarient after mmap)
> + * @vma_vm_mm:          copy of vma->vm_mm (invariant after mmap)
>   * @buffer:             base of per-proc address space mapped via mmap
>   * @buffers:            list of all buffers for this proc
>   * @free_buffers:       rb tree of buffers available for allocation
> --
> 2.37.2.672.g94769d06f0-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH 5/7] binder: remove unused binder_alloc->buffer_free

[Todd Kjos]

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<kernel-team@android.com> wrote:
>
> The ->buffer_free member was introduced in the first revision of the
> driver under staging but it appears like it was never actually used
> according to git's history. Remove it from binder_alloc.
>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Acked-by: Todd Kjos <tkjos@google.com>

> ---
>  drivers/android/binder_alloc.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
> index fe80cc405707..ab3b027bcd29 100644
> --- a/drivers/android/binder_alloc.h
> +++ b/drivers/android/binder_alloc.h
> @@ -109,7 +109,6 @@ struct binder_alloc {
>         size_t free_async_space;
>         struct binder_lru_page *pages;
>         size_t buffer_size;
> -       uint32_t buffer_free;
>         int pid;
>         size_t pages_high;
>         bool oneway_spam_detected;
> --
> 2.37.2.672.g94769d06f0-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH 6/7] binder: fix binder_alloc kernel-doc warnings

[Todd Kjos]

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<kernel-team@android.com> wrote:
>
> Update the kernel-doc section of struct binder_alloc to fix the
> following warnings reported by ./scripts/kernel-doc:
>
>   warning: Function parameter or member 'mutex' not described in 'binder_alloc'
>   warning: Function parameter or member 'vma_addr' not described in 'binder_alloc'
>
> No functional changes in this patch.
>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Acked-by: Todd Kjos <tkjos@google.com>

> ---
>  drivers/android/binder_alloc.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
> index ab3b027bcd29..0f811ac4bcff 100644
> --- a/drivers/android/binder_alloc.h
> +++ b/drivers/android/binder_alloc.h
> @@ -74,10 +74,9 @@ struct binder_lru_page {
>
>  /**
>   * struct binder_alloc - per-binder proc state for binder allocator
> - * @vma:                vm_area_struct passed to mmap_handler
> + * @mutex:              protects binder_alloc fields
> + * @vma_addr:           vm_area_struct->vm_start passed to mmap_handler
>   *                      (invariant after mmap)
> - * @tsk:                tid for task that called init for this proc
> - *                      (invariant after init)
>   * @mm:                 copy of task->mm (invariant after open)
>   * @buffer:             base of per-proc address space mapped via mmap
>   * @buffers:            list of all buffers for this proc
> --
> 2.37.2.672.g94769d06f0-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH 1/7] binder: fix alloc->vma_vm_mm null-ptr dereference

[Todd Kjos]

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<kernel-team@android.com> wrote:
>
> Syzbot reported a couple issues introduced by commit 44e602b4e52f
> ("binder_alloc: add missing mmap_lock calls when using the VMA"), in
> which we attempt to acquire the mmap_lock when alloc->vma_vm_mm has not
> been initialized yet.
>
> This can happen if a binder_proc receives a transaction without having
> previously called mmap() to setup the binder_proc->alloc space in [1].
> Also, a similar issue occurs via binder_alloc_print_pages() when we try
> to dump the debugfs binder stats file in [2].
>
> Sample of syzbot's crash report:
>   ==================================================================
>   KASAN: null-ptr-deref in range [0x0000000000000128-0x000000000000012f]
>   CPU: 0 PID: 3755 Comm: syz-executor229 Not tainted 6.0.0-rc1-next-20220819-syzkaller #0
>   syz-executor229[3755] cmdline: ./syz-executor2294415195
>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
>   RIP: 0010:__lock_acquire+0xd83/0x56d0 kernel/locking/lockdep.c:4923
>   [...]
>   Call Trace:
>    <TASK>
>    lock_acquire kernel/locking/lockdep.c:5666 [inline]
>    lock_acquire+0x1ab/0x570 kernel/locking/lockdep.c:5631
>    down_read+0x98/0x450 kernel/locking/rwsem.c:1499
>    mmap_read_lock include/linux/mmap_lock.h:117 [inline]
>    binder_alloc_new_buf_locked drivers/android/binder_alloc.c:405 [inline]
>    binder_alloc_new_buf+0xa5/0x19e0 drivers/android/binder_alloc.c:593
>    binder_transaction+0x242e/0x9a80 drivers/android/binder.c:3199
>    binder_thread_write+0x664/0x3220 drivers/android/binder.c:3986
>    binder_ioctl_write_read drivers/android/binder.c:5036 [inline]
>    binder_ioctl+0x3470/0x6d00 drivers/android/binder.c:5323
>    vfs_ioctl fs/ioctl.c:51 [inline]
>    __do_sys_ioctl fs/ioctl.c:870 [inline]
>    __se_sys_ioctl fs/ioctl.c:856 [inline]
>    __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
>    do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>    do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>    entry_SYSCALL_64_after_hwframe+0x63/0xcd
>    [...]
>   ==================================================================
>
> Fix these issues by setting up alloc->vma_vm_mm pointer during open()
> and caching directly from current->mm. This guarantees we have a valid
> reference to take the mmap_lock during scenarios described above.
>
> [1] https://syzkaller.appspot.com/bug?extid=f7dc54e5be28950ac459
> [2] https://syzkaller.appspot.com/bug?extid=a75ebe0452711c9e56d9
>
> Fixes: 44e602b4e52f ("binder_alloc: add missing mmap_lock calls when using the VMA")
> Reported-by: syzbot+f7dc54e5be28950ac459@syzkaller.appspotmail.com
> Reported-by: syzbot+a75ebe0452711c9e56d9@syzkaller.appspotmail.com
> Cc: <stable@vger.kernel.org> # v5.15+
> Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Acked-by: Todd Kjos <tkjos@google.com>

> ---
>  drivers/android/binder_alloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 51f4e1c5cd01..9b1778c00610 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -322,7 +322,6 @@ static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
>          */
>         if (vma) {
>                 vm_start = vma->vm_start;
> -               alloc->vma_vm_mm = vma->vm_mm;
>                 mmap_assert_write_locked(alloc->vma_vm_mm);
>         } else {
>                 mmap_assert_locked(alloc->vma_vm_mm);
> @@ -795,7 +794,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
>         binder_insert_free_buffer(alloc, buffer);
>         alloc->free_async_space = alloc->buffer_size / 2;
>         binder_alloc_set_vma(alloc, vma);
> -       mmgrab(alloc->vma_vm_mm);
>
>         return 0;
>
> @@ -1091,6 +1089,8 @@ static struct shrinker binder_shrinker = {
>  void binder_alloc_init(struct binder_alloc *alloc)
>  {
>         alloc->pid = current->group_leader->pid;
> +       alloc->vma_vm_mm = current->mm;
> +       mmgrab(alloc->vma_vm_mm);
>         mutex_init(&alloc->mutex);
>         INIT_LIST_HEAD(&alloc->buffers);
>  }
> --
> 2.37.2.672.g94769d06f0-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH 0/7] fix null-ptr-deref in binder_alloc and others

[Greg Kroah-Hartman]

On Mon, Aug 29, 2022 at 08:12:47PM +0000, Carlos Llamas wrote:
> This patch series fixes primarily a null dereference of alloc->vma_vm_mm
> reported by syzbot which unfortunately is quite easy to reproduce. Also,
> included here are several other patches for more trivial things I found
> along the way.

Because you had 1 bugfix, and 6 "normal" patches, I had to split this up
across branches.  Which caused some of the normal patches to fail to
apply :(

Please wait until 6.0-rc5 or so is out with the fix patch merged and
then rebase the remaining against my char-misc-next branch and resend
them.

thanks,

greg k-h