From bdc1c5fac982845a58d28690cdb56db8c88a530d Mon Sep 17 00:00:00 2001 From: Carlos Llamas Date: Fri, 5 May 2023 20:30:20 +0000 Subject: binder: fix UAF caused by faulty buffer cleanup 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 Cc: stable@vger.kernel.org Signed-off-by: Carlos Llamas Acked-by: Todd Kjos Link: https://lore.kernel.org/r/20230505203020.4101154-1-cmllamas@google.com Signed-off-by: Greg Kroah-Hartman --- drivers/android/binder.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) (limited to 'drivers/android/binder.c') 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); } -- cgit v1.2.3 From 800936191a26a5aba5caa3cbd70a4154b45eb94a Mon Sep 17 00:00:00 2001 From: Chuang Zhang Date: Mon, 24 Apr 2023 19:05:14 +0800 Subject: Binder: Add timestamp to transaction record This patch adds a timestamp field to the binder_transaction structure to track the time consumed during transmission when reading binder_transaction records. Signed-off-by: Chuang Zhang Acked-by: Carlos Llamas Link: https://lore.kernel.org/r/5ac8c0d09392290be789423f0dd78a520b830fab.1682333709.git.zhangchuang3@xiaomi.com Signed-off-by: Greg Kroah-Hartman --- drivers/android/binder.c | 9 +++++++-- drivers/android/binder_internal.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) (limited to 'drivers/android/binder.c') diff --git a/drivers/android/binder.c b/drivers/android/binder.c index fb56bfc45096..b6413652906e 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -66,6 +66,7 @@ #include #include #include +#include #include @@ -2904,6 +2905,7 @@ static void binder_transaction(struct binder_proc *proc, binder_size_t last_fixup_min_off = 0; struct binder_context *context = proc->context; int t_debug_id = atomic_inc_return(&binder_last_id); + ktime_t t_start_time = ktime_get(); char *secctx = NULL; u32 secctx_sz = 0; struct list_head sgc_head; @@ -3145,6 +3147,7 @@ static void binder_transaction(struct binder_proc *proc, binder_stats_created(BINDER_STAT_TRANSACTION_COMPLETE); t->debug_id = t_debug_id; + t->start_time = t_start_time; if (reply) binder_debug(BINDER_DEBUG_TRANSACTION, @@ -5930,17 +5933,19 @@ static void print_binder_transaction_ilocked(struct seq_file *m, { struct binder_proc *to_proc; struct binder_buffer *buffer = t->buffer; + ktime_t current_time = ktime_get(); spin_lock(&t->lock); to_proc = t->to_proc; seq_printf(m, - "%s %d: %pK from %d:%d to %d:%d code %x flags %x pri %ld r%d", + "%s %d: %pK from %d:%d to %d:%d code %x flags %x pri %ld r%d elapsed %lldms", prefix, t->debug_id, t, t->from ? t->from->proc->pid : 0, t->from ? t->from->pid : 0, to_proc ? to_proc->pid : 0, t->to_thread ? t->to_thread->pid : 0, - t->code, t->flags, t->priority, t->need_reply); + t->code, t->flags, t->priority, t->need_reply, + ktime_ms_delta(current_time, t->start_time)); spin_unlock(&t->lock); if (proc != to_proc) { diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h index 28ef5b3704b1..92e64007f2b0 100644 --- a/drivers/android/binder_internal.h +++ b/drivers/android/binder_internal.h @@ -528,6 +528,7 @@ struct binder_transaction { long priority; long saved_priority; kuid_t sender_euid; + ktime_t start_time; struct list_head fd_fixups; binder_uintptr_t security_ctx; /** -- cgit v1.2.3 From c21c0f9a20a963f5a1874657a4e3d657503f7815 Mon Sep 17 00:00:00 2001 From: Chuang Zhang Date: Mon, 24 Apr 2023 19:05:15 +0800 Subject: Binder: Add async from to transaction record This commit adds support for getting the pid and tid information of the sender for asynchronous transfers in binderfs transfer records. In previous versions, it was not possible to obtain this information from the transfer records. While this information may not be necessary for all use cases, it can be useful in some scenarios. Signed-off-by: Chuang Zhang Acked-by: Carlos Llamas Link: https://lore.kernel.org/r/0c1e8bd37c68dd1518bb737b06b768cde9659386.1682333709.git.zhangchuang3@xiaomi.com Signed-off-by: Greg Kroah-Hartman --- drivers/android/binder.c | 6 ++++-- drivers/android/binder_internal.h | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 'drivers/android/binder.c') diff --git a/drivers/android/binder.c b/drivers/android/binder.c index b6413652906e..6674619845e0 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -3172,6 +3172,8 @@ static void binder_transaction(struct binder_proc *proc, t->from = thread; else t->from = NULL; + t->from_pid = proc->pid; + t->from_tid = thread->pid; t->sender_euid = task_euid(proc->tsk); t->to_proc = target_proc; t->to_thread = target_thread; @@ -5940,8 +5942,8 @@ static void print_binder_transaction_ilocked(struct seq_file *m, seq_printf(m, "%s %d: %pK from %d:%d to %d:%d code %x flags %x pri %ld r%d elapsed %lldms", prefix, t->debug_id, t, - t->from ? t->from->proc->pid : 0, - t->from ? t->from->pid : 0, + t->from_pid, + t->from_tid, to_proc ? to_proc->pid : 0, t->to_thread ? t->to_thread->pid : 0, t->code, t->flags, t->priority, t->need_reply, diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h index 92e64007f2b0..7270d4d22207 100644 --- a/drivers/android/binder_internal.h +++ b/drivers/android/binder_internal.h @@ -515,6 +515,8 @@ struct binder_transaction { int debug_id; struct binder_work work; struct binder_thread *from; + pid_t from_pid; + pid_t from_tid; struct binder_transaction *from_parent; struct binder_proc *to_proc; struct binder_thread *to_thread; -- cgit v1.2.3