[PATCH] binder: fix UAF caused by faulty buffer cleanup

[PATCH] binder: fix UAF caused by faulty buffer cleanup

[Carlos Llamas]

In binder_transaction_buffer_release() the 'failed_at' offset indicates
the number of objects to clean up. However, this function was changed by
commit 44d8047f1d87 ("binder: use standard functions to allocate fds"),
to release all the objects in the buffer when 'failed_at' is zero.

This introduced an issue when a transaction buffer is released without
any objects having been processed so far. In this case, 'failed_at' is
indeed zero yet it is misinterpreted as releasing the entire buffer.

This leads to use-after-free errors where nodes are incorrectly freed
and subsequently accessed. Such is the case in the following KASAN
report:

  ==================================================================
  BUG: KASAN: slab-use-after-free in binder_thread_read+0xc40/0x1f30
  Read of size 8 at addr ffff4faf037cfc58 by task poc/474

  CPU: 6 PID: 474 Comm: poc Not tainted 6.3.0-12570-g7df047b3f0aa #5
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   dump_backtrace+0x94/0xec
   show_stack+0x18/0x24
   dump_stack_lvl+0x48/0x60
   print_report+0xf8/0x5b8
   kasan_report+0xb8/0xfc
   __asan_load8+0x9c/0xb8
   binder_thread_read+0xc40/0x1f30
   binder_ioctl+0xd9c/0x1768
   __arm64_sys_ioctl+0xd4/0x118
   invoke_syscall+0x60/0x188
  [...]

  Allocated by task 474:
   kasan_save_stack+0x3c/0x64
   kasan_set_track+0x2c/0x40
   kasan_save_alloc_info+0x24/0x34
   __kasan_kmalloc+0xb8/0xbc
   kmalloc_trace+0x48/0x5c
   binder_new_node+0x3c/0x3a4
   binder_transaction+0x2b58/0x36f0
   binder_thread_write+0x8e0/0x1b78
   binder_ioctl+0x14a0/0x1768
   __arm64_sys_ioctl+0xd4/0x118
   invoke_syscall+0x60/0x188
  [...]

  Freed by task 475:
   kasan_save_stack+0x3c/0x64
   kasan_set_track+0x2c/0x40
   kasan_save_free_info+0x38/0x5c
   __kasan_slab_free+0xe8/0x154
   __kmem_cache_free+0x128/0x2bc
   kfree+0x58/0x70
   binder_dec_node_tmpref+0x178/0x1fc
   binder_transaction_buffer_release+0x430/0x628
   binder_transaction+0x1954/0x36f0
   binder_thread_write+0x8e0/0x1b78
   binder_ioctl+0x14a0/0x1768
   __arm64_sys_ioctl+0xd4/0x118
   invoke_syscall+0x60/0x188
  [...]
  ==================================================================

In order to avoid these issues, let's always calculate the intended
'failed_at' offset beforehand. This is wrapped in a helper function to
make it clear and convenient.

Fixes: 32e9f56a96d8 ("binder: don't detect sender/target during buffer cleanup")
Reported-by: Zi Fan Tan <zifantan@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index fb56bfc45096..6678a862ea84 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1938,7 +1938,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 					      bool is_failure)
 {
 	int debug_id = buffer->debug_id;
-	binder_size_t off_start_offset, buffer_offset, off_end_offset;
+	binder_size_t off_start_offset, buffer_offset;
 
 	binder_debug(BINDER_DEBUG_TRANSACTION,
 		     "%d buffer release %d, size %zd-%zd, failed at %llx\n",
@@ -1950,9 +1950,8 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 		binder_dec_node(buffer->target_node, 1, 0);
 
 	off_start_offset = ALIGN(buffer->data_size, sizeof(void *));
-	off_end_offset = is_failure && failed_at ? failed_at :
-				off_start_offset + buffer->offsets_size;
-	for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
+
+	for (buffer_offset = off_start_offset; buffer_offset < failed_at;
 	     buffer_offset += sizeof(binder_size_t)) {
 		struct binder_object_header *hdr;
 		size_t object_size = 0;
@@ -2111,6 +2110,25 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 	}
 }
 
+/* Clean up all the objects in the buffer */
+static inline void binder_release_entire_buffer(struct binder_proc *proc,
+						struct binder_thread *thread,
+						struct binder_buffer *buffer,
+						bool is_failure)
+{
+	binder_size_t off_end_offset;
+
+	off_end_offset = ALIGN(buffer->data_size, sizeof(void *));
+	off_end_offset += buffer->offsets_size;
+
+	/* We always pass the end of the buffer here to make sure that
+	 * binder_transaction_buffer_release() loops through all the
+	 * objects in the buffer.
+	 */
+	binder_transaction_buffer_release(proc, thread, buffer,
+					  off_end_offset, is_failure);
+}
+
 static int binder_translate_binder(struct flat_binder_object *fp,
 				   struct binder_transaction *t,
 				   struct binder_thread *thread)
@@ -2806,7 +2824,7 @@ static int binder_proc_transaction(struct binder_transaction *t,
 		t_outdated->buffer = NULL;
 		buffer->transaction = NULL;
 		trace_binder_transaction_update_buffer_release(buffer);
-		binder_transaction_buffer_release(proc, NULL, buffer, 0, 0);
+		binder_release_entire_buffer(proc, NULL, buffer, false);
 		binder_alloc_free_buf(&proc->alloc, buffer);
 		kfree(t_outdated);
 		binder_stats_deleted(BINDER_STAT_TRANSACTION);
@@ -3775,7 +3793,7 @@ binder_free_buf(struct binder_proc *proc,
 		binder_node_inner_unlock(buf_node);
 	}
 	trace_binder_transaction_buffer_release(buffer);
-	binder_transaction_buffer_release(proc, thread, buffer, 0, is_failure);
+	binder_release_entire_buffer(proc, thread, buffer, is_failure);
 	binder_alloc_free_buf(&proc->alloc, buffer);
 }
 
-- 
2.40.1.521.gf1e218fcd8-goog

[PATCH v2] binder: fix UAF caused by faulty buffer cleanup

[Carlos Llamas]

In binder_transaction_buffer_release() the 'failed_at' offset indicates
the number of objects to clean up. However, this function was changed by
commit 44d8047f1d87 ("binder: use standard functions to allocate fds"),
to release all the objects in the buffer when 'failed_at' is zero.

This introduced an issue when a transaction buffer is released without
any objects having been processed so far. In this case, 'failed_at' is
indeed zero yet it is misinterpreted as releasing the entire buffer.

This leads to use-after-free errors where nodes are incorrectly freed
and subsequently accessed. Such is the case in the following KASAN
report:

  ==================================================================
  BUG: KASAN: slab-use-after-free in binder_thread_read+0xc40/0x1f30
  Read of size 8 at addr ffff4faf037cfc58 by task poc/474

  CPU: 6 PID: 474 Comm: poc Not tainted 6.3.0-12570-g7df047b3f0aa #5
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   dump_backtrace+0x94/0xec
   show_stack+0x18/0x24
   dump_stack_lvl+0x48/0x60
   print_report+0xf8/0x5b8
   kasan_report+0xb8/0xfc
   __asan_load8+0x9c/0xb8
   binder_thread_read+0xc40/0x1f30
   binder_ioctl+0xd9c/0x1768
   __arm64_sys_ioctl+0xd4/0x118
   invoke_syscall+0x60/0x188
  [...]

  Allocated by task 474:
   kasan_save_stack+0x3c/0x64
   kasan_set_track+0x2c/0x40
   kasan_save_alloc_info+0x24/0x34
   __kasan_kmalloc+0xb8/0xbc
   kmalloc_trace+0x48/0x5c
   binder_new_node+0x3c/0x3a4
   binder_transaction+0x2b58/0x36f0
   binder_thread_write+0x8e0/0x1b78
   binder_ioctl+0x14a0/0x1768
   __arm64_sys_ioctl+0xd4/0x118
   invoke_syscall+0x60/0x188
  [...]

  Freed by task 475:
   kasan_save_stack+0x3c/0x64
   kasan_set_track+0x2c/0x40
   kasan_save_free_info+0x38/0x5c
   __kasan_slab_free+0xe8/0x154
   __kmem_cache_free+0x128/0x2bc
   kfree+0x58/0x70
   binder_dec_node_tmpref+0x178/0x1fc
   binder_transaction_buffer_release+0x430/0x628
   binder_transaction+0x1954/0x36f0
   binder_thread_write+0x8e0/0x1b78
   binder_ioctl+0x14a0/0x1768
   __arm64_sys_ioctl+0xd4/0x118
   invoke_syscall+0x60/0x188
  [...]
  ==================================================================

In order to avoid these issues, let's always calculate the intended
'failed_at' offset beforehand. This is renamed and wrapped in a helper
function to make it clear and convenient.

Fixes: 32e9f56a96d8 ("binder: don't detect sender/target during buffer cleanup")
Reported-by: Zi Fan Tan <zifantan@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
v2: rename 'failed_at' to 'off_end_offsets' and drop the now unecessary
    comments after the rename per Todd's feedback.

 drivers/android/binder.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index fb56bfc45096..8fb7672021ee 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1934,24 +1934,23 @@ static void binder_deferred_fd_close(int fd)
 static void binder_transaction_buffer_release(struct binder_proc *proc,
 					      struct binder_thread *thread,
 					      struct binder_buffer *buffer,
-					      binder_size_t failed_at,
+					      binder_size_t off_end_offset,
 					      bool is_failure)
 {
 	int debug_id = buffer->debug_id;
-	binder_size_t off_start_offset, buffer_offset, off_end_offset;
+	binder_size_t off_start_offset, buffer_offset;
 
 	binder_debug(BINDER_DEBUG_TRANSACTION,
 		     "%d buffer release %d, size %zd-%zd, failed at %llx\n",
 		     proc->pid, buffer->debug_id,
 		     buffer->data_size, buffer->offsets_size,
-		     (unsigned long long)failed_at);
+		     (unsigned long long)off_end_offset);
 
 	if (buffer->target_node)
 		binder_dec_node(buffer->target_node, 1, 0);
 
 	off_start_offset = ALIGN(buffer->data_size, sizeof(void *));
-	off_end_offset = is_failure && failed_at ? failed_at :
-				off_start_offset + buffer->offsets_size;
+
 	for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
 	     buffer_offset += sizeof(binder_size_t)) {
 		struct binder_object_header *hdr;
@@ -2111,6 +2110,21 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 	}
 }
 
+/* Clean up all the objects in the buffer */
+static inline void binder_release_entire_buffer(struct binder_proc *proc,
+						struct binder_thread *thread,
+						struct binder_buffer *buffer,
+						bool is_failure)
+{
+	binder_size_t off_end_offset;
+
+	off_end_offset = ALIGN(buffer->data_size, sizeof(void *));
+	off_end_offset += buffer->offsets_size;
+
+	binder_transaction_buffer_release(proc, thread, buffer,
+					  off_end_offset, is_failure);
+}
+
 static int binder_translate_binder(struct flat_binder_object *fp,
 				   struct binder_transaction *t,
 				   struct binder_thread *thread)
@@ -2806,7 +2820,7 @@ static int binder_proc_transaction(struct binder_transaction *t,
 		t_outdated->buffer = NULL;
 		buffer->transaction = NULL;
 		trace_binder_transaction_update_buffer_release(buffer);
-		binder_transaction_buffer_release(proc, NULL, buffer, 0, 0);
+		binder_release_entire_buffer(proc, NULL, buffer, false);
 		binder_alloc_free_buf(&proc->alloc, buffer);
 		kfree(t_outdated);
 		binder_stats_deleted(BINDER_STAT_TRANSACTION);
@@ -3775,7 +3789,7 @@ binder_free_buf(struct binder_proc *proc,
 		binder_node_inner_unlock(buf_node);
 	}
 	trace_binder_transaction_buffer_release(buffer);
-	binder_transaction_buffer_release(proc, thread, buffer, 0, is_failure);
+	binder_release_entire_buffer(proc, thread, buffer, is_failure);
 	binder_alloc_free_buf(&proc->alloc, buffer);
 }
 
-- 
2.40.1.521.gf1e218fcd8-goog

Re: [PATCH v2] binder: fix UAF caused by faulty buffer cleanup

[Todd Kjos]

On Fri, May 5, 2023 at 1:30 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> In binder_transaction_buffer_release() the 'failed_at' offset indicates
> the number of objects to clean up. However, this function was changed by
> commit 44d8047f1d87 ("binder: use standard functions to allocate fds"),
> to release all the objects in the buffer when 'failed_at' is zero.
>
> This introduced an issue when a transaction buffer is released without
> any objects having been processed so far. In this case, 'failed_at' is
> indeed zero yet it is misinterpreted as releasing the entire buffer.
>
> This leads to use-after-free errors where nodes are incorrectly freed
> and subsequently accessed. Such is the case in the following KASAN
> report:
>
>   ==================================================================
>   BUG: KASAN: slab-use-after-free in binder_thread_read+0xc40/0x1f30
>   Read of size 8 at addr ffff4faf037cfc58 by task poc/474
>
>   CPU: 6 PID: 474 Comm: poc Not tainted 6.3.0-12570-g7df047b3f0aa #5
>   Hardware name: linux,dummy-virt (DT)
>   Call trace:
>    dump_backtrace+0x94/0xec
>    show_stack+0x18/0x24
>    dump_stack_lvl+0x48/0x60
>    print_report+0xf8/0x5b8
>    kasan_report+0xb8/0xfc
>    __asan_load8+0x9c/0xb8
>    binder_thread_read+0xc40/0x1f30
>    binder_ioctl+0xd9c/0x1768
>    __arm64_sys_ioctl+0xd4/0x118
>    invoke_syscall+0x60/0x188
>   [...]
>
>   Allocated by task 474:
>    kasan_save_stack+0x3c/0x64
>    kasan_set_track+0x2c/0x40
>    kasan_save_alloc_info+0x24/0x34
>    __kasan_kmalloc+0xb8/0xbc
>    kmalloc_trace+0x48/0x5c
>    binder_new_node+0x3c/0x3a4
>    binder_transaction+0x2b58/0x36f0
>    binder_thread_write+0x8e0/0x1b78
>    binder_ioctl+0x14a0/0x1768
>    __arm64_sys_ioctl+0xd4/0x118
>    invoke_syscall+0x60/0x188
>   [...]
>
>   Freed by task 475:
>    kasan_save_stack+0x3c/0x64
>    kasan_set_track+0x2c/0x40
>    kasan_save_free_info+0x38/0x5c
>    __kasan_slab_free+0xe8/0x154
>    __kmem_cache_free+0x128/0x2bc
>    kfree+0x58/0x70
>    binder_dec_node_tmpref+0x178/0x1fc
>    binder_transaction_buffer_release+0x430/0x628
>    binder_transaction+0x1954/0x36f0
>    binder_thread_write+0x8e0/0x1b78
>    binder_ioctl+0x14a0/0x1768
>    __arm64_sys_ioctl+0xd4/0x118
>    invoke_syscall+0x60/0x188
>   [...]
>   ==================================================================
>
> In order to avoid these issues, let's always calculate the intended
> 'failed_at' offset beforehand. This is renamed and wrapped in a helper
> function to make it clear and convenient.
>
> Fixes: 32e9f56a96d8 ("binder: don't detect sender/target during buffer cleanup")
> Reported-by: Zi Fan Tan <zifantan@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
> v2: rename 'failed_at' to 'off_end_offsets' and drop the now unecessary
>     comments after the rename per Todd's feedback.
>
>  drivers/android/binder.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index fb56bfc45096..8fb7672021ee 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1934,24 +1934,23 @@ static void binder_deferred_fd_close(int fd)
>  static void binder_transaction_buffer_release(struct binder_proc *proc,
>                                               struct binder_thread *thread,
>                                               struct binder_buffer *buffer,
> -                                             binder_size_t failed_at,
> +                                             binder_size_t off_end_offset,
>                                               bool is_failure)
>  {
>         int debug_id = buffer->debug_id;
> -       binder_size_t off_start_offset, buffer_offset, off_end_offset;
> +       binder_size_t off_start_offset, buffer_offset;
>
>         binder_debug(BINDER_DEBUG_TRANSACTION,
>                      "%d buffer release %d, size %zd-%zd, failed at %llx\n",
>                      proc->pid, buffer->debug_id,
>                      buffer->data_size, buffer->offsets_size,
> -                    (unsigned long long)failed_at);
> +                    (unsigned long long)off_end_offset);
>
>         if (buffer->target_node)
>                 binder_dec_node(buffer->target_node, 1, 0);
>
>         off_start_offset = ALIGN(buffer->data_size, sizeof(void *));
> -       off_end_offset = is_failure && failed_at ? failed_at :
> -                               off_start_offset + buffer->offsets_size;
> +
>         for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
>              buffer_offset += sizeof(binder_size_t)) {
>                 struct binder_object_header *hdr;
> @@ -2111,6 +2110,21 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
>         }
>  }
>
> +/* Clean up all the objects in the buffer */
> +static inline void binder_release_entire_buffer(struct binder_proc *proc,
> +                                               struct binder_thread *thread,
> +                                               struct binder_buffer *buffer,
> +                                               bool is_failure)
> +{
> +       binder_size_t off_end_offset;
> +
> +       off_end_offset = ALIGN(buffer->data_size, sizeof(void *));
> +       off_end_offset += buffer->offsets_size;
> +
> +       binder_transaction_buffer_release(proc, thread, buffer,
> +                                         off_end_offset, is_failure);
> +}
> +
>  static int binder_translate_binder(struct flat_binder_object *fp,
>                                    struct binder_transaction *t,
>                                    struct binder_thread *thread)
> @@ -2806,7 +2820,7 @@ static int binder_proc_transaction(struct binder_transaction *t,
>                 t_outdated->buffer = NULL;
>                 buffer->transaction = NULL;
>                 trace_binder_transaction_update_buffer_release(buffer);
> -               binder_transaction_buffer_release(proc, NULL, buffer, 0, 0);
> +               binder_release_entire_buffer(proc, NULL, buffer, false);
>                 binder_alloc_free_buf(&proc->alloc, buffer);
>                 kfree(t_outdated);
>                 binder_stats_deleted(BINDER_STAT_TRANSACTION);
> @@ -3775,7 +3789,7 @@ binder_free_buf(struct binder_proc *proc,
>                 binder_node_inner_unlock(buf_node);
>         }
>         trace_binder_transaction_buffer_release(buffer);
> -       binder_transaction_buffer_release(proc, thread, buffer, 0, is_failure);
> +       binder_release_entire_buffer(proc, thread, buffer, is_failure);
>         binder_alloc_free_buf(&proc->alloc, buffer);
>  }
>
> --
> 2.40.1.521.gf1e218fcd8-goog
>