2022-08-29 20:19:10

by Carlos Llamas

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

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 <[email protected]>
Cc: "Arve Hjønnevåg" <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Martijn Coenen <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Liam Howlett <[email protected]>
Cc: [email protected]
Cc: [email protected]

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


2022-08-29 20:22:44

by Carlos Llamas

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

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

No functional changes in this patch.

Signed-off-by: Carlos Llamas <[email protected]>
---
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

2022-08-29 20:28:37

by Carlos Llamas

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

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 <[email protected]>
Signed-off-by: Carlos Llamas <[email protected]>
---
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

2022-08-29 20:32:19

by Carlos Llamas

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

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 <[email protected]>
---
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

2022-08-29 20:33:40

by Carlos Llamas

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

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 <[email protected]>
---
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

2022-08-29 20:36:43

by Carlos Llamas

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

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: [email protected]
Reported-by: [email protected]
Cc: <[email protected]> # v5.15+
Cc: Liam R. Howlett <[email protected]>
Signed-off-by: Carlos Llamas <[email protected]>
---
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

2022-08-29 20:37:03

by Carlos Llamas

[permalink] [raw]
Subject: [PATCH 7/7] binderfs: remove unused INTSTRLEN macro

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 <[email protected]>
---
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

2022-08-29 20:46:30

by Andrew Morton

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

On Mon, 29 Aug 2022 20:12:48 +0000 Carlos Llamas <[email protected]> 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: [email protected]
> Reported-by: [email protected]
> Cc: <[email protected]> # v5.15+

44e602b4e52f is only present in 6.0-rcX?


2022-08-29 20:49:30

by Carlos Llamas

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

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 <[email protected]>
---
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

2022-08-29 21:27:14

by Carlos Llamas

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

On Mon, Aug 29, 2022 at 01:34:52PM -0700, Andrew Morton wrote:
> On Mon, 29 Aug 2022 20:12:48 +0000 Carlos Llamas <[email protected]> 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: [email protected]
> > Reported-by: [email protected]
> > Cc: <[email protected]> # 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/[email protected]/
https://lore.kernel.org/all/[email protected]/

2022-08-30 08:13:41

by Christian Brauner

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

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 <[email protected]>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <[email protected]>

2022-08-30 08:15:04

by Christian Brauner

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

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 <[email protected]>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <[email protected]>

2022-08-30 08:52:23

by Christian Brauner

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

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 <[email protected]>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <[email protected]>

2022-08-30 08:52:59

by Christian Brauner

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

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 <[email protected]>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <[email protected]>

2022-08-30 09:35:13

by Christian Brauner

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

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 <[email protected]>
> ---

Thanks,
Reviewed-by: Christian Brauner (Microsoft) <[email protected]>

2022-08-30 19:38:12

by Liam R. Howlett

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

* Carlos Llamas <[email protected]> [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 <[email protected]>
> Signed-off-by: Carlos Llamas <[email protected]>
> ---
> 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
>

2022-08-30 19:39:45

by Liam R. Howlett

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

* Carlos Llamas <[email protected]> [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: [email protected]
> Reported-by: [email protected]
> Cc: <[email protected]> # v5.15+
> Cc: Liam R. Howlett <[email protected]>
> Signed-off-by: Carlos Llamas <[email protected]>
> ---
> 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
>

2022-08-30 19:44:13

by Carlos Llamas

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

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.

2022-08-30 21:09:55

by Liam R. Howlett

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

* Carlos Llamas <[email protected]> [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 <[email protected]>

2022-08-30 21:35:26

by Carlos Llamas

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

On Tue, Aug 30, 2022 at 06:57:59PM +0000, Liam Howlett wrote:
> * Carlos Llamas <[email protected]> [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.

2022-08-30 22:27:34

by Todd Kjos

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

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<[email protected]> 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 <[email protected]>

Acked-by: Todd Kjos <[email protected]>

> ---
> 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 [email protected].
>

2022-08-30 22:44:18

by Todd Kjos

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

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<[email protected]> 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 <[email protected]>

Acked-by: Todd Kjos <[email protected]>

> ---
> 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 [email protected].
>

2022-08-30 22:45:10

by Todd Kjos

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

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

Acked-by: Todd Kjos <[email protected]>

> ---
> 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 [email protected].
>

2022-08-30 22:46:03

by Todd Kjos

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

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<[email protected]> 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: [email protected]
> Reported-by: [email protected]
> Cc: <[email protected]> # v5.15+
> Cc: Liam R. Howlett <[email protected]>
> Signed-off-by: Carlos Llamas <[email protected]>

Acked-by: Todd Kjos <[email protected]>

> ---
> 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 [email protected].
>

2022-08-30 23:02:55

by Todd Kjos

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

On Mon, Aug 29, 2022 at 1:13 PM 'Carlos Llamas' via kernel-team
<[email protected]> 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 <[email protected]>

Acked-by: Todd Kjos <[email protected]>

> ---
> 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 [email protected].
>

2022-09-01 14:46:33

by Greg Kroah-Hartman

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

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