From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v4.20-rc1 commit 44d8047f1d87adc2fd7eccc88533794f6d88c15e category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
Binder uses internal fs interfaces to allocate and install fds:
__alloc_fd __fd_install __close_fd get_files_struct put_files_struct
These were used to support the passing of fds between processes as part of a transaction. The actual allocation and installation of the fds in the target process was handled by the sending process so the standard functions, alloc_fd() and fd_install() which assume task==current couldn't be used.
This patch refactors this mechanism so that the fds are allocated and installed by the target process allowing the standard functions to be used.
The sender now creates a list of fd fixups that contains the struct *file and the address to fixup with the new fd once it is allocated. This list is processed by the target process when the transaction is dequeued.
A new error case is introduced by this change. If an async transaction with file descriptors cannot allocate new fds in the target (probably due to out of file descriptors), the transaction is discarded with a log message. In the old implementation this would have been detected in the sender context and failed prior to sending.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Conflicts: drivers/android/binder.c Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/Kconfig | 2 +- drivers/android/binder.c | 391 ++++++++++++++++++++------------- drivers/android/binder_trace.h | 36 ++- 3 files changed, 262 insertions(+), 167 deletions(-)
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig index 432e9ad77070..51e8250d113f 100644 --- a/drivers/android/Kconfig +++ b/drivers/android/Kconfig @@ -10,7 +10,7 @@ if ANDROID
config ANDROID_BINDER_IPC bool "Android Binder IPC Driver" - depends on MMU + depends on MMU && !CPU_CACHE_VIVT default n ---help--- Binder is used in Android for both communication between processes, diff --git a/drivers/android/binder.c b/drivers/android/binder.c index c5a39b8ae886..8a72aacd7a39 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -71,6 +71,7 @@ #include <linux/security.h> #include <linux/spinlock.h> #include <linux/ratelimit.h> +#include <linux/syscalls.h>
#include <uapi/linux/android/binder.h>
@@ -457,9 +458,8 @@ struct binder_ref { };
enum binder_deferred_state { - BINDER_DEFERRED_PUT_FILES = 0x01, - BINDER_DEFERRED_FLUSH = 0x02, - BINDER_DEFERRED_RELEASE = 0x04, + BINDER_DEFERRED_FLUSH = 0x01, + BINDER_DEFERRED_RELEASE = 0x02, };
/** @@ -480,9 +480,6 @@ enum binder_deferred_state { * (invariant after initialized) * @tsk task_struct for group_leader of process * (invariant after initialized) - * @files files_struct for process - * (protected by @files_lock) - * @files_lock mutex to protect @files * @cred struct cred associated with the `struct file` * in binder_open() * (invariant after initialized) @@ -530,8 +527,6 @@ struct binder_proc { struct list_head waiting_threads; int pid; struct task_struct *tsk; - struct files_struct *files; - struct mutex files_lock; const struct cred *cred; struct hlist_node deferred_work_node; int deferred_work; @@ -615,6 +610,23 @@ struct binder_thread { bool is_dead; };
+/** + * struct binder_txn_fd_fixup - transaction fd fixup list element + * @fixup_entry: list entry + * @file: struct file to be associated with new fd + * @offset: offset in buffer data to this fixup + * + * List element for fd fixups in a transaction. Since file + * descriptors need to be allocated in the context of the + * target process, we pass each fd to be processed in this + * struct. + */ +struct binder_txn_fd_fixup { + struct list_head fixup_entry; + struct file *file; + size_t offset; +}; + struct binder_transaction { int debug_id; struct binder_work work; @@ -632,6 +644,7 @@ struct binder_transaction { long priority; long saved_priority; kuid_t sender_euid; + struct list_head fd_fixups; /** * @lock: protects @from, @to_proc, and @to_thread * @@ -905,66 +918,6 @@ static void binder_free_thread(struct binder_thread *thread); static void binder_free_proc(struct binder_proc *proc); static void binder_inc_node_tmpref_ilocked(struct binder_node *node);
-static int task_get_unused_fd_flags(struct binder_proc *proc, int flags) -{ - unsigned long rlim_cur; - unsigned long irqs; - int ret; - - mutex_lock(&proc->files_lock); - if (proc->files == NULL) { - ret = -ESRCH; - goto err; - } - if (!lock_task_sighand(proc->tsk, &irqs)) { - ret = -EMFILE; - goto err; - } - rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE); - unlock_task_sighand(proc->tsk, &irqs); - - ret = __alloc_fd(proc->files, 0, rlim_cur, flags); -err: - mutex_unlock(&proc->files_lock); - return ret; -} - -/* - * copied from fd_install - */ -static void task_fd_install( - struct binder_proc *proc, unsigned int fd, struct file *file) -{ - mutex_lock(&proc->files_lock); - if (proc->files) - __fd_install(proc->files, fd, file); - mutex_unlock(&proc->files_lock); -} - -/* - * copied from sys_close - */ -static long task_close_fd(struct binder_proc *proc, unsigned int fd) -{ - int retval; - - mutex_lock(&proc->files_lock); - if (proc->files == NULL) { - retval = -ESRCH; - goto err; - } - retval = __close_fd(proc->files, fd); - /* can't restart close syscall because file table entry was cleared */ - if (unlikely(retval == -ERESTARTSYS || - retval == -ERESTARTNOINTR || - retval == -ERESTARTNOHAND || - retval == -ERESTART_RESTARTBLOCK)) - retval = -EINTR; -err: - mutex_unlock(&proc->files_lock); - return retval; -} - static bool binder_has_work_ilocked(struct binder_thread *thread, bool do_proc_work) { @@ -1948,6 +1901,27 @@ static struct binder_thread *binder_get_txn_from_and_acq_inner( return NULL; }
+/** + * binder_free_txn_fixups() - free unprocessed fd fixups + * @t: binder transaction for t->from + * + * If the transaction is being torn down prior to being + * processed by the target process, free all of the + * fd fixups and fput the file structs. It is safe to + * call this function after the fixups have been + * processed -- in that case, the list will be empty. + */ +static void binder_free_txn_fixups(struct binder_transaction *t) +{ + struct binder_txn_fd_fixup *fixup, *tmp; + + list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) { + fput(fixup->file); + list_del(&fixup->fixup_entry); + kfree(fixup); + } +} + static void binder_free_transaction(struct binder_transaction *t) { struct binder_proc *target_proc = t->to_proc; @@ -1962,6 +1936,7 @@ static void binder_free_transaction(struct binder_transaction *t) * If the transaction has no target_proc, then * t->buffer->transaction has already been cleared. */ + binder_free_txn_fixups(t); kfree(t); binder_stats_deleted(BINDER_STAT_TRANSACTION); } @@ -2262,12 +2237,17 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, } break;
case BINDER_TYPE_FD: { - struct binder_fd_object *fp = to_binder_fd_object(hdr); - - binder_debug(BINDER_DEBUG_TRANSACTION, - " fd %d\n", fp->fd); - if (failed_at) - task_close_fd(proc, fp->fd); + /* + * No need to close the file here since user-space + * closes it for for successfully delivered + * transactions. For transactions that weren't + * delivered, the new fd was never allocated so + * there is no need to close and the fput on the + * file is done when the transaction is torn + * down. + */ + WARN_ON(failed_at && + proc->tsk == current->group_leader); } break; case BINDER_TYPE_PTR: /* @@ -2283,6 +2263,15 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, size_t fd_index; binder_size_t fd_buf_size;
+ if (proc->tsk != current->group_leader) { + /* + * Nothing to do if running in sender context + * The fd fixups have not been applied so no + * fds need to be closed. + */ + continue; + } + fda = to_binder_fd_array_object(hdr); parent = binder_validate_ptr(buffer, fda->parent, off_start, @@ -2315,7 +2304,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, } fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset); for (fd_index = 0; fd_index < fda->num_fds; fd_index++) - task_close_fd(proc, fd_array[fd_index]); + ksys_close(fd_array[fd_index]); } break; default: pr_err("transaction release %d bad object type %x\n", @@ -2447,17 +2436,18 @@ static int binder_translate_handle(struct flat_binder_object *fp, return ret; }
-static int binder_translate_fd(int fd, +static int binder_translate_fd(u32 *fdp, struct binder_transaction *t, struct binder_thread *thread, struct binder_transaction *in_reply_to) { struct binder_proc *proc = thread->proc; struct binder_proc *target_proc = t->to_proc; - int target_fd; + struct binder_txn_fd_fixup *fixup; struct file *file; - int ret; + int ret = 0; bool target_allows_fd; + int fd = *fdp;
if (in_reply_to) target_allows_fd = !!(in_reply_to->flags & TF_ACCEPT_FDS); @@ -2485,19 +2475,24 @@ static int binder_translate_fd(int fd, goto err_security; }
- target_fd = task_get_unused_fd_flags(target_proc, O_CLOEXEC); - if (target_fd < 0) { + /* + * Add fixup record for this transaction. The allocation + * of the fd in the target needs to be done from a + * target thread. + */ + fixup = kzalloc(sizeof(*fixup), GFP_KERNEL); + if (!fixup) { ret = -ENOMEM; - goto err_get_unused_fd; + goto err_alloc; } - task_fd_install(target_proc, target_fd, file); - trace_binder_transaction_fd(t, fd, target_fd); - binder_debug(BINDER_DEBUG_TRANSACTION, " fd %d -> %d\n", - fd, target_fd); + fixup->file = file; + fixup->offset = (uintptr_t)fdp - (uintptr_t)t->buffer->data; + trace_binder_transaction_fd_send(t, fd, fixup->offset); + list_add_tail(&fixup->fixup_entry, &t->fd_fixups);
- return target_fd; + return ret;
-err_get_unused_fd: +err_alloc: err_security: fput(file); err_fget: @@ -2511,8 +2506,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, struct binder_thread *thread, struct binder_transaction *in_reply_to) { - binder_size_t fdi, fd_buf_size, num_installed_fds; - int target_fd; + binder_size_t fdi, fd_buf_size; uintptr_t parent_buffer; u32 *fd_array; struct binder_proc *proc = thread->proc; @@ -2544,23 +2538,12 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, return -EINVAL; } for (fdi = 0; fdi < fda->num_fds; fdi++) { - target_fd = binder_translate_fd(fd_array[fdi], t, thread, + int ret = binder_translate_fd(&fd_array[fdi], t, thread, in_reply_to); - if (target_fd < 0) - goto err_translate_fd_failed; - fd_array[fdi] = target_fd; + if (ret < 0) + return ret; } return 0; - -err_translate_fd_failed: - /* - * Failed to allocate fd or security error, free fds - * installed so far. - */ - num_installed_fds = fdi; - for (fdi = 0; fdi < num_installed_fds; fdi++) - task_close_fd(target_proc, fd_array[fdi]); - return target_fd; }
static int binder_fixup_parent(struct binder_transaction *t, @@ -2935,6 +2918,7 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_alloc_t_failed; } + INIT_LIST_HEAD(&t->fd_fixups); binder_stats_created(BINDER_STAT_TRANSACTION); spin_lock_init(&t->lock);
@@ -3089,17 +3073,16 @@ static void binder_transaction(struct binder_proc *proc,
case BINDER_TYPE_FD: { struct binder_fd_object *fp = to_binder_fd_object(hdr); - int target_fd = binder_translate_fd(fp->fd, t, thread, - in_reply_to); + int ret = binder_translate_fd(&fp->fd, t, thread, + in_reply_to);
- if (target_fd < 0) { + if (ret < 0) { return_error = BR_FAILED_REPLY; - return_error_param = target_fd; + return_error_param = ret; return_error_line = __LINE__; goto err_translate_failed; } fp->pad_binder = 0; - fp->fd = target_fd; } break; case BINDER_TYPE_FDA: { struct binder_fd_array_object *fda = @@ -3256,6 +3239,7 @@ static void binder_transaction(struct binder_proc *proc, err_bad_offset: err_bad_parent: err_copy_data_failed: + binder_free_txn_fixups(t); trace_binder_transaction_failed_buffer_release(t->buffer); binder_transaction_buffer_release(target_proc, t->buffer, offp); if (target_node) @@ -3318,6 +3302,49 @@ static void binder_transaction(struct binder_proc *proc, } }
+/** + * binder_free_buf() - free the specified buffer + * @proc: binder proc that owns buffer + * @buffer: buffer to be freed + * + * If buffer for an async transaction, enqueue the next async + * transaction from the node. + * + * Cleanup buffer and free it. + */ +void +binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer) +{ + binder_inner_proc_lock(proc); + if (buffer->transaction) { + buffer->transaction->buffer = NULL; + buffer->transaction = NULL; + } + binder_inner_proc_unlock(proc); + if (buffer->async_transaction && buffer->target_node) { + struct binder_node *buf_node; + struct binder_work *w; + + buf_node = buffer->target_node; + binder_node_inner_lock(buf_node); + BUG_ON(!buf_node->has_async_transaction); + BUG_ON(buf_node->proc != proc); + w = binder_dequeue_work_head_ilocked( + &buf_node->async_todo); + if (!w) { + buf_node->has_async_transaction = false; + } else { + binder_enqueue_work_ilocked( + w, &proc->todo); + binder_wakeup_proc_ilocked(proc); + } + binder_node_inner_unlock(buf_node); + } + trace_binder_transaction_buffer_release(buffer); + binder_transaction_buffer_release(proc, buffer, NULL); + binder_alloc_free_buf(&proc->alloc, buffer); +} + static int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread, binder_uintptr_t binder_buffer, size_t size, @@ -3508,35 +3535,7 @@ static int binder_thread_write(struct binder_proc *proc, proc->pid, thread->pid, (u64)data_ptr, buffer->debug_id, buffer->transaction ? "active" : "finished"); - - binder_inner_proc_lock(proc); - if (buffer->transaction) { - buffer->transaction->buffer = NULL; - buffer->transaction = NULL; - } - binder_inner_proc_unlock(proc); - if (buffer->async_transaction && buffer->target_node) { - struct binder_node *buf_node; - struct binder_work *w; - - buf_node = buffer->target_node; - binder_node_inner_lock(buf_node); - BUG_ON(!buf_node->has_async_transaction); - BUG_ON(buf_node->proc != proc); - w = binder_dequeue_work_head_ilocked( - &buf_node->async_todo); - if (!w) { - buf_node->has_async_transaction = false; - } else { - binder_enqueue_work_ilocked( - w, &proc->todo); - binder_wakeup_proc_ilocked(proc); - } - binder_node_inner_unlock(buf_node); - } - trace_binder_transaction_buffer_release(buffer); - binder_transaction_buffer_release(proc, buffer, NULL); - binder_alloc_free_buf(&proc->alloc, buffer); + binder_free_buf(proc, buffer); break; }
@@ -3859,6 +3858,76 @@ static int binder_wait_for_work(struct binder_thread *thread, return ret; }
+/** + * binder_apply_fd_fixups() - finish fd translation + * @t: binder transaction with list of fd fixups + * + * Now that we are in the context of the transaction target + * process, we can allocate and install fds. Process the + * list of fds to translate and fixup the buffer with the + * new fds. + * + * If we fail to allocate an fd, then free the resources by + * fput'ing files that have not been processed and ksys_close'ing + * any fds that have already been allocated. + */ +static int binder_apply_fd_fixups(struct binder_transaction *t) +{ + struct binder_txn_fd_fixup *fixup, *tmp; + int ret = 0; + + list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) { + int fd = get_unused_fd_flags(O_CLOEXEC); + u32 *fdp; + + if (fd < 0) { + binder_debug(BINDER_DEBUG_TRANSACTION, + "failed fd fixup txn %d fd %d\n", + t->debug_id, fd); + ret = -ENOMEM; + break; + } + binder_debug(BINDER_DEBUG_TRANSACTION, + "fd fixup txn %d fd %d\n", + t->debug_id, fd); + trace_binder_transaction_fd_recv(t, fd, fixup->offset); + fd_install(fd, fixup->file); + fixup->file = NULL; + fdp = (u32 *)(t->buffer->data + fixup->offset); + /* + * This store can cause problems for CPUs with a + * VIVT cache (eg ARMv5) since the cache cannot + * detect virtual aliases to the same physical cacheline. + * To support VIVT, this address and the user-space VA + * would both need to be flushed. Since this kernel + * VA is not constructed via page_to_virt(), we can't + * use flush_dcache_page() on it, so we'd have to use + * an internal function. If devices with VIVT ever + * need to run Android, we'll either need to go back + * to patching the translated fd from the sender side + * (using the non-standard kernel functions), or rework + * how the kernel uses the buffer to use page_to_virt() + * addresses instead of allocating in our own vm area. + * + * For now, we disable compilation if CONFIG_CPU_CACHE_VIVT. + */ + *fdp = fd; + } + list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) { + if (fixup->file) { + fput(fixup->file); + } else if (ret) { + u32 *fdp = (u32 *)(t->buffer->data + fixup->offset); + + ksys_close(*fdp); + } + list_del(&fixup->fixup_entry); + kfree(fixup); + } + + return ret; +} + static int binder_thread_read(struct binder_proc *proc, struct binder_thread *thread, binder_uintptr_t binder_buffer, size_t size, @@ -4140,6 +4209,34 @@ static int binder_thread_read(struct binder_proc *proc, tr.sender_pid = 0; }
+ ret = binder_apply_fd_fixups(t); + if (ret) { + struct binder_buffer *buffer = t->buffer; + bool oneway = !!(t->flags & TF_ONE_WAY); + int tid = t->debug_id; + + if (t_from) + binder_thread_dec_tmpref(t_from); + buffer->transaction = NULL; + binder_cleanup_transaction(t, "fd fixups failed", + BR_FAILED_REPLY); + binder_free_buf(proc, buffer); + binder_debug(BINDER_DEBUG_FAILED_TRANSACTION, + "%d:%d %stransaction %d fd fixups failed %d/%d, line %d\n", + proc->pid, thread->pid, + oneway ? "async " : + (cmd == BR_REPLY ? "reply " : ""), + tid, BR_FAILED_REPLY, ret, __LINE__); + if (cmd == BR_REPLY) { + cmd = BR_FAILED_REPLY; + if (put_user(cmd, (uint32_t __user *)ptr)) + return -EFAULT; + ptr += sizeof(uint32_t); + binder_stat_br(proc, thread, cmd); + break; + } + continue; + } tr.data_size = t->buffer->data_size; tr.offsets_size = t->buffer->offsets_size; tr.data.ptr.buffer = (binder_uintptr_t) @@ -4730,7 +4827,6 @@ static void binder_vma_close(struct vm_area_struct *vma) (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags, (unsigned long)pgprot_val(vma->vm_page_prot)); binder_alloc_vma_close(&proc->alloc); - binder_defer_work(proc, BINDER_DEFERRED_PUT_FILES); }
static vm_fault_t binder_vm_fault(struct vm_fault *vmf) @@ -4776,9 +4872,6 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma) ret = binder_alloc_mmap_handler(&proc->alloc, vma); if (ret) return ret; - mutex_lock(&proc->files_lock); - proc->files = get_files_struct(current); - mutex_unlock(&proc->files_lock); return 0;
err_bad_arg: @@ -4802,7 +4895,6 @@ static int binder_open(struct inode *nodp, struct file *filp) spin_lock_init(&proc->outer_lock); get_task_struct(current->group_leader); proc->tsk = current->group_leader; - mutex_init(&proc->files_lock); proc->cred = get_cred(filp->f_cred); INIT_LIST_HEAD(&proc->todo); proc->default_priority = task_nice(current); @@ -4953,8 +5045,6 @@ static void binder_deferred_release(struct binder_proc *proc) struct rb_node *n; int threads, nodes, incoming_refs, outgoing_refs, active_transactions;
- BUG_ON(proc->files); - mutex_lock(&binder_procs_lock); hlist_del(&proc->proc_node); mutex_unlock(&binder_procs_lock); @@ -5036,7 +5126,6 @@ static void binder_deferred_release(struct binder_proc *proc) static void binder_deferred_func(struct work_struct *work) { struct binder_proc *proc; - struct files_struct *files;
int defer;
@@ -5054,23 +5143,11 @@ static void binder_deferred_func(struct work_struct *work) } mutex_unlock(&binder_deferred_lock);
- files = NULL; - if (defer & BINDER_DEFERRED_PUT_FILES) { - mutex_lock(&proc->files_lock); - files = proc->files; - if (files) - proc->files = NULL; - mutex_unlock(&proc->files_lock); - } - if (defer & BINDER_DEFERRED_FLUSH) binder_deferred_flush(proc);
if (defer & BINDER_DEFERRED_RELEASE) binder_deferred_release(proc); /* frees proc */ - - if (files) - put_files_struct(files); } while (proc); } static DECLARE_WORK(binder_deferred_work, binder_deferred_func); diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h index 588eb3ec3507..14de7ac57a34 100644 --- a/drivers/android/binder_trace.h +++ b/drivers/android/binder_trace.h @@ -223,22 +223,40 @@ TRACE_EVENT(binder_transaction_ref_to_ref, __entry->dest_ref_debug_id, __entry->dest_ref_desc) );
-TRACE_EVENT(binder_transaction_fd, - TP_PROTO(struct binder_transaction *t, int src_fd, int dest_fd), - TP_ARGS(t, src_fd, dest_fd), +TRACE_EVENT(binder_transaction_fd_send, + TP_PROTO(struct binder_transaction *t, int fd, size_t offset), + TP_ARGS(t, fd, offset),
TP_STRUCT__entry( __field(int, debug_id) - __field(int, src_fd) - __field(int, dest_fd) + __field(int, fd) + __field(size_t, offset) + ), + TP_fast_assign( + __entry->debug_id = t->debug_id; + __entry->fd = fd; + __entry->offset = offset; + ), + TP_printk("transaction=%d src_fd=%d offset=%zu", + __entry->debug_id, __entry->fd, __entry->offset) +); + +TRACE_EVENT(binder_transaction_fd_recv, + TP_PROTO(struct binder_transaction *t, int fd, size_t offset), + TP_ARGS(t, fd, offset), + + TP_STRUCT__entry( + __field(int, debug_id) + __field(int, fd) + __field(size_t, offset) ), TP_fast_assign( __entry->debug_id = t->debug_id; - __entry->src_fd = src_fd; - __entry->dest_fd = dest_fd; + __entry->fd = fd; + __entry->offset = offset; ), - TP_printk("transaction=%d src_fd=%d ==> dest_fd=%d", - __entry->debug_id, __entry->src_fd, __entry->dest_fd) + TP_printk("transaction=%d dest_fd=%d offset=%zu", + __entry->debug_id, __entry->fd, __entry->offset) );
DECLARE_EVENT_CLASS(binder_buffer_class,
From: Martijn Coenen maco@android.com
mainline inclusion from mainline-v4.20-rc1 commit b7e6a8961b5d6dd3fc535970e65d497d868bb49f category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
This allows the context manager to retrieve information about nodes that it holds a reference to, such as the current number of references to those nodes.
Such information can for example be used to determine whether the servicemanager is the only process holding a reference to a node. This information can then be passed on to the process holding the node, which can in turn decide whether it wants to shut down to reduce resource usage.
Signed-off-by: Martijn Coenen maco@android.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 55 +++++++++++++++++++++++++++++ include/uapi/linux/android/binder.h | 10 ++++++ 2 files changed, 65 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 8a72aacd7a39..f6a8fd678b4b 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -4678,6 +4678,42 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp) return ret; }
+static int binder_ioctl_get_node_info_for_ref(struct binder_proc *proc, + struct binder_node_info_for_ref *info) +{ + struct binder_node *node; + struct binder_context *context = proc->context; + __u32 handle = info->handle; + + if (info->strong_count || info->weak_count || info->reserved1 || + info->reserved2 || info->reserved3) { + binder_user_error("%d BINDER_GET_NODE_INFO_FOR_REF: only handle may be non-zero.", + proc->pid); + return -EINVAL; + } + + /* This ioctl may only be used by the context manager */ + mutex_lock(&context->context_mgr_node_lock); + if (!context->binder_context_mgr_node || + context->binder_context_mgr_node->proc != proc) { + mutex_unlock(&context->context_mgr_node_lock); + return -EPERM; + } + mutex_unlock(&context->context_mgr_node_lock); + + node = binder_get_node_from_ref(proc, handle, true, NULL); + if (!node) + return -EINVAL; + + info->strong_count = node->local_strong_refs + + node->internal_strong_refs; + info->weak_count = node->local_weak_refs; + + binder_put_node(node); + + return 0; +} + static int binder_ioctl_get_node_debug_info(struct binder_proc *proc, struct binder_node_debug_info *info) { @@ -4772,6 +4808,25 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) } break; } + case BINDER_GET_NODE_INFO_FOR_REF: { + struct binder_node_info_for_ref info; + + if (copy_from_user(&info, ubuf, sizeof(info))) { + ret = -EFAULT; + goto err; + } + + ret = binder_ioctl_get_node_info_for_ref(proc, &info); + if (ret < 0) + goto err; + + if (copy_to_user(ubuf, &info, sizeof(info))) { + ret = -EFAULT; + goto err; + } + + break; + } case BINDER_GET_NODE_DEBUG_INFO: { struct binder_node_debug_info info;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h index bfaec6903b8b..b9ba520f7e4b 100644 --- a/include/uapi/linux/android/binder.h +++ b/include/uapi/linux/android/binder.h @@ -200,6 +200,15 @@ struct binder_node_debug_info { __u32 has_weak_ref; };
+struct binder_node_info_for_ref { + __u32 handle; + __u32 strong_count; + __u32 weak_count; + __u32 reserved1; + __u32 reserved2; + __u32 reserved3; +}; + #define BINDER_WRITE_READ _IOWR('b', 1, struct binder_write_read) #define BINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64) #define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32) @@ -208,6 +217,7 @@ struct binder_node_debug_info { #define BINDER_THREAD_EXIT _IOW('b', 8, __s32) #define BINDER_VERSION _IOWR('b', 9, struct binder_version) #define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct binder_node_debug_info) +#define BINDER_GET_NODE_INFO_FOR_REF _IOWR('b', 12, struct binder_node_info_for_ref)
/* * NOTE: Two special error codes you should check for when calling
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit ec74136ded792deed80780a2f8baf3521eeb72f9 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
To allow servers to verify client identity, allow a node flag to be set that causes the sender's security context to be delivered with the transaction. The BR_TRANSACTION command is extended in BR_TRANSACTION_SEC_CTX to contain a pointer to the security context string.
Signed-off-by: Todd Kjos tkjos@google.com Reviewed-by: Joel Fernandes (Google) joel@joelfernandes.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 106 ++++++++++++++++++++++------ include/uapi/linux/android/binder.h | 19 +++++ 2 files changed, 102 insertions(+), 23 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index f6a8fd678b4b..888af4181941 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -355,6 +355,8 @@ struct binder_error { * (invariant after initialized) * @min_priority: minimum scheduling priority * (invariant after initialized) + * @txn_security_ctx: require sender's security context + * (invariant after initialized) * @async_todo: list of async work items * (protected by @proc->inner_lock) * @@ -391,6 +393,7 @@ struct binder_node { * invariant after initialization */ u8 accept_fds:1; + u8 txn_security_ctx:1; u8 min_priority; }; bool has_async_transaction; @@ -645,6 +648,7 @@ struct binder_transaction { long saved_priority; kuid_t sender_euid; struct list_head fd_fixups; + binder_uintptr_t security_ctx; /** * @lock: protects @from, @to_proc, and @to_thread * @@ -1147,6 +1151,7 @@ static struct binder_node *binder_init_node_ilocked( node->work.type = BINDER_WORK_NODE; node->min_priority = flags & FLAT_BINDER_FLAG_PRIORITY_MASK; node->accept_fds = !!(flags & FLAT_BINDER_FLAG_ACCEPTS_FDS); + node->txn_security_ctx = !!(flags & FLAT_BINDER_FLAG_TXN_SECURITY_CTX); spin_lock_init(&node->lock); INIT_LIST_HEAD(&node->work.entry); INIT_LIST_HEAD(&node->async_todo); @@ -2723,6 +2728,8 @@ 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); + char *secctx = NULL; + u32 secctx_sz = 0;
e = binder_transaction_log_add(&binder_transaction_log); e->debug_id = t_debug_id; @@ -2963,6 +2970,20 @@ static void binder_transaction(struct binder_proc *proc, t->flags = tr->flags; t->priority = task_nice(current);
+ if (target_node && target_node->txn_security_ctx) { + u32 secid; + + security_task_getsecid(proc->tsk, &secid); + ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); + if (ret) { + return_error = BR_FAILED_REPLY; + return_error_param = ret; + return_error_line = __LINE__; + goto err_get_secctx_failed; + } + extra_buffers_size += ALIGN(secctx_sz, sizeof(u64)); + } + trace_binder_transaction(reply, t, target_node);
t->buffer = binder_alloc_new_buf(&target_proc->alloc, tr->data_size, @@ -2979,6 +3000,19 @@ static void binder_transaction(struct binder_proc *proc, t->buffer = NULL; goto err_binder_alloc_buf_failed; } + if (secctx) { + size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) + + ALIGN(tr->offsets_size, sizeof(void *)) + + ALIGN(extra_buffers_size, sizeof(void *)) - + ALIGN(secctx_sz, sizeof(u64)); + char *kptr = t->buffer->data + buf_offset; + + t->security_ctx = (uintptr_t)kptr + + binder_alloc_get_user_buffer_offset(&target_proc->alloc); + memcpy(kptr, secctx, secctx_sz); + security_release_secctx(secctx, secctx_sz); + secctx = NULL; + } t->buffer->debug_id = t->debug_id; t->buffer->transaction = t; t->buffer->target_node = target_node; @@ -3248,6 +3282,9 @@ static void binder_transaction(struct binder_proc *proc, t->buffer->transaction = NULL; binder_alloc_free_buf(&target_proc->alloc, t->buffer); err_binder_alloc_buf_failed: + if (secctx) + security_release_secctx(secctx, secctx_sz); +err_get_secctx_failed: kfree(tcomplete); binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE); err_alloc_tcomplete_failed: @@ -3981,11 +4018,13 @@ static int binder_thread_read(struct binder_proc *proc,
while (1) { uint32_t cmd; - struct binder_transaction_data tr; + struct binder_transaction_data_secctx tr; + struct binder_transaction_data *trd = &tr.transaction_data; struct binder_work *w = NULL; struct list_head *list = NULL; struct binder_transaction *t = NULL; struct binder_thread *t_from; + size_t trsize = sizeof(*trd);
binder_inner_proc_lock(proc); if (!binder_worklist_empty_ilocked(&thread->todo)) @@ -4180,8 +4219,8 @@ static int binder_thread_read(struct binder_proc *proc, if (t->buffer->target_node) { struct binder_node *target_node = t->buffer->target_node;
- tr.target.ptr = target_node->ptr; - tr.cookie = target_node->cookie; + trd->target.ptr = target_node->ptr; + trd->cookie = target_node->cookie; t->saved_priority = task_nice(current); if (t->priority < target_node->min_priority && !(t->flags & TF_ONE_WAY)) @@ -4191,22 +4230,23 @@ static int binder_thread_read(struct binder_proc *proc, binder_set_nice(target_node->min_priority); cmd = BR_TRANSACTION; } else { - tr.target.ptr = 0; - tr.cookie = 0; + trd->target.ptr = 0; + trd->cookie = 0; cmd = BR_REPLY; } - tr.code = t->code; - tr.flags = t->flags; - tr.sender_euid = from_kuid(current_user_ns(), t->sender_euid); + trd->code = t->code; + trd->flags = t->flags; + trd->sender_euid = from_kuid(current_user_ns(), t->sender_euid);
t_from = binder_get_txn_from(t); if (t_from) { struct task_struct *sender = t_from->proc->tsk;
- tr.sender_pid = task_tgid_nr_ns(sender, - task_active_pid_ns(current)); + trd->sender_pid = + task_tgid_nr_ns(sender, + task_active_pid_ns(current)); } else { - tr.sender_pid = 0; + trd->sender_pid = 0; }
ret = binder_apply_fd_fixups(t); @@ -4237,15 +4277,20 @@ static int binder_thread_read(struct binder_proc *proc, } continue; } - tr.data_size = t->buffer->data_size; - tr.offsets_size = t->buffer->offsets_size; - tr.data.ptr.buffer = (binder_uintptr_t) + trd->data_size = t->buffer->data_size; + trd->offsets_size = t->buffer->offsets_size; + trd->data.ptr.buffer = (binder_uintptr_t) ((uintptr_t)t->buffer->data + binder_alloc_get_user_buffer_offset(&proc->alloc)); - tr.data.ptr.offsets = tr.data.ptr.buffer + + trd->data.ptr.offsets = trd->data.ptr.buffer + ALIGN(t->buffer->data_size, sizeof(void *));
+ tr.secctx = t->security_ctx; + if (t->security_ctx) { + cmd = BR_TRANSACTION_SEC_CTX; + trsize = sizeof(tr); + } if (put_user(cmd, (uint32_t __user *)ptr)) { if (t_from) binder_thread_dec_tmpref(t_from); @@ -4256,7 +4301,7 @@ static int binder_thread_read(struct binder_proc *proc, return -EFAULT; } ptr += sizeof(uint32_t); - if (copy_to_user(ptr, &tr, sizeof(tr))) { + if (copy_to_user(ptr, &tr, trsize)) { if (t_from) binder_thread_dec_tmpref(t_from);
@@ -4265,7 +4310,7 @@ static int binder_thread_read(struct binder_proc *proc,
return -EFAULT; } - ptr += sizeof(tr); + ptr += trsize;
trace_binder_transaction_received(t); binder_stat_br(proc, thread, cmd); @@ -4273,16 +4318,18 @@ static int binder_thread_read(struct binder_proc *proc, "%d:%d %s %d %d:%d, cmd %d size %zd-%zd ptr %016llx-%016llx\n", proc->pid, thread->pid, (cmd == BR_TRANSACTION) ? "BR_TRANSACTION" : - "BR_REPLY", + (cmd == BR_TRANSACTION_SEC_CTX) ? + "BR_TRANSACTION_SEC_CTX" : "BR_REPLY", t->debug_id, t_from ? t_from->proc->pid : 0, t_from ? t_from->pid : 0, cmd, t->buffer->data_size, t->buffer->offsets_size, - (u64)tr.data.ptr.buffer, (u64)tr.data.ptr.offsets); + (u64)trd->data.ptr.buffer, + (u64)trd->data.ptr.offsets);
if (t_from) binder_thread_dec_tmpref(t_from); t->buffer->allow_user_free = 1; - if (cmd == BR_TRANSACTION && !(t->flags & TF_ONE_WAY)) { + if (cmd != BR_REPLY && !(t->flags & TF_ONE_WAY)) { binder_inner_proc_lock(thread->proc); t->to_parent = thread->transaction_stack; t->to_thread = thread; @@ -4631,7 +4678,8 @@ static int binder_ioctl_write_read(struct file *filp, return ret; }
-static int binder_ioctl_set_ctx_mgr(struct file *filp) +static int binder_ioctl_set_ctx_mgr(struct file *filp, + struct flat_binder_object *fbo) { int ret = 0; struct binder_proc *proc = filp->private_data; @@ -4660,7 +4708,7 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp) } else { context->binder_context_mgr_uid = curr_euid; } - new_node = binder_new_node(proc, NULL); + new_node = binder_new_node(proc, fbo); if (!new_node) { ret = -ENOMEM; goto out; @@ -4783,8 +4831,20 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) binder_inner_proc_unlock(proc); break; } + case BINDER_SET_CONTEXT_MGR_EXT: { + struct flat_binder_object fbo; + + if (copy_from_user(&fbo, ubuf, sizeof(fbo))) { + ret = -EINVAL; + goto err; + } + ret = binder_ioctl_set_ctx_mgr(filp, &fbo); + if (ret) + goto err; + break; + } case BINDER_SET_CONTEXT_MGR: - ret = binder_ioctl_set_ctx_mgr(filp); + ret = binder_ioctl_set_ctx_mgr(filp, NULL); if (ret) goto err; break; diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h index b9ba520f7e4b..2832134e5397 100644 --- a/include/uapi/linux/android/binder.h +++ b/include/uapi/linux/android/binder.h @@ -41,6 +41,14 @@ enum { enum { FLAT_BINDER_FLAG_PRIORITY_MASK = 0xff, FLAT_BINDER_FLAG_ACCEPTS_FDS = 0x100, + + /** + * @FLAT_BINDER_FLAG_TXN_SECURITY_CTX: request security contexts + * + * Only when set, causes senders to include their security + * context + */ + FLAT_BINDER_FLAG_TXN_SECURITY_CTX = 0x1000, };
#ifdef BINDER_IPC_32BIT @@ -218,6 +226,7 @@ struct binder_node_info_for_ref { #define BINDER_VERSION _IOWR('b', 9, struct binder_version) #define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct binder_node_debug_info) #define BINDER_GET_NODE_INFO_FOR_REF _IOWR('b', 12, struct binder_node_info_for_ref) +#define BINDER_SET_CONTEXT_MGR_EXT _IOW('b', 13, struct flat_binder_object)
/* * NOTE: Two special error codes you should check for when calling @@ -276,6 +285,11 @@ struct binder_transaction_data { } data; };
+struct binder_transaction_data_secctx { + struct binder_transaction_data transaction_data; + binder_uintptr_t secctx; +}; + struct binder_transaction_data_sg { struct binder_transaction_data transaction_data; binder_size_t buffers_size; @@ -311,6 +325,11 @@ enum binder_driver_return_protocol { BR_OK = _IO('r', 1), /* No parameters! */
+ BR_TRANSACTION_SEC_CTX = _IOR('r', 2, + struct binder_transaction_data_secctx), + /* + * binder_transaction_data_secctx: the received command. + */ BR_TRANSACTION = _IOR('r', 2, struct binder_transaction_data), BR_REPLY = _IOR('r', 3, struct binder_transaction_data), /*
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.0-rc1 commit 7a2670a5bc917e4e7c9be5274efc004f9bd1216a category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
Fix the incomplete kerneldoc header for struct binder_buffer.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder_alloc.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h index fb3238c74c8a..c0aadbbf7f19 100644 --- a/drivers/android/binder_alloc.h +++ b/drivers/android/binder_alloc.h @@ -30,16 +30,16 @@ struct binder_transaction; * struct binder_buffer - buffer used for binder transactions * @entry: entry alloc->buffers * @rb_node: node for allocated_buffers/free_buffers rb trees - * @free: true if buffer is free - * @allow_user_free: describe the second member of struct blah, - * @async_transaction: describe the second member of struct blah, - * @debug_id: describe the second member of struct blah, - * @transaction: describe the second member of struct blah, - * @target_node: describe the second member of struct blah, - * @data_size: describe the second member of struct blah, - * @offsets_size: describe the second member of struct blah, - * @extra_buffers_size: describe the second member of struct blah, - * @data:i describe the second member of struct blah, + * @free: %true if buffer is free + * @allow_user_free: %true if user is allowed to free buffer + * @async_transaction: %true if buffer is in use for an async txn + * @debug_id: unique ID for debugging + * @transaction: pointer to associated struct binder_transaction + * @target_node: struct binder_node associated with this buffer + * @data_size: size of @transaction data + * @offsets_size: size of array of offsets + * @extra_buffers_size: size of space for other objects (like sg lists) + * @data: pointer to base of buffer space * * Bookkeeping structure for binder transaction buffers */
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.0-rc1 commit 80cd795630d6526ba729a089a435bf74a57af927 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
44d8047f1d8 ("binder: use standard functions to allocate fds") exposed a pre-existing issue in the binder driver.
fdget() is used in ksys_ioctl() as a performance optimization. One of the rules associated with fdget() is that ksys_close() must not be called between the fdget() and the fdput(). There is a case where this requirement is not met in the binder driver which results in the reference count dropping to 0 when the device is still in use. This can result in use-after-free or other issues.
If userpace has passed a file-descriptor for the binder driver using a BINDER_TYPE_FDA object, then kys_close() is called on it when handling a binder_ioctl(BC_FREE_BUFFER) command. This violates the assumptions for using fdget().
The problem is fixed by deferring the close using task_work_add(). A new variant of __close_fd() was created that returns a struct file with a reference. The fput() is deferred instead of using ksys_close().
Fixes: 44d8047f1d87a ("binder: use standard functions to allocate fds") Suggested-by: Al Viro viro@zeniv.linux.org.uk Signed-off-by: Todd Kjos tkjos@google.com Cc: stable stable@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Conflicts: drivers/android/binder.c include/linux/fdtable.h fs/file.c Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 65 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 888af4181941..021a7a967749 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -72,6 +72,7 @@ #include <linux/spinlock.h> #include <linux/ratelimit.h> #include <linux/syscalls.h> +#include <linux/task_work.h>
#include <uapi/linux/android/binder.h>
@@ -2170,6 +2171,66 @@ static bool binder_validate_fixup(struct binder_buffer *b, return (fixup_offset >= last_min_offset); }
+/** + * struct binder_task_work_cb - for deferred close + * + * @twork: callback_head for task work + * @fd: fd to close + * + * Structure to pass task work to be handled after + * returning from binder_ioctl() via task_work_add(). + */ +struct binder_task_work_cb { + struct callback_head twork; + struct file *file; +}; + +/** + * binder_do_fd_close() - close list of file descriptors + * @twork: callback head for task work + * + * It is not safe to call ksys_close() during the binder_ioctl() + * function if there is a chance that binder's own file descriptor + * might be closed. This is to meet the requirements for using + * fdget() (see comments for __fget_light()). Therefore use + * task_work_add() to schedule the close operation once we have + * returned from binder_ioctl(). This function is a callback + * for that mechanism and does the actual ksys_close() on the + * given file descriptor. + */ +static void binder_do_fd_close(struct callback_head *twork) +{ + struct binder_task_work_cb *twcb = container_of(twork, + struct binder_task_work_cb, twork); + + fput(twcb->file); + kfree(twcb); +} + +/** + * binder_deferred_fd_close() - schedule a close for the given file-descriptor + * @fd: file-descriptor to close + * + * See comments in binder_do_fd_close(). This function is used to schedule + * a file-descriptor to be closed after returning from binder_ioctl(). + */ +static void binder_deferred_fd_close(int fd) +{ + struct binder_task_work_cb *twcb; + + twcb = kzalloc(sizeof(*twcb), GFP_KERNEL); + if (!twcb) + return; + init_task_work(&twcb->twork, binder_do_fd_close); + __close_fd_get_file(fd, &twcb->file); + if (twcb->file) { + filp_close(twcb->file, current->files); + task_work_add(current, &twcb->twork, true); + } else { + kfree(twcb); + } +} + static void binder_transaction_buffer_release(struct binder_proc *proc, struct binder_buffer *buffer, binder_size_t *failed_at) @@ -2309,7 +2370,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, } fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset); for (fd_index = 0; fd_index < fda->num_fds; fd_index++) - ksys_close(fd_array[fd_index]); + binder_deferred_fd_close(fd_array[fd_index]); } break; default: pr_err("transaction release %d bad object type %x\n", @@ -3956,7 +4017,7 @@ static int binder_apply_fd_fixups(struct binder_transaction *t) } else if (ret) { u32 *fdp = (u32 *)(t->buffer->data + fixup->offset);
- ksys_close(*fdp); + binder_deferred_fd_close(*fdp); } list_del(&fixup->fixup_entry); kfree(fixup);
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit 1a7c3d9bb7a926e88d5f57643e75ad1abfc55013 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
The binder driver uses a vm_area to map the per-process binder buffer space. For 32-bit android devices, this is now taking too much vmalloc space. This patch removes the use of vm_area when copying the transaction data from the sender to the buffer space. Instead of using copy_from_user() for multi-page copies, it now uses binder_alloc_copy_user_to_buffer() which uses kmap() and kunmap() to map each page, and uses copy_from_user() for copying to that page.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 29 +++++++-- drivers/android/binder_alloc.c | 113 +++++++++++++++++++++++++++++++++ drivers/android/binder_alloc.h | 8 +++ 3 files changed, 143 insertions(+), 7 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 021a7a967749..8ce21a1019ef 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -3082,8 +3082,12 @@ static void binder_transaction(struct binder_proc *proc, ALIGN(tr->data_size, sizeof(void *))); offp = off_start;
- if (copy_from_user(t->buffer->data, (const void __user *)(uintptr_t) - tr->data.ptr.buffer, tr->data_size)) { + if (binder_alloc_copy_user_to_buffer( + &target_proc->alloc, + t->buffer, 0, + (const void __user *) + (uintptr_t)tr->data.ptr.buffer, + tr->data_size)) { binder_user_error("%d:%d got transaction with invalid data ptr\n", proc->pid, thread->pid); return_error = BR_FAILED_REPLY; @@ -3091,8 +3095,13 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_copy_data_failed; } - if (copy_from_user(offp, (const void __user *)(uintptr_t) - tr->data.ptr.offsets, tr->offsets_size)) { + if (binder_alloc_copy_user_to_buffer( + &target_proc->alloc, + t->buffer, + ALIGN(tr->data_size, sizeof(void *)), + (const void __user *) + (uintptr_t)tr->data.ptr.offsets, + tr->offsets_size)) { binder_user_error("%d:%d got transaction with invalid offsets ptr\n", proc->pid, thread->pid); return_error = BR_FAILED_REPLY; @@ -3221,6 +3230,8 @@ static void binder_transaction(struct binder_proc *proc, struct binder_buffer_object *bp = to_binder_buffer_object(hdr); size_t buf_left = sg_buf_end - sg_bufp; + binder_size_t sg_buf_offset = (uintptr_t)sg_bufp - + (uintptr_t)t->buffer->data;
if (bp->length > buf_left) { binder_user_error("%d:%d got transaction with too large buffer\n", @@ -3230,9 +3241,13 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_bad_offset; } - if (copy_from_user(sg_bufp, - (const void __user *)(uintptr_t) - bp->buffer, bp->length)) { + if (binder_alloc_copy_user_to_buffer( + &target_proc->alloc, + t->buffer, + sg_buf_offset, + (const void __user *) + (uintptr_t)bp->buffer, + bp->length)) { binder_user_error("%d:%d got transaction with invalid offsets ptr\n", proc->pid, thread->pid); return_error_param = -EFAULT; diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index c3fdd79c4082..342fbe124628 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -29,6 +29,8 @@ #include <linux/list_lru.h> #include <linux/ratelimit.h> #include <asm/cacheflush.h> +#include <linux/uaccess.h> +#include <linux/highmem.h> #include "binder_alloc.h" #include "binder_trace.h"
@@ -1058,3 +1060,114 @@ int binder_alloc_shrinker_init(void) } return ret; } + +/** + * check_buffer() - verify that buffer/offset is safe to access + * @alloc: binder_alloc for this proc + * @buffer: binder buffer to be accessed + * @offset: offset into @buffer data + * @bytes: bytes to access from offset + * + * Check that the @offset/@bytes are within the size of the given + * @buffer and that the buffer is currently active and not freeable. + * Offsets must also be multiples of sizeof(u32). The kernel is + * allowed to touch the buffer in two cases: + * + * 1) when the buffer is being created: + * (buffer->free == 0 && buffer->allow_user_free == 0) + * 2) when the buffer is being torn down: + * (buffer->free == 0 && buffer->transaction == NULL). + * + * Return: true if the buffer is safe to access + */ +static inline bool check_buffer(struct binder_alloc *alloc, + struct binder_buffer *buffer, + binder_size_t offset, size_t bytes) +{ + size_t buffer_size = binder_alloc_buffer_size(alloc, buffer); + + return buffer_size >= bytes && + offset <= buffer_size - bytes && + IS_ALIGNED(offset, sizeof(u32)) && + !buffer->free && + (!buffer->allow_user_free || !buffer->transaction); +} + +/** + * binder_alloc_get_page() - get kernel pointer for given buffer offset + * @alloc: binder_alloc for this proc + * @buffer: binder buffer to be accessed + * @buffer_offset: offset into @buffer data + * @pgoffp: address to copy final page offset to + * + * Lookup the struct page corresponding to the address + * at @buffer_offset into @buffer->data. If @pgoffp is not + * NULL, the byte-offset into the page is written there. + * + * The caller is responsible to ensure that the offset points + * to a valid address within the @buffer and that @buffer is + * not freeable by the user. Since it can't be freed, we are + * guaranteed that the corresponding elements of @alloc->pages[] + * cannot change. + * + * Return: struct page + */ +static struct page *binder_alloc_get_page(struct binder_alloc *alloc, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + pgoff_t *pgoffp) +{ + binder_size_t buffer_space_offset = buffer_offset + + (buffer->data - alloc->buffer); + pgoff_t pgoff = buffer_space_offset & ~PAGE_MASK; + size_t index = buffer_space_offset >> PAGE_SHIFT; + struct binder_lru_page *lru_page; + + lru_page = &alloc->pages[index]; + *pgoffp = pgoff; + return lru_page->page_ptr; +} + +/** + * binder_alloc_copy_user_to_buffer() - copy src user to tgt user + * @alloc: binder_alloc for this proc + * @buffer: binder buffer to be accessed + * @buffer_offset: offset into @buffer data + * @from: userspace pointer to source buffer + * @bytes: bytes to copy + * + * Copy bytes from source userspace to target buffer. + * + * Return: bytes remaining to be copied + */ +unsigned long +binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + const void __user *from, + size_t bytes) +{ + if (!check_buffer(alloc, buffer, buffer_offset, bytes)) + return bytes; + + while (bytes) { + unsigned long size; + unsigned long ret; + struct page *page; + pgoff_t pgoff; + void *kptr; + + page = binder_alloc_get_page(alloc, buffer, + buffer_offset, &pgoff); + size = min_t(size_t, bytes, PAGE_SIZE - pgoff); + kptr = kmap(page) + pgoff; + ret = copy_from_user(kptr, from, size); + kunmap(page); + if (ret) + return bytes - size + ret; + bytes -= size; + from += size; + buffer_offset += size; + } + return 0; +} diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h index c0aadbbf7f19..995155f31dbd 100644 --- a/drivers/android/binder_alloc.h +++ b/drivers/android/binder_alloc.h @@ -22,6 +22,7 @@ #include <linux/vmalloc.h> #include <linux/slab.h> #include <linux/list_lru.h> +#include <uapi/linux/android/binder.h>
extern struct list_lru binder_alloc_lru; struct binder_transaction; @@ -183,5 +184,12 @@ binder_alloc_get_user_buffer_offset(struct binder_alloc *alloc) return alloc->user_buffer_offset; }
+unsigned long +binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + const void __user *from, + size_t bytes); + #endif /* _LINUX_BINDER_ALLOC_H */
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit 8ced0c6231ead26eca8cb416dcb7cc1c2cdd41d8 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
Avoid vm_area when copying to or from binder buffers. Instead, new copy functions are added that copy from kernel space to binder buffer space. These use kmap_atomic() and kunmap_atomic() to create temporary mappings and then memcpy() is used to copy within that page.
Also, kmap_atomic() / kunmap_atomic() use the appropriate cache flushing to support VIVT cache architectures. Allow binder to build if CPU_CACHE_VIVT is defined.
Several uses of the new functions are added here. More to follow in subsequent patches.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/Kconfig | 2 +- drivers/android/binder.c | 119 +++++++++++++++++++++------------ drivers/android/binder_alloc.c | 58 ++++++++++++++++ drivers/android/binder_alloc.h | 12 ++++ 4 files changed, 146 insertions(+), 45 deletions(-)
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig index 51e8250d113f..432e9ad77070 100644 --- a/drivers/android/Kconfig +++ b/drivers/android/Kconfig @@ -10,7 +10,7 @@ if ANDROID
config ANDROID_BINDER_IPC bool "Android Binder IPC Driver" - depends on MMU && !CPU_CACHE_VIVT + depends on MMU default n ---help--- Binder is used in Android for both communication between processes, diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 8ce21a1019ef..1e5865e0e920 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2254,14 +2254,22 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, off_end = (void *)off_start + buffer->offsets_size; for (offp = off_start; offp < off_end; offp++) { struct binder_object_header *hdr; - size_t object_size = binder_validate_object(buffer, *offp); - + size_t object_size; + binder_size_t object_offset; + binder_size_t buffer_offset = (uintptr_t)offp - + (uintptr_t)buffer->data; + + binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, + buffer, buffer_offset, + sizeof(object_offset)); + object_size = binder_validate_object(buffer, object_offset); if (object_size == 0) { pr_err("transaction release %d bad object at offset %lld, size %zd\n", - debug_id, (u64)*offp, buffer->data_size); + debug_id, (u64)object_offset, buffer->data_size); continue; } - hdr = (struct binder_object_header *)(buffer->data + *offp); + hdr = (struct binder_object_header *) + (buffer->data + object_offset); switch (hdr->type) { case BINDER_TYPE_BINDER: case BINDER_TYPE_WEAK_BINDER: { @@ -2369,8 +2377,20 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, continue; } fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset); - for (fd_index = 0; fd_index < fda->num_fds; fd_index++) - binder_deferred_fd_close(fd_array[fd_index]); + for (fd_index = 0; fd_index < fda->num_fds; + fd_index++) { + u32 fd; + binder_size_t offset = + (uintptr_t)&fd_array[fd_index] - + (uintptr_t)buffer->data; + + binder_alloc_copy_from_buffer(&proc->alloc, + &fd, + buffer, + offset, + sizeof(fd)); + binder_deferred_fd_close(fd); + } } break; default: pr_err("transaction release %d bad object type %x\n", @@ -2502,7 +2522,7 @@ static int binder_translate_handle(struct flat_binder_object *fp, return ret; }
-static int binder_translate_fd(u32 *fdp, +static int binder_translate_fd(u32 fd, binder_size_t fd_offset, struct binder_transaction *t, struct binder_thread *thread, struct binder_transaction *in_reply_to) @@ -2513,7 +2533,6 @@ static int binder_translate_fd(u32 *fdp, struct file *file; int ret = 0; bool target_allows_fd; - int fd = *fdp;
if (in_reply_to) target_allows_fd = !!(in_reply_to->flags & TF_ACCEPT_FDS); @@ -2552,7 +2571,7 @@ static int binder_translate_fd(u32 *fdp, goto err_alloc; } fixup->file = file; - fixup->offset = (uintptr_t)fdp - (uintptr_t)t->buffer->data; + fixup->offset = fd_offset; trace_binder_transaction_fd_send(t, fd, fixup->offset); list_add_tail(&fixup->fixup_entry, &t->fd_fixups);
@@ -2604,8 +2623,17 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, return -EINVAL; } for (fdi = 0; fdi < fda->num_fds; fdi++) { - int ret = binder_translate_fd(&fd_array[fdi], t, thread, - in_reply_to); + u32 fd; + int ret; + binder_size_t offset = + (uintptr_t)&fd_array[fdi] - + (uintptr_t)t->buffer->data; + + binder_alloc_copy_from_buffer(&target_proc->alloc, + &fd, t->buffer, + offset, sizeof(fd)); + ret = binder_translate_fd(fd, offset, t, thread, + in_reply_to); if (ret < 0) return ret; } @@ -3070,7 +3098,9 @@ static void binder_transaction(struct binder_proc *proc,
t->security_ctx = (uintptr_t)kptr + binder_alloc_get_user_buffer_offset(&target_proc->alloc); - memcpy(kptr, secctx, secctx_sz); + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, buf_offset, + secctx, secctx_sz); security_release_secctx(secctx, secctx_sz); secctx = NULL; } @@ -3132,11 +3162,21 @@ static void binder_transaction(struct binder_proc *proc, off_min = 0; for (; offp < off_end; offp++) { struct binder_object_header *hdr; - size_t object_size = binder_validate_object(t->buffer, *offp); - - if (object_size == 0 || *offp < off_min) { + size_t object_size; + binder_size_t object_offset; + binder_size_t buffer_offset = + (uintptr_t)offp - (uintptr_t)t->buffer->data; + + binder_alloc_copy_from_buffer(&target_proc->alloc, + &object_offset, + t->buffer, + buffer_offset, + sizeof(object_offset)); + object_size = binder_validate_object(t->buffer, object_offset); + if (object_size == 0 || object_offset < off_min) { binder_user_error("%d:%d got transaction with invalid offset (%lld, min %lld max %lld) or object.\n", - proc->pid, thread->pid, (u64)*offp, + proc->pid, thread->pid, + (u64)object_offset, (u64)off_min, (u64)t->buffer->data_size); return_error = BR_FAILED_REPLY; @@ -3145,8 +3185,9 @@ static void binder_transaction(struct binder_proc *proc, goto err_bad_offset; }
- hdr = (struct binder_object_header *)(t->buffer->data + *offp); - off_min = *offp + object_size; + hdr = (struct binder_object_header *) + (t->buffer->data + object_offset); + off_min = object_offset + object_size; switch (hdr->type) { case BINDER_TYPE_BINDER: case BINDER_TYPE_WEAK_BINDER: { @@ -3177,8 +3218,10 @@ static void binder_transaction(struct binder_proc *proc,
case BINDER_TYPE_FD: { struct binder_fd_object *fp = to_binder_fd_object(hdr); - int ret = binder_translate_fd(&fp->fd, t, thread, - in_reply_to); + binder_size_t fd_offset = object_offset + + (uintptr_t)&fp->fd - (uintptr_t)fp; + int ret = binder_translate_fd(fp->fd, fd_offset, t, + thread, in_reply_to);
if (ret < 0) { return_error = BR_FAILED_REPLY; @@ -3973,6 +4016,7 @@ static int binder_wait_for_work(struct binder_thread *thread,
/** * binder_apply_fd_fixups() - finish fd translation + * @proc: binder_proc associated @t->buffer * @t: binder transaction with list of fd fixups * * Now that we are in the context of the transaction target @@ -3984,14 +4028,14 @@ static int binder_wait_for_work(struct binder_thread *thread, * fput'ing files that have not been processed and ksys_close'ing * any fds that have already been allocated. */ -static int binder_apply_fd_fixups(struct binder_transaction *t) +static int binder_apply_fd_fixups(struct binder_proc *proc, + struct binder_transaction *t) { struct binder_txn_fd_fixup *fixup, *tmp; int ret = 0;
list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) { int fd = get_unused_fd_flags(O_CLOEXEC); - u32 *fdp;
if (fd < 0) { binder_debug(BINDER_DEBUG_TRANSACTION, @@ -4006,33 +4050,20 @@ static int binder_apply_fd_fixups(struct binder_transaction *t) trace_binder_transaction_fd_recv(t, fd, fixup->offset); fd_install(fd, fixup->file); fixup->file = NULL; - fdp = (u32 *)(t->buffer->data + fixup->offset); - /* - * This store can cause problems for CPUs with a - * VIVT cache (eg ARMv5) since the cache cannot - * detect virtual aliases to the same physical cacheline. - * To support VIVT, this address and the user-space VA - * would both need to be flushed. Since this kernel - * VA is not constructed via page_to_virt(), we can't - * use flush_dcache_page() on it, so we'd have to use - * an internal function. If devices with VIVT ever - * need to run Android, we'll either need to go back - * to patching the translated fd from the sender side - * (using the non-standard kernel functions), or rework - * how the kernel uses the buffer to use page_to_virt() - * addresses instead of allocating in our own vm area. - * - * For now, we disable compilation if CONFIG_CPU_CACHE_VIVT. - */ - *fdp = fd; + binder_alloc_copy_to_buffer(&proc->alloc, t->buffer, + fixup->offset, &fd, + sizeof(u32)); } list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) { if (fixup->file) { fput(fixup->file); } else if (ret) { - u32 *fdp = (u32 *)(t->buffer->data + fixup->offset); + u32 fd;
- binder_deferred_fd_close(*fdp); + binder_alloc_copy_from_buffer(&proc->alloc, &fd, + t->buffer, fixup->offset, + sizeof(fd)); + binder_deferred_fd_close(fd); } list_del(&fixup->fixup_entry); kfree(fixup); @@ -4325,7 +4356,7 @@ static int binder_thread_read(struct binder_proc *proc, trd->sender_pid = 0; }
- ret = binder_apply_fd_fixups(t); + ret = binder_apply_fd_fixups(proc, t); if (ret) { struct binder_buffer *buffer = t->buffer; bool oneway = !!(t->flags & TF_ONE_WAY); diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 342fbe124628..f3054e30e85b 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -1171,3 +1171,61 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, } return 0; } + +static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc, + bool to_buffer, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + void *ptr, + size_t bytes) +{ + /* All copies must be 32-bit aligned and 32-bit size */ + BUG_ON(!check_buffer(alloc, buffer, buffer_offset, bytes)); + + while (bytes) { + unsigned long size; + struct page *page; + pgoff_t pgoff; + void *tmpptr; + void *base_ptr; + + page = binder_alloc_get_page(alloc, buffer, + buffer_offset, &pgoff); + size = min_t(size_t, bytes, PAGE_SIZE - pgoff); + base_ptr = kmap_atomic(page); + tmpptr = base_ptr + pgoff; + if (to_buffer) + memcpy(tmpptr, ptr, size); + else + memcpy(ptr, tmpptr, size); + /* + * kunmap_atomic() takes care of flushing the cache + * if this device has VIVT cache arch + */ + kunmap_atomic(base_ptr); + bytes -= size; + pgoff = 0; + ptr = ptr + size; + buffer_offset += size; + } +} + +void binder_alloc_copy_to_buffer(struct binder_alloc *alloc, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + void *src, + size_t bytes) +{ + binder_alloc_do_buffer_copy(alloc, true, buffer, buffer_offset, + src, bytes); +} + +void binder_alloc_copy_from_buffer(struct binder_alloc *alloc, + void *dest, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + size_t bytes) +{ + binder_alloc_do_buffer_copy(alloc, false, buffer, buffer_offset, + dest, bytes); +} diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h index 995155f31dbd..9d682b9d6c24 100644 --- a/drivers/android/binder_alloc.h +++ b/drivers/android/binder_alloc.h @@ -191,5 +191,17 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, const void __user *from, size_t bytes);
+void binder_alloc_copy_to_buffer(struct binder_alloc *alloc, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + void *src, + size_t bytes); + +void binder_alloc_copy_from_buffer(struct binder_alloc *alloc, + void *dest, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + size_t bytes); + #endif /* _LINUX_BINDER_ALLOC_H */
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit 7a67a39320dfba4b36d3be5dae4581194e650316 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
When creating or tearing down a transaction, the binder driver examines objects in the buffer and takes appropriate action. To do this without needing to dereference pointers into the buffer, the local copies of the objects are needed. This patch introduces a function to validate and copy binder objects from the buffer to a local structure.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 75 +++++++++++++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 17 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 1e5865e0e920..9c05a4478f66 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -659,6 +659,26 @@ struct binder_transaction { spinlock_t lock; };
+/** + * struct binder_object - union of flat binder object types + * @hdr: generic object header + * @fbo: binder object (nodes and refs) + * @fdo: file descriptor object + * @bbo: binder buffer pointer + * @fdao: file descriptor array + * + * Used for type-independent object copies + */ +struct binder_object { + union { + struct binder_object_header hdr; + struct flat_binder_object fbo; + struct binder_fd_object fdo; + struct binder_buffer_object bbo; + struct binder_fd_array_object fdao; + }; +}; + /** * binder_proc_lock() - Acquire outer lock for given binder_proc * @proc: struct binder_proc to acquire @@ -2025,26 +2045,33 @@ static void binder_cleanup_transaction(struct binder_transaction *t, }
/** - * binder_validate_object() - checks for a valid metadata object in a buffer. + * binder_get_object() - gets object and checks for valid metadata + * @proc: binder_proc owning the buffer * @buffer: binder_buffer that we're parsing. - * @offset: offset in the buffer at which to validate an object. + * @offset: offset in the @buffer at which to validate an object. + * @object: struct binder_object to read into * * Return: If there's a valid metadata object at @offset in @buffer, the - * size of that object. Otherwise, it returns zero. + * size of that object. Otherwise, it returns zero. The object + * is read into the struct binder_object pointed to by @object. */ -static size_t binder_validate_object(struct binder_buffer *buffer, u64 offset) +static size_t binder_get_object(struct binder_proc *proc, + struct binder_buffer *buffer, + unsigned long offset, + struct binder_object *object) { - /* Check if we can read a header first */ + size_t read_size; struct binder_object_header *hdr; size_t object_size = 0;
- if (buffer->data_size < sizeof(*hdr) || - offset > buffer->data_size - sizeof(*hdr) || - !IS_ALIGNED(offset, sizeof(u32))) + read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset); + if (read_size < sizeof(*hdr)) return 0; + binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, + offset, read_size);
- /* Ok, now see if we can read a complete object. */ - hdr = (struct binder_object_header *)(buffer->data + offset); + /* Ok, now see if we read a complete object. */ + hdr = &object->hdr; switch (hdr->type) { case BINDER_TYPE_BINDER: case BINDER_TYPE_WEAK_BINDER: @@ -2255,6 +2282,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, for (offp = off_start; offp < off_end; offp++) { struct binder_object_header *hdr; size_t object_size; + struct binder_object object; binder_size_t object_offset; binder_size_t buffer_offset = (uintptr_t)offp - (uintptr_t)buffer->data; @@ -2262,14 +2290,14 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, buffer, buffer_offset, sizeof(object_offset)); - object_size = binder_validate_object(buffer, object_offset); + object_size = binder_get_object(proc, buffer, + object_offset, &object); if (object_size == 0) { pr_err("transaction release %d bad object at offset %lld, size %zd\n", debug_id, (u64)object_offset, buffer->data_size); continue; } - hdr = (struct binder_object_header *) - (buffer->data + object_offset); + hdr = &object.hdr; switch (hdr->type) { case BINDER_TYPE_BINDER: case BINDER_TYPE_WEAK_BINDER: { @@ -3163,6 +3191,7 @@ static void binder_transaction(struct binder_proc *proc, for (; offp < off_end; offp++) { struct binder_object_header *hdr; size_t object_size; + struct binder_object object; binder_size_t object_offset; binder_size_t buffer_offset = (uintptr_t)offp - (uintptr_t)t->buffer->data; @@ -3172,7 +3201,8 @@ static void binder_transaction(struct binder_proc *proc, t->buffer, buffer_offset, sizeof(object_offset)); - object_size = binder_validate_object(t->buffer, object_offset); + object_size = binder_get_object(target_proc, t->buffer, + object_offset, &object); if (object_size == 0 || object_offset < off_min) { binder_user_error("%d:%d got transaction with invalid offset (%lld, min %lld max %lld) or object.\n", proc->pid, thread->pid, @@ -3185,8 +3215,7 @@ static void binder_transaction(struct binder_proc *proc, goto err_bad_offset; }
- hdr = (struct binder_object_header *) - (t->buffer->data + object_offset); + hdr = &object.hdr; off_min = object_offset + object_size; switch (hdr->type) { case BINDER_TYPE_BINDER: @@ -3201,6 +3230,9 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_translate_failed; } + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, object_offset, + fp, sizeof(*fp)); } break; case BINDER_TYPE_HANDLE: case BINDER_TYPE_WEAK_HANDLE: { @@ -3214,6 +3246,9 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_translate_failed; } + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, object_offset, + fp, sizeof(*fp)); } break;
case BINDER_TYPE_FD: { @@ -3230,6 +3265,9 @@ static void binder_transaction(struct binder_proc *proc, goto err_translate_failed; } fp->pad_binder = 0; + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, object_offset, + fp, sizeof(*fp)); } break; case BINDER_TYPE_FDA: { struct binder_fd_array_object *fda = @@ -3314,7 +3352,10 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_translate_failed; } - last_fixup_obj = bp; + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, object_offset, + bp, sizeof(*bp)); + last_fixup_obj = t->buffer->data + object_offset; last_fixup_min_off = 0; } break; default:
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit db6b0b810bf945d1991917ffce0e93383101f2fa category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
Refactor the functions to validate and fixup struct binder_buffer pointer objects to avoid using vm_area pointers. Instead copy to/from kernel space using binder_alloc_copy_to_buffer() and binder_alloc_copy_from_buffer(). The following functions were refactored:
refactor binder_validate_ptr() binder_validate_fixup() binder_fixup_parent()
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 146 ++++++++++++++++++++++++++------------- 1 file changed, 97 insertions(+), 49 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 9c05a4478f66..163eb6e5d4e7 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2100,10 +2100,13 @@ static size_t binder_get_object(struct binder_proc *proc,
/** * binder_validate_ptr() - validates binder_buffer_object in a binder_buffer. + * @proc: binder_proc owning the buffer * @b: binder_buffer containing the object + * @object: struct binder_object to read into * @index: index in offset array at which the binder_buffer_object is * located - * @start: points to the start of the offset array + * @start_offset: points to the start of the offset array + * @object_offsetp: offset of @object read from @b * @num_valid: the number of valid offsets in the offset array * * Return: If @index is within the valid range of the offset array @@ -2114,34 +2117,46 @@ static size_t binder_get_object(struct binder_proc *proc, * Note that the offset found in index @index itself is not * verified; this function assumes that @num_valid elements * from @start were previously verified to have valid offsets. + * If @object_offsetp is non-NULL, then the offset within + * @b is written to it. */ -static struct binder_buffer_object *binder_validate_ptr(struct binder_buffer *b, - binder_size_t index, - binder_size_t *start, - binder_size_t num_valid) +static struct binder_buffer_object *binder_validate_ptr( + struct binder_proc *proc, + struct binder_buffer *b, + struct binder_object *object, + binder_size_t index, + binder_size_t start_offset, + binder_size_t *object_offsetp, + binder_size_t num_valid) { - struct binder_buffer_object *buffer_obj; - binder_size_t *offp; + size_t object_size; + binder_size_t object_offset; + unsigned long buffer_offset;
if (index >= num_valid) return NULL;
- offp = start + index; - buffer_obj = (struct binder_buffer_object *)(b->data + *offp); - if (buffer_obj->hdr.type != BINDER_TYPE_PTR) + buffer_offset = start_offset + sizeof(binder_size_t) * index; + binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, + b, buffer_offset, sizeof(object_offset)); + object_size = binder_get_object(proc, b, object_offset, object); + if (!object_size || object->hdr.type != BINDER_TYPE_PTR) return NULL; + if (object_offsetp) + *object_offsetp = object_offset;
- return buffer_obj; + return &object->bbo; }
/** * binder_validate_fixup() - validates pointer/fd fixups happen in order. + * @proc: binder_proc owning the buffer * @b: transaction buffer - * @objects_start start of objects buffer - * @buffer: binder_buffer_object in which to fix up - * @offset: start offset in @buffer to fix up - * @last_obj: last binder_buffer_object that we fixed up in - * @last_min_offset: minimum fixup offset in @last_obj + * @objects_start_offset: offset to start of objects buffer + * @buffer_obj_offset: offset to binder_buffer_object in which to fix up + * @fixup_offset: start offset in @buffer to fix up + * @last_obj_offset: offset to last binder_buffer_object that we fixed + * @last_min_offset: minimum fixup offset in object at @last_obj_offset * * Return: %true if a fixup in buffer @buffer at offset @offset is * allowed. @@ -2172,28 +2187,41 @@ static struct binder_buffer_object *binder_validate_ptr(struct binder_buffer *b, * C (parent = A, offset = 16) * D (parent = B, offset = 0) // B is not A or any of A's parents */ -static bool binder_validate_fixup(struct binder_buffer *b, - binder_size_t *objects_start, - struct binder_buffer_object *buffer, +static bool binder_validate_fixup(struct binder_proc *proc, + struct binder_buffer *b, + binder_size_t objects_start_offset, + binder_size_t buffer_obj_offset, binder_size_t fixup_offset, - struct binder_buffer_object *last_obj, + binder_size_t last_obj_offset, binder_size_t last_min_offset) { - if (!last_obj) { + if (!last_obj_offset) { /* Nothing to fix up in */ return false; }
- while (last_obj != buffer) { + while (last_obj_offset != buffer_obj_offset) { + unsigned long buffer_offset; + struct binder_object last_object; + struct binder_buffer_object *last_bbo; + size_t object_size = binder_get_object(proc, b, last_obj_offset, + &last_object); + if (object_size != sizeof(*last_bbo)) + return false; + + last_bbo = &last_object.bbo; /* * Safe to retrieve the parent of last_obj, since it * was already previously verified by the driver. */ - if ((last_obj->flags & BINDER_BUFFER_FLAG_HAS_PARENT) == 0) + if ((last_bbo->flags & BINDER_BUFFER_FLAG_HAS_PARENT) == 0) return false; - last_min_offset = last_obj->parent_offset + sizeof(uintptr_t); - last_obj = (struct binder_buffer_object *) - (b->data + *(objects_start + last_obj->parent)); + last_min_offset = last_bbo->parent_offset + sizeof(uintptr_t); + buffer_offset = objects_start_offset + + sizeof(binder_size_t) * last_bbo->parent, + binder_alloc_copy_from_buffer(&proc->alloc, &last_obj_offset, + b, buffer_offset, + sizeof(last_obj_offset)); } return (fixup_offset >= last_min_offset); } @@ -2264,6 +2292,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, { binder_size_t *offp, *off_start, *off_end; int debug_id = buffer->debug_id; + binder_size_t off_start_offset;
binder_debug(BINDER_DEBUG_TRANSACTION, "%d buffer release %d, size %zd-%zd, failed at %pK\n", @@ -2273,8 +2302,8 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, if (buffer->target_node) binder_dec_node(buffer->target_node, 1, 0);
- off_start = (binder_size_t *)(buffer->data + - ALIGN(buffer->data_size, sizeof(void *))); + off_start_offset = ALIGN(buffer->data_size, sizeof(void *)); + off_start = (binder_size_t *)(buffer->data + off_start_offset); if (failed_at) off_end = failed_at; else @@ -2360,6 +2389,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, case BINDER_TYPE_FDA: { struct binder_fd_array_object *fda; struct binder_buffer_object *parent; + struct binder_object ptr_object; uintptr_t parent_buffer; u32 *fd_array; size_t fd_index; @@ -2375,8 +2405,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, }
fda = to_binder_fd_array_object(hdr); - parent = binder_validate_ptr(buffer, fda->parent, - off_start, + parent = binder_validate_ptr(proc, buffer, &ptr_object, + fda->parent, + off_start_offset, + NULL, offp - off_start); if (!parent) { pr_err("transaction release %d bad parent offset\n", @@ -2671,9 +2703,9 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, static int binder_fixup_parent(struct binder_transaction *t, struct binder_thread *thread, struct binder_buffer_object *bp, - binder_size_t *off_start, + binder_size_t off_start_offset, binder_size_t num_valid, - struct binder_buffer_object *last_fixup_obj, + binder_size_t last_fixup_obj_off, binder_size_t last_fixup_min_off) { struct binder_buffer_object *parent; @@ -2681,20 +2713,25 @@ static int binder_fixup_parent(struct binder_transaction *t, struct binder_buffer *b = t->buffer; struct binder_proc *proc = thread->proc; struct binder_proc *target_proc = t->to_proc; + struct binder_object object; + binder_size_t buffer_offset; + binder_size_t parent_offset;
if (!(bp->flags & BINDER_BUFFER_FLAG_HAS_PARENT)) return 0;
- parent = binder_validate_ptr(b, bp->parent, off_start, num_valid); + parent = binder_validate_ptr(target_proc, b, &object, bp->parent, + off_start_offset, &parent_offset, + num_valid); if (!parent) { binder_user_error("%d:%d got transaction with invalid parent offset or type\n", proc->pid, thread->pid); return -EINVAL; }
- if (!binder_validate_fixup(b, off_start, - parent, bp->parent_offset, - last_fixup_obj, + if (!binder_validate_fixup(target_proc, b, off_start_offset, + parent_offset, bp->parent_offset, + last_fixup_obj_off, last_fixup_min_off)) { binder_user_error("%d:%d got transaction with out-of-order buffer fixup\n", proc->pid, thread->pid); @@ -2711,7 +2748,10 @@ static int binder_fixup_parent(struct binder_transaction *t, parent_buffer = (u8 *)((uintptr_t)parent->buffer - binder_alloc_get_user_buffer_offset( &target_proc->alloc)); - *(binder_uintptr_t *)(parent_buffer + bp->parent_offset) = bp->buffer; + buffer_offset = bp->parent_offset + + (uintptr_t)parent_buffer - (uintptr_t)b->data; + binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset, + &bp->buffer, sizeof(bp->buffer));
return 0; } @@ -2831,6 +2871,7 @@ static void binder_transaction(struct binder_proc *proc, struct binder_work *w; struct binder_work *tcomplete; binder_size_t *offp, *off_end, *off_start; + binder_size_t off_start_offset; binder_size_t off_min; u8 *sg_bufp, *sg_buf_end; struct binder_proc *target_proc = NULL; @@ -2841,7 +2882,7 @@ static void binder_transaction(struct binder_proc *proc, uint32_t return_error = 0; uint32_t return_error_param = 0; uint32_t return_error_line = 0; - struct binder_buffer_object *last_fixup_obj = NULL; + binder_size_t last_fixup_obj_off = 0; binder_size_t last_fixup_min_off = 0; struct binder_context *context = proc->context; int t_debug_id = atomic_inc_return(&binder_last_id); @@ -3136,8 +3177,8 @@ static void binder_transaction(struct binder_proc *proc, t->buffer->transaction = t; t->buffer->target_node = target_node; trace_binder_transaction_alloc_buf(t->buffer); - off_start = (binder_size_t *)(t->buffer->data + - ALIGN(tr->data_size, sizeof(void *))); + off_start_offset = ALIGN(tr->data_size, sizeof(void *)); + off_start = (binder_size_t *)(t->buffer->data + off_start_offset); offp = off_start;
if (binder_alloc_copy_user_to_buffer( @@ -3270,11 +3311,15 @@ static void binder_transaction(struct binder_proc *proc, fp, sizeof(*fp)); } break; case BINDER_TYPE_FDA: { + struct binder_object ptr_object; + binder_size_t parent_offset; struct binder_fd_array_object *fda = to_binder_fd_array_object(hdr); struct binder_buffer_object *parent = - binder_validate_ptr(t->buffer, fda->parent, - off_start, + binder_validate_ptr(target_proc, t->buffer, + &ptr_object, fda->parent, + off_start_offset, + &parent_offset, offp - off_start); if (!parent) { binder_user_error("%d:%d got transaction with invalid parent offset or type\n", @@ -3284,9 +3329,11 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_bad_parent; } - if (!binder_validate_fixup(t->buffer, off_start, - parent, fda->parent_offset, - last_fixup_obj, + if (!binder_validate_fixup(target_proc, t->buffer, + off_start_offset, + parent_offset, + fda->parent_offset, + last_fixup_obj_off, last_fixup_min_off)) { binder_user_error("%d:%d got transaction with out-of-order buffer fixup\n", proc->pid, thread->pid); @@ -3303,7 +3350,7 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_translate_failed; } - last_fixup_obj = parent; + last_fixup_obj_off = parent_offset; last_fixup_min_off = fda->parent_offset + sizeof(u32) * fda->num_fds; } break; @@ -3342,9 +3389,10 @@ static void binder_transaction(struct binder_proc *proc, &target_proc->alloc); sg_bufp += ALIGN(bp->length, sizeof(u64));
- ret = binder_fixup_parent(t, thread, bp, off_start, + ret = binder_fixup_parent(t, thread, bp, + off_start_offset, offp - off_start, - last_fixup_obj, + last_fixup_obj_off, last_fixup_min_off); if (ret < 0) { return_error = BR_FAILED_REPLY; @@ -3355,7 +3403,7 @@ static void binder_transaction(struct binder_proc *proc, binder_alloc_copy_to_buffer(&target_proc->alloc, t->buffer, object_offset, bp, sizeof(*bp)); - last_fixup_obj = t->buffer->data + object_offset; + last_fixup_obj_off = object_offset; last_fixup_min_off = 0; } break; default:
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit 880211667b203dd32724f3be224c44c0400aa0a6 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
Remove the kernel's vm_area and the code that maps buffer pages into it.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder_alloc.c | 40 ++-------------------------------- 1 file changed, 2 insertions(+), 38 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index f3054e30e85b..474d90fd8163 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -265,16 +265,6 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, page->alloc = alloc; INIT_LIST_HEAD(&page->lru);
- ret = map_kernel_range_noflush((unsigned long)page_addr, - PAGE_SIZE, PAGE_KERNEL, - &page->page_ptr); - flush_cache_vmap((unsigned long)page_addr, - (unsigned long)page_addr + PAGE_SIZE); - if (ret != 1) { - pr_err("%d: binder_alloc_buf failed to map page at %pK in kernel\n", - alloc->pid, page_addr); - goto err_map_kernel_failed; - } user_page_addr = (uintptr_t)page_addr + alloc->user_buffer_offset; ret = vm_insert_page(vma, user_page_addr, page[0].page_ptr); @@ -315,8 +305,6 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, continue;
err_vm_insert_page_failed: - unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE); -err_map_kernel_failed: __free_page(page->page_ptr); page->page_ptr = NULL; err_alloc_page_failed: @@ -697,7 +685,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, struct vm_area_struct *vma) { int ret; - struct vm_struct *area; const char *failure_string; struct binder_buffer *buffer;
@@ -708,28 +695,10 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, goto err_already_mapped; }
- area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC); - if (area == NULL) { - ret = -ENOMEM; - failure_string = "get_vm_area"; - goto err_get_vm_area_failed; - } - alloc->buffer = area->addr; - alloc->user_buffer_offset = - vma->vm_start - (uintptr_t)alloc->buffer; + alloc->buffer = (void *)vma->vm_start; + alloc->user_buffer_offset = 0; mutex_unlock(&binder_alloc_mmap_lock);
-#ifdef CONFIG_CPU_CACHE_VIPT - if (cache_is_vipt_aliasing()) { - while (CACHE_COLOUR( - (vma->vm_start ^ (uint32_t)alloc->buffer))) { - pr_info("%s: %d %lx-%lx maps %pK bad alignment\n", - __func__, alloc->pid, vma->vm_start, - vma->vm_end, alloc->buffer); - vma->vm_start += PAGE_SIZE; - } - } -#endif alloc->pages = kcalloc((vma->vm_end - vma->vm_start) / PAGE_SIZE, sizeof(alloc->pages[0]), GFP_KERNEL); @@ -762,9 +731,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, alloc->pages = NULL; err_alloc_pages_failed: mutex_lock(&binder_alloc_mmap_lock); - vfree(alloc->buffer); alloc->buffer = NULL; -err_get_vm_area_failed: err_already_mapped: mutex_unlock(&binder_alloc_mmap_lock); binder_alloc_debug(BINDER_DEBUG_USER_ERROR, @@ -823,12 +790,10 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc) "%s: %d: page %d at %pK %s\n", __func__, alloc->pid, i, page_addr, on_lru ? "on lru" : "active"); - unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE); __free_page(alloc->pages[i].page_ptr); page_count++; } kfree(alloc->pages); - vfree(alloc->buffer); } mutex_unlock(&alloc->mutex); if (alloc->vma_vm_mm) @@ -993,7 +958,6 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
trace_binder_unmap_kernel_start(alloc, index);
- unmap_kernel_range(page_addr, PAGE_SIZE); __free_page(page->page_ptr); page->page_ptr = NULL;
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit c41358a5f5217abd7c051e8d42397e5b80f3b3ed category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
Remove user_buffer_offset since there is no kernel buffer pointer anymore.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Conflicts: drivers/android/binder_alloc.c Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 39 ++++++---------------------------- drivers/android/binder_alloc.c | 16 ++++++-------- drivers/android/binder_alloc.h | 23 -------------------- 3 files changed, 13 insertions(+), 65 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 163eb6e5d4e7..c171a7dd8e19 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2390,7 +2390,6 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, struct binder_fd_array_object *fda; struct binder_buffer_object *parent; struct binder_object ptr_object; - uintptr_t parent_buffer; u32 *fd_array; size_t fd_index; binder_size_t fd_buf_size; @@ -2415,14 +2414,6 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, debug_id); continue; } - /* - * Since the parent was already fixed up, convert it - * back to kernel address space to access it - */ - parent_buffer = parent->buffer - - binder_alloc_get_user_buffer_offset( - &proc->alloc); - fd_buf_size = sizeof(u32) * fda->num_fds; if (fda->num_fds >= SIZE_MAX / sizeof(u32)) { pr_err("transaction release %d invalid number of fds (%lld)\n", @@ -2436,7 +2427,8 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, debug_id, (u64)fda->num_fds); continue; } - fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset); + fd_array = (u32 *)(uintptr_t) + (parent->buffer + fda->parent_offset); for (fd_index = 0; fd_index < fda->num_fds; fd_index++) { u32 fd; @@ -2652,7 +2644,6 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, struct binder_transaction *in_reply_to) { binder_size_t fdi, fd_buf_size; - uintptr_t parent_buffer; u32 *fd_array; struct binder_proc *proc = thread->proc; struct binder_proc *target_proc = t->to_proc; @@ -2670,13 +2661,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, proc->pid, thread->pid, (u64)fda->num_fds); return -EINVAL; } - /* - * Since the parent was already fixed up, convert it - * back to the kernel address space to access it - */ - parent_buffer = parent->buffer - - binder_alloc_get_user_buffer_offset(&target_proc->alloc); - fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset); + fd_array = (u32 *)(uintptr_t)(parent->buffer + fda->parent_offset); if (!IS_ALIGNED((unsigned long)fd_array, sizeof(u32))) { binder_user_error("%d:%d parent offset not aligned correctly.\n", proc->pid, thread->pid); @@ -2709,7 +2694,6 @@ static int binder_fixup_parent(struct binder_transaction *t, binder_size_t last_fixup_min_off) { struct binder_buffer_object *parent; - u8 *parent_buffer; struct binder_buffer *b = t->buffer; struct binder_proc *proc = thread->proc; struct binder_proc *target_proc = t->to_proc; @@ -2745,11 +2729,8 @@ static int binder_fixup_parent(struct binder_transaction *t, proc->pid, thread->pid); return -EINVAL; } - parent_buffer = (u8 *)((uintptr_t)parent->buffer - - binder_alloc_get_user_buffer_offset( - &target_proc->alloc)); buffer_offset = bp->parent_offset + - (uintptr_t)parent_buffer - (uintptr_t)b->data; + (uintptr_t)parent->buffer - (uintptr_t)b->data; binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset, &bp->buffer, sizeof(bp->buffer));
@@ -3163,10 +3144,8 @@ static void binder_transaction(struct binder_proc *proc, ALIGN(tr->offsets_size, sizeof(void *)) + ALIGN(extra_buffers_size, sizeof(void *)) - ALIGN(secctx_sz, sizeof(u64)); - char *kptr = t->buffer->data + buf_offset;
- t->security_ctx = (uintptr_t)kptr + - binder_alloc_get_user_buffer_offset(&target_proc->alloc); + t->security_ctx = (uintptr_t)t->buffer->data + buf_offset; binder_alloc_copy_to_buffer(&target_proc->alloc, t->buffer, buf_offset, secctx, secctx_sz); @@ -3384,9 +3363,7 @@ static void binder_transaction(struct binder_proc *proc, goto err_copy_data_failed; } /* Fixup buffer pointer to target proc address space */ - bp->buffer = (uintptr_t)sg_bufp + - binder_alloc_get_user_buffer_offset( - &target_proc->alloc); + bp->buffer = (uintptr_t)sg_bufp; sg_bufp += ALIGN(bp->length, sizeof(u64));
ret = binder_fixup_parent(t, thread, bp, @@ -4475,9 +4452,7 @@ static int binder_thread_read(struct binder_proc *proc, } trd->data_size = t->buffer->data_size; trd->offsets_size = t->buffer->offsets_size; - trd->data.ptr.buffer = (binder_uintptr_t) - ((uintptr_t)t->buffer->data + - binder_alloc_get_user_buffer_offset(&proc->alloc)); + trd->data.ptr.buffer = (uintptr_t)t->buffer->data; trd->data.ptr.offsets = trd->data.ptr.buffer + ALIGN(t->buffer->data_size, sizeof(void *)); diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 474d90fd8163..9da631abfb04 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -138,17 +138,17 @@ static struct binder_buffer *binder_alloc_prepare_to_free_locked( { struct rb_node *n = alloc->allocated_buffers.rb_node; struct binder_buffer *buffer; - void *kern_ptr; + void *uptr;
- kern_ptr = (void *)(user_ptr - alloc->user_buffer_offset); + uptr = (void *)user_ptr;
while (n) { buffer = rb_entry(n, struct binder_buffer, rb_node); BUG_ON(buffer->free);
- if (kern_ptr < buffer->data) + if (uptr < buffer->data) n = n->rb_left; - else if (kern_ptr > buffer->data) + else if (uptr > buffer->data) n = n->rb_right; else { /* @@ -265,8 +265,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, page->alloc = alloc; INIT_LIST_HEAD(&page->lru);
- user_page_addr = - (uintptr_t)page_addr + alloc->user_buffer_offset; + user_page_addr = (uintptr_t)page_addr; ret = vm_insert_page(vma, user_page_addr, page[0].page_ptr); if (ret) { pr_err("%d: binder_alloc_buf failed to map page at %lx in userspace\n", @@ -696,7 +695,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, }
alloc->buffer = (void *)vma->vm_start; - alloc->user_buffer_offset = 0; mutex_unlock(&binder_alloc_mmap_lock);
alloc->pages = kcalloc((vma->vm_end - vma->vm_start) / PAGE_SIZE, @@ -947,9 +945,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item, if (vma) { trace_binder_unmap_user_start(alloc, index);
- zap_page_range(vma, - page_addr + alloc->user_buffer_offset, - PAGE_SIZE); + zap_page_range(vma, page_addr, PAGE_SIZE);
trace_binder_unmap_user_end(alloc, index); } diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h index 9d682b9d6c24..1026e9fb20db 100644 --- a/drivers/android/binder_alloc.h +++ b/drivers/android/binder_alloc.h @@ -82,7 +82,6 @@ struct binder_lru_page { * (invariant after init) * @vma_vm_mm: copy of vma->vm_mm (invarient after mmap) * @buffer: base of per-proc address space mapped via mmap - * @user_buffer_offset: offset between user and kernel VAs for buffer * @buffers: list of all buffers for this proc * @free_buffers: rb tree of buffers available for allocation * sorted by size @@ -104,7 +103,6 @@ struct binder_alloc { struct vm_area_struct *vma; struct mm_struct *vma_vm_mm; void *buffer; - ptrdiff_t user_buffer_offset; struct list_head buffers; struct rb_root free_buffers; struct rb_root allocated_buffers; @@ -163,27 +161,6 @@ binder_alloc_get_free_async_space(struct binder_alloc *alloc) return free_async_space; }
-/** - * binder_alloc_get_user_buffer_offset() - get offset between kernel/user addrs - * @alloc: binder_alloc for this proc - * - * Return: the offset between kernel and user-space addresses to use for - * virtual address conversion - */ -static inline ptrdiff_t -binder_alloc_get_user_buffer_offset(struct binder_alloc *alloc) -{ - /* - * user_buffer_offset is constant if vma is set and - * undefined if vma is not set. It is possible to - * get here with !alloc->vma if the target process - * is dying while a transaction is being initiated. - * Returning the old value is ok in this case and - * the transaction will fail. - */ - return alloc->user_buffer_offset; -} - unsigned long binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, struct binder_buffer *buffer,
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit bde4a19fc04f5f46298c86b1acb7a4af1d5f138d category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
Now that alloc->buffer points to the userspace vm_area rename buffer->data to buffer->user_data and rename local pointers that hold user addresses. Also use the "__user" tag to annotate all user pointers so sparse can flag cases where user pointer vaues are copied to kernel pointers. Refactor code to use offsets instead of user pointers.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 118 ++++++++++++++---------- drivers/android/binder_alloc.c | 87 ++++++++--------- drivers/android/binder_alloc.h | 6 +- drivers/android/binder_alloc_selftest.c | 4 +- drivers/android/binder_trace.h | 2 +- 5 files changed, 118 insertions(+), 99 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index c171a7dd8e19..c19c4eec5170 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2288,33 +2288,30 @@ static void binder_deferred_fd_close(int fd)
static void binder_transaction_buffer_release(struct binder_proc *proc, struct binder_buffer *buffer, - binder_size_t *failed_at) + binder_size_t failed_at, + bool is_failure) { - binder_size_t *offp, *off_start, *off_end; int debug_id = buffer->debug_id; - binder_size_t off_start_offset; + binder_size_t off_start_offset, buffer_offset, off_end_offset;
binder_debug(BINDER_DEBUG_TRANSACTION, - "%d buffer release %d, size %zd-%zd, failed at %pK\n", + "%d buffer release %d, size %zd-%zd, failed at %llx\n", proc->pid, buffer->debug_id, - buffer->data_size, buffer->offsets_size, failed_at); + buffer->data_size, buffer->offsets_size, + (unsigned long long)failed_at);
if (buffer->target_node) binder_dec_node(buffer->target_node, 1, 0);
off_start_offset = ALIGN(buffer->data_size, sizeof(void *)); - off_start = (binder_size_t *)(buffer->data + off_start_offset); - if (failed_at) - off_end = failed_at; - else - off_end = (void *)off_start + buffer->offsets_size; - for (offp = off_start; offp < off_end; offp++) { + off_end_offset = is_failure ? 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; size_t object_size; struct binder_object object; binder_size_t object_offset; - binder_size_t buffer_offset = (uintptr_t)offp - - (uintptr_t)buffer->data;
binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, buffer, buffer_offset, @@ -2390,9 +2387,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, struct binder_fd_array_object *fda; struct binder_buffer_object *parent; struct binder_object ptr_object; - u32 *fd_array; + binder_size_t fda_offset; size_t fd_index; binder_size_t fd_buf_size; + binder_size_t num_valid;
if (proc->tsk != current->group_leader) { /* @@ -2403,12 +2401,14 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, continue; }
+ num_valid = (buffer_offset - off_start_offset) / + sizeof(binder_size_t); fda = to_binder_fd_array_object(hdr); parent = binder_validate_ptr(proc, buffer, &ptr_object, fda->parent, off_start_offset, NULL, - offp - off_start); + num_valid); if (!parent) { pr_err("transaction release %d bad parent offset\n", debug_id); @@ -2427,14 +2427,21 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, debug_id, (u64)fda->num_fds); continue; } - fd_array = (u32 *)(uintptr_t) - (parent->buffer + fda->parent_offset); + /* + * the source data for binder_buffer_object is visible + * to user-space and the @buffer element is the user + * pointer to the buffer_object containing the fd_array. + * Convert the address to an offset relative to + * the base of the transaction buffer. + */ + fda_offset = + (parent->buffer - (uintptr_t)buffer->user_data) + + fda->parent_offset; for (fd_index = 0; fd_index < fda->num_fds; fd_index++) { u32 fd; - binder_size_t offset = - (uintptr_t)&fd_array[fd_index] - - (uintptr_t)buffer->data; + binder_size_t offset = fda_offset + + fd_index * sizeof(fd);
binder_alloc_copy_from_buffer(&proc->alloc, &fd, @@ -2644,7 +2651,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, struct binder_transaction *in_reply_to) { binder_size_t fdi, fd_buf_size; - u32 *fd_array; + binder_size_t fda_offset; struct binder_proc *proc = thread->proc; struct binder_proc *target_proc = t->to_proc;
@@ -2661,8 +2668,16 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, proc->pid, thread->pid, (u64)fda->num_fds); return -EINVAL; } - fd_array = (u32 *)(uintptr_t)(parent->buffer + fda->parent_offset); - if (!IS_ALIGNED((unsigned long)fd_array, sizeof(u32))) { + /* + * the source data for binder_buffer_object is visible + * to user-space and the @buffer element is the user + * pointer to the buffer_object containing the fd_array. + * Convert the address to an offset relative to + * the base of the transaction buffer. + */ + fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) + + fda->parent_offset; + if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32))) { binder_user_error("%d:%d parent offset not aligned correctly.\n", proc->pid, thread->pid); return -EINVAL; @@ -2670,9 +2685,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, for (fdi = 0; fdi < fda->num_fds; fdi++) { u32 fd; int ret; - binder_size_t offset = - (uintptr_t)&fd_array[fdi] - - (uintptr_t)t->buffer->data; + binder_size_t offset = fda_offset + fdi * sizeof(fd);
binder_alloc_copy_from_buffer(&target_proc->alloc, &fd, t->buffer, @@ -2730,7 +2743,7 @@ static int binder_fixup_parent(struct binder_transaction *t, return -EINVAL; } buffer_offset = bp->parent_offset + - (uintptr_t)parent->buffer - (uintptr_t)b->data; + (uintptr_t)parent->buffer - (uintptr_t)b->user_data; binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset, &bp->buffer, sizeof(bp->buffer));
@@ -2851,10 +2864,10 @@ static void binder_transaction(struct binder_proc *proc, struct binder_transaction *t; struct binder_work *w; struct binder_work *tcomplete; - binder_size_t *offp, *off_end, *off_start; - binder_size_t off_start_offset; + binder_size_t buffer_offset = 0; + binder_size_t off_start_offset, off_end_offset; binder_size_t off_min; - u8 *sg_bufp, *sg_buf_end; + binder_size_t sg_buf_offset, sg_buf_end_offset; struct binder_proc *target_proc = NULL; struct binder_thread *target_thread = NULL; struct binder_node *target_node = NULL; @@ -3145,7 +3158,7 @@ static void binder_transaction(struct binder_proc *proc, ALIGN(extra_buffers_size, sizeof(void *)) - ALIGN(secctx_sz, sizeof(u64));
- t->security_ctx = (uintptr_t)t->buffer->data + buf_offset; + t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset; binder_alloc_copy_to_buffer(&target_proc->alloc, t->buffer, buf_offset, secctx, secctx_sz); @@ -3156,9 +3169,6 @@ static void binder_transaction(struct binder_proc *proc, t->buffer->transaction = t; t->buffer->target_node = target_node; trace_binder_transaction_alloc_buf(t->buffer); - off_start_offset = ALIGN(tr->data_size, sizeof(void *)); - off_start = (binder_size_t *)(t->buffer->data + off_start_offset); - offp = off_start;
if (binder_alloc_copy_user_to_buffer( &target_proc->alloc, @@ -3204,17 +3214,18 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_bad_offset; } - off_end = (void *)off_start + tr->offsets_size; - sg_bufp = (u8 *)(PTR_ALIGN(off_end, sizeof(void *))); - sg_buf_end = sg_bufp + extra_buffers_size; + off_start_offset = ALIGN(tr->data_size, sizeof(void *)); + buffer_offset = off_start_offset; + off_end_offset = off_start_offset + tr->offsets_size; + sg_buf_offset = ALIGN(off_end_offset, sizeof(void *)); + sg_buf_end_offset = sg_buf_offset + extra_buffers_size; off_min = 0; - for (; offp < off_end; offp++) { + for (buffer_offset = off_start_offset; buffer_offset < off_end_offset; + buffer_offset += sizeof(binder_size_t)) { struct binder_object_header *hdr; size_t object_size; struct binder_object object; binder_size_t object_offset; - binder_size_t buffer_offset = - (uintptr_t)offp - (uintptr_t)t->buffer->data;
binder_alloc_copy_from_buffer(&target_proc->alloc, &object_offset, @@ -3294,12 +3305,14 @@ static void binder_transaction(struct binder_proc *proc, binder_size_t parent_offset; struct binder_fd_array_object *fda = to_binder_fd_array_object(hdr); + size_t num_valid = (buffer_offset - off_start_offset) * + sizeof(binder_size_t); struct binder_buffer_object *parent = binder_validate_ptr(target_proc, t->buffer, &ptr_object, fda->parent, off_start_offset, &parent_offset, - offp - off_start); + num_valid); if (!parent) { binder_user_error("%d:%d got transaction with invalid parent offset or type\n", proc->pid, thread->pid); @@ -3336,9 +3349,8 @@ static void binder_transaction(struct binder_proc *proc, case BINDER_TYPE_PTR: { struct binder_buffer_object *bp = to_binder_buffer_object(hdr); - size_t buf_left = sg_buf_end - sg_bufp; - binder_size_t sg_buf_offset = (uintptr_t)sg_bufp - - (uintptr_t)t->buffer->data; + size_t buf_left = sg_buf_end_offset - sg_buf_offset; + size_t num_valid;
if (bp->length > buf_left) { binder_user_error("%d:%d got transaction with too large buffer\n", @@ -3363,12 +3375,15 @@ static void binder_transaction(struct binder_proc *proc, goto err_copy_data_failed; } /* Fixup buffer pointer to target proc address space */ - bp->buffer = (uintptr_t)sg_bufp; - sg_bufp += ALIGN(bp->length, sizeof(u64)); + bp->buffer = (uintptr_t) + t->buffer->user_data + sg_buf_offset; + sg_buf_offset += ALIGN(bp->length, sizeof(u64));
+ num_valid = (buffer_offset - off_start_offset) * + sizeof(binder_size_t); ret = binder_fixup_parent(t, thread, bp, off_start_offset, - offp - off_start, + num_valid, last_fixup_obj_off, last_fixup_min_off); if (ret < 0) { @@ -3460,7 +3475,8 @@ static void binder_transaction(struct binder_proc *proc, err_copy_data_failed: binder_free_txn_fixups(t); trace_binder_transaction_failed_buffer_release(t->buffer); - binder_transaction_buffer_release(target_proc, t->buffer, offp); + binder_transaction_buffer_release(target_proc, t->buffer, + buffer_offset, true); if (target_node) binder_dec_node_tmpref(target_node); target_node = NULL; @@ -3563,7 +3579,7 @@ binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer) binder_node_inner_unlock(buf_node); } trace_binder_transaction_buffer_release(buffer); - binder_transaction_buffer_release(proc, buffer, NULL); + binder_transaction_buffer_release(proc, buffer, 0, false); binder_alloc_free_buf(&proc->alloc, buffer); }
@@ -4452,7 +4468,7 @@ static int binder_thread_read(struct binder_proc *proc, } trd->data_size = t->buffer->data_size; trd->offsets_size = t->buffer->offsets_size; - trd->data.ptr.buffer = (uintptr_t)t->buffer->data; + trd->data.ptr.buffer = (uintptr_t)t->buffer->user_data; trd->data.ptr.offsets = trd->data.ptr.buffer + ALIGN(t->buffer->data_size, sizeof(void *)); @@ -5488,7 +5504,7 @@ static void print_binder_transaction_ilocked(struct seq_file *m, seq_printf(m, " node %d", buffer->target_node->debug_id); seq_printf(m, " size %zd:%zd data %pK\n", buffer->data_size, buffer->offsets_size, - buffer->data); + buffer->user_data); }
static void print_binder_work_ilocked(struct seq_file *m, diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 9da631abfb04..f04dc5976751 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -69,9 +69,8 @@ static size_t binder_alloc_buffer_size(struct binder_alloc *alloc, struct binder_buffer *buffer) { if (list_is_last(&buffer->entry, &alloc->buffers)) - return (u8 *)alloc->buffer + - alloc->buffer_size - (u8 *)buffer->data; - return (u8 *)binder_buffer_next(buffer)->data - (u8 *)buffer->data; + return alloc->buffer + alloc->buffer_size - buffer->user_data; + return binder_buffer_next(buffer)->user_data - buffer->user_data; }
static void binder_insert_free_buffer(struct binder_alloc *alloc, @@ -121,9 +120,9 @@ static void binder_insert_allocated_buffer_locked( buffer = rb_entry(parent, struct binder_buffer, rb_node); BUG_ON(buffer->free);
- if (new_buffer->data < buffer->data) + if (new_buffer->user_data < buffer->user_data) p = &parent->rb_left; - else if (new_buffer->data > buffer->data) + else if (new_buffer->user_data > buffer->user_data) p = &parent->rb_right; else BUG(); @@ -138,17 +137,17 @@ static struct binder_buffer *binder_alloc_prepare_to_free_locked( { struct rb_node *n = alloc->allocated_buffers.rb_node; struct binder_buffer *buffer; - void *uptr; + void __user *uptr;
- uptr = (void *)user_ptr; + uptr = (void __user *)user_ptr;
while (n) { buffer = rb_entry(n, struct binder_buffer, rb_node); BUG_ON(buffer->free);
- if (uptr < buffer->data) + if (uptr < buffer->user_data) n = n->rb_left; - else if (uptr > buffer->data) + else if (uptr > buffer->user_data) n = n->rb_right; else { /* @@ -188,9 +187,9 @@ struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc, }
static int binder_update_page_range(struct binder_alloc *alloc, int allocate, - void *start, void *end) + void __user *start, void __user *end) { - void *page_addr; + void __user *page_addr; unsigned long user_page_addr; struct binder_lru_page *page; struct vm_area_struct *vma = NULL; @@ -359,8 +358,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked( struct binder_buffer *buffer; size_t buffer_size; struct rb_node *best_fit = NULL; - void *has_page_addr; - void *end_page_addr; + void __user *has_page_addr; + void __user *end_page_addr; size_t size, data_offsets_size; int ret;
@@ -458,15 +457,15 @@ static struct binder_buffer *binder_alloc_new_buf_locked( "%d: binder_alloc_buf size %zd got buffer %pK size %zd\n", alloc->pid, size, buffer, buffer_size);
- has_page_addr = - (void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK); + has_page_addr = (void __user *) + (((uintptr_t)buffer->user_data + buffer_size) & PAGE_MASK); WARN_ON(n && buffer_size != size); end_page_addr = - (void *)PAGE_ALIGN((uintptr_t)buffer->data + size); + (void __user *)PAGE_ALIGN((uintptr_t)buffer->user_data + size); if (end_page_addr > has_page_addr) end_page_addr = has_page_addr; - ret = binder_update_page_range(alloc, 1, - (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr); + ret = binder_update_page_range(alloc, 1, (void __user *) + PAGE_ALIGN((uintptr_t)buffer->user_data), end_page_addr); if (ret) return ERR_PTR(ret);
@@ -479,7 +478,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked( __func__, alloc->pid); goto err_alloc_buf_struct_failed; } - new_buffer->data = (u8 *)buffer->data + size; + new_buffer->user_data = (u8 __user *)buffer->user_data + size; list_add(&new_buffer->entry, &buffer->entry); new_buffer->free = 1; binder_insert_free_buffer(alloc, new_buffer); @@ -505,8 +504,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked( return buffer;
err_alloc_buf_struct_failed: - binder_update_page_range(alloc, 0, - (void *)PAGE_ALIGN((uintptr_t)buffer->data), + binder_update_page_range(alloc, 0, (void __user *) + PAGE_ALIGN((uintptr_t)buffer->user_data), end_page_addr); return ERR_PTR(-ENOMEM); } @@ -541,14 +540,15 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc, return buffer; }
-static void *buffer_start_page(struct binder_buffer *buffer) +static void __user *buffer_start_page(struct binder_buffer *buffer) { - return (void *)((uintptr_t)buffer->data & PAGE_MASK); + return (void __user *)((uintptr_t)buffer->user_data & PAGE_MASK); }
-static void *prev_buffer_end_page(struct binder_buffer *buffer) +static void __user *prev_buffer_end_page(struct binder_buffer *buffer) { - return (void *)(((uintptr_t)(buffer->data) - 1) & PAGE_MASK); + return (void __user *) + (((uintptr_t)(buffer->user_data) - 1) & PAGE_MASK); }
static void binder_delete_free_buffer(struct binder_alloc *alloc, @@ -563,7 +563,8 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc, to_free = false; binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: merge free, buffer %pK share page with %pK\n", - alloc->pid, buffer->data, prev->data); + alloc->pid, buffer->user_data, + prev->user_data); }
if (!list_is_last(&buffer->entry, &alloc->buffers)) { @@ -573,23 +574,24 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc, binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: merge free, buffer %pK share page with %pK\n", alloc->pid, - buffer->data, - next->data); + buffer->user_data, + next->user_data); } }
- if (PAGE_ALIGNED(buffer->data)) { + if (PAGE_ALIGNED(buffer->user_data)) { binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: merge free, buffer start %pK is page aligned\n", - alloc->pid, buffer->data); + alloc->pid, buffer->user_data); to_free = false; }
if (to_free) { binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: merge free, buffer %pK do not share page with %pK or %pK\n", - alloc->pid, buffer->data, - prev->data, next ? next->data : NULL); + alloc->pid, buffer->user_data, + prev->user_data, + next ? next->user_data : NULL); binder_update_page_range(alloc, 0, buffer_start_page(buffer), buffer_start_page(buffer) + PAGE_SIZE); } @@ -615,8 +617,8 @@ static void binder_free_buf_locked(struct binder_alloc *alloc, BUG_ON(buffer->free); BUG_ON(size > buffer_size); BUG_ON(buffer->transaction != NULL); - BUG_ON(buffer->data < alloc->buffer); - BUG_ON(buffer->data > alloc->buffer + alloc->buffer_size); + BUG_ON(buffer->user_data < alloc->buffer); + BUG_ON(buffer->user_data > alloc->buffer + alloc->buffer_size);
if (buffer->async_transaction) { alloc->free_async_space += size + sizeof(struct binder_buffer); @@ -627,8 +629,9 @@ static void binder_free_buf_locked(struct binder_alloc *alloc, }
binder_update_page_range(alloc, 0, - (void *)PAGE_ALIGN((uintptr_t)buffer->data), - (void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK)); + (void __user *)PAGE_ALIGN((uintptr_t)buffer->user_data), + (void __user *)(((uintptr_t) + buffer->user_data + buffer_size) & PAGE_MASK));
rb_erase(&buffer->rb_node, &alloc->allocated_buffers); buffer->free = 1; @@ -694,7 +697,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, goto err_already_mapped; }
- alloc->buffer = (void *)vma->vm_start; + alloc->buffer = (void __user *)vma->vm_start; mutex_unlock(&binder_alloc_mmap_lock);
alloc->pages = kcalloc((vma->vm_end - vma->vm_start) / PAGE_SIZE, @@ -714,7 +717,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, goto err_alloc_buf_struct_failed; }
- buffer->data = alloc->buffer; + buffer->user_data = alloc->buffer; list_add(&buffer->entry, &alloc->buffers); buffer->free = 1; binder_insert_free_buffer(alloc, buffer); @@ -775,7 +778,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc) int i;
for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) { - void *page_addr; + void __user *page_addr; bool on_lru;
if (!alloc->pages[i].page_ptr) @@ -806,7 +809,7 @@ static void print_binder_buffer(struct seq_file *m, const char *prefix, struct binder_buffer *buffer) { seq_printf(m, "%s %d: %pK size %zd:%zd:%zd %s\n", - prefix, buffer->debug_id, buffer->data, + prefix, buffer->debug_id, buffer->user_data, buffer->data_size, buffer->offsets_size, buffer->extra_buffers_size, buffer->transaction ? "active" : "delivered"); @@ -1061,7 +1064,7 @@ static inline bool check_buffer(struct binder_alloc *alloc, * @pgoffp: address to copy final page offset to * * Lookup the struct page corresponding to the address - * at @buffer_offset into @buffer->data. If @pgoffp is not + * at @buffer_offset into @buffer->user_data. If @pgoffp is not * NULL, the byte-offset into the page is written there. * * The caller is responsible to ensure that the offset points @@ -1078,7 +1081,7 @@ static struct page *binder_alloc_get_page(struct binder_alloc *alloc, pgoff_t *pgoffp) { binder_size_t buffer_space_offset = buffer_offset + - (buffer->data - alloc->buffer); + (buffer->user_data - alloc->buffer); pgoff_t pgoff = buffer_space_offset & ~PAGE_MASK; size_t index = buffer_space_offset >> PAGE_SHIFT; struct binder_lru_page *lru_page; diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h index 1026e9fb20db..b60d161b7a7a 100644 --- a/drivers/android/binder_alloc.h +++ b/drivers/android/binder_alloc.h @@ -40,7 +40,7 @@ struct binder_transaction; * @data_size: size of @transaction data * @offsets_size: size of array of offsets * @extra_buffers_size: size of space for other objects (like sg lists) - * @data: pointer to base of buffer space + * @user_data: user pointer to base of buffer space * * Bookkeeping structure for binder transaction buffers */ @@ -59,7 +59,7 @@ struct binder_buffer { size_t data_size; size_t offsets_size; size_t extra_buffers_size; - void *data; + void __user *user_data; };
/** @@ -102,7 +102,7 @@ struct binder_alloc { struct mutex mutex; struct vm_area_struct *vma; struct mm_struct *vma_vm_mm; - void *buffer; + void __user *buffer; struct list_head buffers; struct rb_root free_buffers; struct rb_root allocated_buffers; diff --git a/drivers/android/binder_alloc_selftest.c b/drivers/android/binder_alloc_selftest.c index 8bd7bcef967d..f0f4d7d02635 100644 --- a/drivers/android/binder_alloc_selftest.c +++ b/drivers/android/binder_alloc_selftest.c @@ -105,8 +105,8 @@ static bool check_buffer_pages_allocated(struct binder_alloc *alloc, void *page_addr, *end; int page_index;
- end = (void *)PAGE_ALIGN((uintptr_t)buffer->data + size); - page_addr = buffer->data; + end = (void *)PAGE_ALIGN((uintptr_t)buffer->user_data + size); + page_addr = buffer->user_data; for (; page_addr < end; page_addr += PAGE_SIZE) { page_index = (page_addr - alloc->buffer) / PAGE_SIZE; if (!alloc->pages[page_index].page_ptr || diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h index 14de7ac57a34..83cc254d2335 100644 --- a/drivers/android/binder_trace.h +++ b/drivers/android/binder_trace.h @@ -293,7 +293,7 @@ DEFINE_EVENT(binder_buffer_class, binder_transaction_failed_buffer_release,
TRACE_EVENT(binder_update_page_range, TP_PROTO(struct binder_alloc *alloc, bool allocate, - void *start, void *end), + void __user *start, void __user *end), TP_ARGS(alloc, allocate, start, end), TP_STRUCT__entry( __field(int, proc)
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc1 commit 26528be6720bb40bc8844e97ee73a37e530e9c5e category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
Fixes crash found by syzbot: kernel BUG at drivers/android/binder_alloc.c:LINE! (2)
Reported-and-tested-by: syzbot+55de1eb4975dec156d8f@syzkaller.appspotmail.com Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index c19c4eec5170..6042a5b713be 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2065,7 +2065,7 @@ static size_t binder_get_object(struct binder_proc *proc, size_t object_size = 0;
read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset); - if (read_size < sizeof(*hdr)) + if (read_size < sizeof(*hdr) || !IS_ALIGNED(offset, sizeof(u32))) return 0; binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, offset, read_size);
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.1-rc3 commit 5997da82145bb7c9a56d834894cb81f81f219344 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
The selinux-testsuite found an issue resulting in a BUG_ON() where a conditional relied on a size_t going negative when checking the validity of a buffer offset.
Fixes: 7a67a39320df ("binder: add function to copy binder object from buffer") Reported-by: Paul Moore paul@paul-moore.com Tested-by: Paul Moore paul@paul-moore.com Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 6042a5b713be..a32dad26f422 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2065,7 +2065,8 @@ static size_t binder_get_object(struct binder_proc *proc, size_t object_size = 0;
read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset); - if (read_size < sizeof(*hdr) || !IS_ALIGNED(offset, sizeof(u32))) + if (offset > buffer->data_size || read_size < sizeof(*hdr) || + !IS_ALIGNED(offset, sizeof(u32))) return 0; binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, offset, read_size);
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.2-rc1 commit 0b0509508beff65c1d50541861bc0d4973487dc5 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
When allocating space in the target buffer for the security context, make sure the extra_buffers_size doesn't overflow. This can only happen if the given size is invalid, but an overflow can turn it into a valid size. Fail the transaction if an overflow is detected.
Signed-off-by: Todd Kjos tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index a32dad26f422..7f37f493715d 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -3125,6 +3125,7 @@ static void binder_transaction(struct binder_proc *proc,
if (target_node && target_node->txn_security_ctx) { u32 secid; + size_t added_size;
security_task_getsecid(proc->tsk, &secid); ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); @@ -3134,7 +3135,15 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_get_secctx_failed; } - extra_buffers_size += ALIGN(secctx_sz, sizeof(u64)); + added_size = ALIGN(secctx_sz, sizeof(u64)); + extra_buffers_size += added_size; + if (extra_buffers_size < added_size) { + /* integer overflow of extra_buffers_size */ + return_error = BR_FAILED_REPLY; + return_error_param = EINVAL; + return_error_line = __LINE__; + goto err_bad_extra_size; + } }
trace_binder_transaction(reply, t, target_node); @@ -3484,6 +3493,7 @@ static void binder_transaction(struct binder_proc *proc, t->buffer->transaction = NULL; binder_alloc_free_buf(&target_proc->alloc, t->buffer); err_binder_alloc_buf_failed: +err_bad_extra_size: if (secctx) security_release_secctx(secctx, secctx_sz); err_get_secctx_failed:
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.3-rc1 commit bb4a2e48d5100ed3ff614df158a636bca3c6bf9f category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
The buffer copy functions assumed the caller would ensure correct alignment and that the memory to be copied was completely within the binder buffer. There have been a few cases discovered by syzkallar where a malformed transaction created by a user could violated the assumptions and resulted in a BUG_ON.
The fix is to remove the BUG_ON and always return the error to be handled appropriately by the caller.
Acked-by: Martijn Coenen maco@android.com Reported-by: syzbot+3ae18325f96190606754@syzkaller.appspotmail.com Fixes: bde4a19fc04f ("binder: use userspace pointer as base of buffer space") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Todd Kjos tkjos@google.com Cc: stable stable@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 153 ++++++++++++++++++++------------- drivers/android/binder_alloc.c | 44 +++++----- drivers/android/binder_alloc.h | 22 ++--- 3 files changed, 126 insertions(+), 93 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 7f37f493715d..3dcf28177ce6 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2066,10 +2066,9 @@ static size_t binder_get_object(struct binder_proc *proc,
read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset); if (offset > buffer->data_size || read_size < sizeof(*hdr) || - !IS_ALIGNED(offset, sizeof(u32))) + binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, + offset, read_size)) return 0; - binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, - offset, read_size);
/* Ok, now see if we read a complete object. */ hdr = &object->hdr; @@ -2138,8 +2137,10 @@ static struct binder_buffer_object *binder_validate_ptr( return NULL;
buffer_offset = start_offset + sizeof(binder_size_t) * index; - binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, - b, buffer_offset, sizeof(object_offset)); + if (binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, + b, buffer_offset, + sizeof(object_offset))) + return NULL; object_size = binder_get_object(proc, b, object_offset, object); if (!object_size || object->hdr.type != BINDER_TYPE_PTR) return NULL; @@ -2219,10 +2220,12 @@ static bool binder_validate_fixup(struct binder_proc *proc, return false; last_min_offset = last_bbo->parent_offset + sizeof(uintptr_t); buffer_offset = objects_start_offset + - sizeof(binder_size_t) * last_bbo->parent, - binder_alloc_copy_from_buffer(&proc->alloc, &last_obj_offset, - b, buffer_offset, - sizeof(last_obj_offset)); + sizeof(binder_size_t) * last_bbo->parent; + if (binder_alloc_copy_from_buffer(&proc->alloc, + &last_obj_offset, + b, buffer_offset, + sizeof(last_obj_offset))) + return false; } return (fixup_offset >= last_min_offset); } @@ -2310,15 +2313,15 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, for (buffer_offset = off_start_offset; buffer_offset < off_end_offset; buffer_offset += sizeof(binder_size_t)) { struct binder_object_header *hdr; - size_t object_size; + size_t object_size = 0; struct binder_object object; binder_size_t object_offset;
- binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, - buffer, buffer_offset, - sizeof(object_offset)); - object_size = binder_get_object(proc, buffer, - object_offset, &object); + if (!binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, + buffer, buffer_offset, + sizeof(object_offset))) + object_size = binder_get_object(proc, buffer, + object_offset, &object); if (object_size == 0) { pr_err("transaction release %d bad object at offset %lld, size %zd\n", debug_id, (u64)object_offset, buffer->data_size); @@ -2441,15 +2444,16 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, for (fd_index = 0; fd_index < fda->num_fds; fd_index++) { u32 fd; + int err; binder_size_t offset = fda_offset + fd_index * sizeof(fd);
- binder_alloc_copy_from_buffer(&proc->alloc, - &fd, - buffer, - offset, - sizeof(fd)); - binder_deferred_fd_close(fd); + err = binder_alloc_copy_from_buffer( + &proc->alloc, &fd, buffer, + offset, sizeof(fd)); + WARN_ON(err); + if (!err) + binder_deferred_fd_close(fd); } } break; default: @@ -2688,11 +2692,12 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, int ret; binder_size_t offset = fda_offset + fdi * sizeof(fd);
- binder_alloc_copy_from_buffer(&target_proc->alloc, - &fd, t->buffer, - offset, sizeof(fd)); - ret = binder_translate_fd(fd, offset, t, thread, - in_reply_to); + ret = binder_alloc_copy_from_buffer(&target_proc->alloc, + &fd, t->buffer, + offset, sizeof(fd)); + if (!ret) + ret = binder_translate_fd(fd, offset, t, thread, + in_reply_to); if (ret < 0) return ret; } @@ -2745,8 +2750,12 @@ static int binder_fixup_parent(struct binder_transaction *t, } buffer_offset = bp->parent_offset + (uintptr_t)parent->buffer - (uintptr_t)b->user_data; - binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset, - &bp->buffer, sizeof(bp->buffer)); + if (binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset, + &bp->buffer, sizeof(bp->buffer))) { + binder_user_error("%d:%d got transaction with invalid parent offset\n", + proc->pid, thread->pid); + return -EINVAL; + }
return 0; } @@ -3163,15 +3172,20 @@ static void binder_transaction(struct binder_proc *proc, goto err_binder_alloc_buf_failed; } if (secctx) { + int err; size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) + ALIGN(tr->offsets_size, sizeof(void *)) + ALIGN(extra_buffers_size, sizeof(void *)) - ALIGN(secctx_sz, sizeof(u64));
t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset; - binder_alloc_copy_to_buffer(&target_proc->alloc, - t->buffer, buf_offset, - secctx, secctx_sz); + err = binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, buf_offset, + secctx, secctx_sz); + if (err) { + t->security_ctx = 0; + WARN_ON(1); + } security_release_secctx(secctx, secctx_sz); secctx = NULL; } @@ -3237,11 +3251,16 @@ static void binder_transaction(struct binder_proc *proc, struct binder_object object; binder_size_t object_offset;
- binder_alloc_copy_from_buffer(&target_proc->alloc, - &object_offset, - t->buffer, - buffer_offset, - sizeof(object_offset)); + if (binder_alloc_copy_from_buffer(&target_proc->alloc, + &object_offset, + t->buffer, + buffer_offset, + sizeof(object_offset))) { + return_error = BR_FAILED_REPLY; + return_error_param = -EINVAL; + return_error_line = __LINE__; + goto err_bad_offset; + } object_size = binder_get_object(target_proc, t->buffer, object_offset, &object); if (object_size == 0 || object_offset < off_min) { @@ -3265,15 +3284,17 @@ static void binder_transaction(struct binder_proc *proc,
fp = to_flat_binder_object(hdr); ret = binder_translate_binder(fp, t, thread); - if (ret < 0) { + + if (ret < 0 || + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, + object_offset, + fp, sizeof(*fp))) { return_error = BR_FAILED_REPLY; return_error_param = ret; return_error_line = __LINE__; goto err_translate_failed; } - binder_alloc_copy_to_buffer(&target_proc->alloc, - t->buffer, object_offset, - fp, sizeof(*fp)); } break; case BINDER_TYPE_HANDLE: case BINDER_TYPE_WEAK_HANDLE: { @@ -3281,15 +3302,16 @@ static void binder_transaction(struct binder_proc *proc,
fp = to_flat_binder_object(hdr); ret = binder_translate_handle(fp, t, thread); - if (ret < 0) { + if (ret < 0 || + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, + object_offset, + fp, sizeof(*fp))) { return_error = BR_FAILED_REPLY; return_error_param = ret; return_error_line = __LINE__; goto err_translate_failed; } - binder_alloc_copy_to_buffer(&target_proc->alloc, - t->buffer, object_offset, - fp, sizeof(*fp)); } break;
case BINDER_TYPE_FD: { @@ -3299,16 +3321,17 @@ static void binder_transaction(struct binder_proc *proc, int ret = binder_translate_fd(fp->fd, fd_offset, t, thread, in_reply_to);
- if (ret < 0) { + fp->pad_binder = 0; + if (ret < 0 || + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, + object_offset, + fp, sizeof(*fp))) { return_error = BR_FAILED_REPLY; return_error_param = ret; return_error_line = __LINE__; goto err_translate_failed; } - fp->pad_binder = 0; - binder_alloc_copy_to_buffer(&target_proc->alloc, - t->buffer, object_offset, - fp, sizeof(*fp)); } break; case BINDER_TYPE_FDA: { struct binder_object ptr_object; @@ -3396,15 +3419,16 @@ static void binder_transaction(struct binder_proc *proc, num_valid, last_fixup_obj_off, last_fixup_min_off); - if (ret < 0) { + if (ret < 0 || + binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, + object_offset, + bp, sizeof(*bp))) { return_error = BR_FAILED_REPLY; return_error_param = ret; return_error_line = __LINE__; goto err_translate_failed; } - binder_alloc_copy_to_buffer(&target_proc->alloc, - t->buffer, object_offset, - bp, sizeof(*bp)); last_fixup_obj_off = object_offset; last_fixup_min_off = 0; } break; @@ -4143,20 +4167,27 @@ static int binder_apply_fd_fixups(struct binder_proc *proc, trace_binder_transaction_fd_recv(t, fd, fixup->offset); fd_install(fd, fixup->file); fixup->file = NULL; - binder_alloc_copy_to_buffer(&proc->alloc, t->buffer, - fixup->offset, &fd, - sizeof(u32)); + if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer, + fixup->offset, &fd, + sizeof(u32))) { + ret = -EINVAL; + break; + } } list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) { if (fixup->file) { fput(fixup->file); } else if (ret) { u32 fd; - - binder_alloc_copy_from_buffer(&proc->alloc, &fd, - t->buffer, fixup->offset, - sizeof(fd)); - binder_deferred_fd_close(fd); + int err; + + err = binder_alloc_copy_from_buffer(&proc->alloc, &fd, + t->buffer, + fixup->offset, + sizeof(fd)); + WARN_ON(err); + if (!err) + binder_deferred_fd_close(fd); } list_del(&fixup->fixup_entry); kfree(fixup); diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index f04dc5976751..ebdf714d3f2d 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -1135,15 +1135,16 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, return 0; }
-static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc, - bool to_buffer, - struct binder_buffer *buffer, - binder_size_t buffer_offset, - void *ptr, - size_t bytes) +static int binder_alloc_do_buffer_copy(struct binder_alloc *alloc, + bool to_buffer, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + void *ptr, + size_t bytes) { /* All copies must be 32-bit aligned and 32-bit size */ - BUG_ON(!check_buffer(alloc, buffer, buffer_offset, bytes)); + if (!check_buffer(alloc, buffer, buffer_offset, bytes)) + return -EINVAL;
while (bytes) { unsigned long size; @@ -1171,24 +1172,25 @@ static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc, ptr = ptr + size; buffer_offset += size; } + return 0; }
-void binder_alloc_copy_to_buffer(struct binder_alloc *alloc, - struct binder_buffer *buffer, - binder_size_t buffer_offset, - void *src, - size_t bytes) +int binder_alloc_copy_to_buffer(struct binder_alloc *alloc, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + void *src, + size_t bytes) { - binder_alloc_do_buffer_copy(alloc, true, buffer, buffer_offset, - src, bytes); + return binder_alloc_do_buffer_copy(alloc, true, buffer, buffer_offset, + src, bytes); }
-void binder_alloc_copy_from_buffer(struct binder_alloc *alloc, - void *dest, - struct binder_buffer *buffer, - binder_size_t buffer_offset, - size_t bytes) +int binder_alloc_copy_from_buffer(struct binder_alloc *alloc, + void *dest, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + size_t bytes) { - binder_alloc_do_buffer_copy(alloc, false, buffer, buffer_offset, - dest, bytes); + return binder_alloc_do_buffer_copy(alloc, false, buffer, buffer_offset, + dest, bytes); } diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h index b60d161b7a7a..9a51b72624ae 100644 --- a/drivers/android/binder_alloc.h +++ b/drivers/android/binder_alloc.h @@ -168,17 +168,17 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, const void __user *from, size_t bytes);
-void binder_alloc_copy_to_buffer(struct binder_alloc *alloc, - struct binder_buffer *buffer, - binder_size_t buffer_offset, - void *src, - size_t bytes); - -void binder_alloc_copy_from_buffer(struct binder_alloc *alloc, - void *dest, - struct binder_buffer *buffer, - binder_size_t buffer_offset, - size_t bytes); +int binder_alloc_copy_to_buffer(struct binder_alloc *alloc, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + void *src, + size_t bytes); + +int binder_alloc_copy_from_buffer(struct binder_alloc *alloc, + void *dest, + struct binder_buffer *buffer, + binder_size_t buffer_offset, + size_t bytes);
#endif /* _LINUX_BINDER_ALLOC_H */
From: Martijn Coenen maco@android.com
mainline inclusion from mainline-v5.3-rc2 commit a56587065094fd96eb4c2b5ad65571daad32156d category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
In case the target node requests a security context, the extra_buffers_size is increased with the size of the security context. But, that size is not available for use by regular scatter-gather buffers; make sure the ending of that buffer is marked correctly.
Acked-by: Todd Kjos tkjos@google.com Fixes: ec74136ded79 ("binder: create node flag to request sender's security context") Signed-off-by: Martijn Coenen maco@android.com Cc: stable@vger.kernel.org # 5.1+ Link: https://lore.kernel.org/r/20190709110923.220736-1-maco@android.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 3dcf28177ce6..ebfe94a93371 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -3242,7 +3242,8 @@ static void binder_transaction(struct binder_proc *proc, buffer_offset = off_start_offset; off_end_offset = off_start_offset + tr->offsets_size; sg_buf_offset = ALIGN(off_end_offset, sizeof(void *)); - sg_buf_end_offset = sg_buf_offset + extra_buffers_size; + sg_buf_end_offset = sg_buf_offset + extra_buffers_size - + ALIGN(secctx_sz, sizeof(u64)); off_min = 0; for (buffer_offset = off_start_offset; buffer_offset < off_end_offset; buffer_offset += sizeof(binder_size_t)) {
From: Jann Horn jannh@google.com
mainline inclusion from mainline-v5.4-rc5 commit 45d02f79b539073b76077836871de6b674e36eb4 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
binder_mmap() tries to prevent the creation of overly big binder mappings by silently truncating the size of the VMA to 4MiB. However, this violates the API contract of mmap(). If userspace attempts to create a large binder VMA, and later attempts to unmap that VMA, it will call munmap() on a range beyond the end of the VMA, which may have been allocated to another VMA in the meantime. This can lead to userspace memory corruption.
The following sequence of calls leads to a segfault without this commit:
int main(void) { int binder_fd = open("/dev/binder", O_RDWR); if (binder_fd == -1) err(1, "open binder"); void *binder_mapping = mmap(NULL, 0x800000UL, PROT_READ, MAP_SHARED, binder_fd, 0); if (binder_mapping == MAP_FAILED) err(1, "mmap binder"); void *data_mapping = mmap(NULL, 0x400000UL, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); if (data_mapping == MAP_FAILED) err(1, "mmap data"); munmap(binder_mapping, 0x800000UL); *(char*)data_mapping = 1; return 0; }
Cc: stable@vger.kernel.org Signed-off-by: Jann Horn jannh@google.com Acked-by: Todd Kjos tkjos@google.com Acked-by: Christian Brauner christian.brauner@ubuntu.com Link: https://lore.kernel.org/r/20191016150119.154756-1-jannh@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 7 ------- drivers/android/binder_alloc.c | 6 ++++-- 2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index ebfe94a93371..b8000984514e 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -117,10 +117,6 @@ BINDER_DEBUG_ENTRY(proc); #define SZ_1K 0x400 #endif
-#ifndef SZ_4M -#define SZ_4M 0x400000 -#endif - #define FORBIDDEN_MMAP_FLAGS (VM_WRITE)
enum { @@ -5194,9 +5190,6 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma) if (proc->tsk != current->group_leader) return -EINVAL;
- if ((vma->vm_end - vma->vm_start) > SZ_4M) - vma->vm_end = vma->vm_start + SZ_4M; - binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d %lx-%lx (%ld K) vma %lx pagep %lx\n", __func__, proc->pid, vma->vm_start, vma->vm_end, diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index ebdf714d3f2d..868b9fbd9ee1 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -31,6 +31,7 @@ #include <asm/cacheflush.h> #include <linux/uaccess.h> #include <linux/highmem.h> +#include <linux/sizes.h> #include "binder_alloc.h" #include "binder_trace.h"
@@ -700,7 +701,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, alloc->buffer = (void __user *)vma->vm_start; mutex_unlock(&binder_alloc_mmap_lock);
- alloc->pages = kcalloc((vma->vm_end - vma->vm_start) / PAGE_SIZE, + alloc->buffer_size = min_t(unsigned long, vma->vm_end - vma->vm_start, + SZ_4M); + alloc->pages = kcalloc(alloc->buffer_size / PAGE_SIZE, sizeof(alloc->pages[0]), GFP_KERNEL); if (alloc->pages == NULL) { @@ -708,7 +711,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, failure_string = "alloc page array"; goto err_alloc_pages_failed; } - alloc->buffer_size = vma->vm_end - vma->vm_start;
buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); if (!buffer) {
From: Jann Horn jannh@google.com
mainline inclusion from mainline-v5.5-rc1 commit a7a74d7ff55a0c657bc46238b050460b9eacea95 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
binder_alloc_mmap_handler() attempts to detect the use of ->mmap() on a binder_proc whose binder_alloc has already been initialized by checking whether alloc->buffer is non-zero.
Before commit 880211667b20 ("binder: remove kernel vm_area for buffer space"), alloc->buffer was a kernel mapping address, which is always non-zero, but since that commit, it is a userspace mapping address.
A sufficiently privileged user can map /dev/binder at NULL, tricking binder_alloc_mmap_handler() into assuming that the binder_proc has not been mapped yet. This leads to memory unsafety. Luckily, no context on Android has such privileges, and on a typical Linux desktop system, you need to be root to do that.
Fix it by using the mapping size instead of the mapping address to distinguish the mapped case. A valid VMA can't have size zero.
Fixes: 880211667b20 ("binder: remove kernel vm_area for buffer space") Cc: stable@vger.kernel.org Signed-off-by: Jann Horn jannh@google.com Acked-by: Christian Brauner christian.brauner@ubuntu.com Link: https://lore.kernel.org/r/20191018205631.248274-2-jannh@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder_alloc.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 868b9fbd9ee1..af856205d40a 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -692,17 +692,17 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, struct binder_buffer *buffer;
mutex_lock(&binder_alloc_mmap_lock); - if (alloc->buffer) { + if (alloc->buffer_size) { ret = -EBUSY; failure_string = "already mapped"; goto err_already_mapped; } + alloc->buffer_size = min_t(unsigned long, vma->vm_end - vma->vm_start, + SZ_4M); + mutex_unlock(&binder_alloc_mmap_lock);
alloc->buffer = (void __user *)vma->vm_start; - mutex_unlock(&binder_alloc_mmap_lock);
- alloc->buffer_size = min_t(unsigned long, vma->vm_end - vma->vm_start, - SZ_4M); alloc->pages = kcalloc(alloc->buffer_size / PAGE_SIZE, sizeof(alloc->pages[0]), GFP_KERNEL); @@ -733,8 +733,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, kfree(alloc->pages); alloc->pages = NULL; err_alloc_pages_failed: - mutex_lock(&binder_alloc_mmap_lock); alloc->buffer = NULL; + mutex_lock(&binder_alloc_mmap_lock); + alloc->buffer_size = 0; err_already_mapped: mutex_unlock(&binder_alloc_mmap_lock); binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.5-rc2 commit 16981742717b04644a41052570fb502682a315d2 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
For BINDER_TYPE_PTR and BINDER_TYPE_FDA transactions, the num_valid local was calculated incorrectly causing the range check in binder_validate_ptr() to miss out-of-bounds offsets.
Fixes: bde4a19fc04f ("binder: use userspace pointer as base of buffer space") Signed-off-by: Todd Kjos tkjos@google.com Cc: stable stable@vger.kernel.org Link: https://lore.kernel.org/r/20191213202531.55010-1-tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index b8000984514e..b250ff3946d0 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -3335,7 +3335,7 @@ static void binder_transaction(struct binder_proc *proc, binder_size_t parent_offset; struct binder_fd_array_object *fda = to_binder_fd_array_object(hdr); - size_t num_valid = (buffer_offset - off_start_offset) * + size_t num_valid = (buffer_offset - off_start_offset) / sizeof(binder_size_t); struct binder_buffer_object *parent = binder_validate_ptr(target_proc, t->buffer, @@ -3409,7 +3409,7 @@ static void binder_transaction(struct binder_proc *proc, t->buffer->user_data + sg_buf_offset; sg_buf_offset += ALIGN(bp->length, sizeof(u64));
- num_valid = (buffer_offset - off_start_offset) * + num_valid = (buffer_offset - off_start_offset) / sizeof(binder_size_t); ret = binder_fixup_parent(t, thread, bp, off_start_offset,
From: Jann Horn jannh@google.com
mainline inclusion from mainline-v5.10-rc1 commit e8b8ae7ce32e17a5c29f0289e9e2a39c7dcaa1b8 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
While binder transactions with the same binder_proc as sender and recipient are forbidden, transactions with the same task_struct as sender and recipient are possible (even though currently there is a weird check in binder_transaction() that rejects them in the target==0 case). Therefore, task_struct identities can't be used to distinguish whether the caller is running in the context of the sender or the recipient.
Since I see no easy way to make this WARN_ON() useful and correct, let's just remove it.
Fixes: 44d8047f1d87 ("binder: use standard functions to allocate fds") Reported-by: syzbot+e113a0b970b7b3f394ba@syzkaller.appspotmail.com Acked-by: Christian Brauner christian.brauner@ubuntu.com Acked-by: Todd Kjos tkjos@google.com Signed-off-by: Jann Horn jannh@google.com Link: https://lore.kernel.org/r/20200806165359.2381483-1-jannh@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index b250ff3946d0..d8b9fde55465 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2374,8 +2374,6 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, * file is done when the transaction is torn * down. */ - WARN_ON(failed_at && - proc->tsk == current->group_leader); } break; case BINDER_TYPE_PTR: /*
From: Todd Kjos tkjos@google.com
stable inclusion from stable-v5.10.70 commit d5b0473707fa53b03a5db0256ce62b2874bddbc7 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit 5fdb55c1ac9585eb23bb2541d5819224429e103d upstream.
During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.
Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.
Fixes: 80cd795630d6 ("binder: fix use-after-free due to ksys_close() during fdget()") Cc: stable stable@vger.kernel.org Reviewed-by: Martijn Coenen maco@android.com Acked-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Todd Kjos tkjos@google.com Link: https://lore.kernel.org/r/20210830195146.587206-1-tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Conflicts: drivers/android/binder.c Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index d8b9fde55465..fb8be34c83cb 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2287,6 +2287,7 @@ 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, bool is_failure) @@ -2446,8 +2447,16 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, &proc->alloc, &fd, buffer, offset, sizeof(fd)); WARN_ON(err); - if (!err) + if (!err) { binder_deferred_fd_close(fd); + /* + * Need to make sure the thread goes + * back to userspace to complete the + * deferred close + */ + if (thread) + thread->looper_need_return = true; + } } } break; default: @@ -3504,7 +3513,7 @@ static void binder_transaction(struct binder_proc *proc, err_copy_data_failed: binder_free_txn_fixups(t); trace_binder_transaction_failed_buffer_release(t->buffer); - binder_transaction_buffer_release(target_proc, t->buffer, + binder_transaction_buffer_release(target_proc, NULL, t->buffer, buffer_offset, true); if (target_node) binder_dec_node_tmpref(target_node); @@ -3580,8 +3589,10 @@ static void binder_transaction(struct binder_proc *proc, * * Cleanup buffer and free it. */ -void -binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer) +static void +binder_free_buf(struct binder_proc *proc, + struct binder_thread *thread, + struct binder_buffer *buffer) { binder_inner_proc_lock(proc); if (buffer->transaction) { @@ -3609,7 +3620,7 @@ binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer) binder_node_inner_unlock(buf_node); } trace_binder_transaction_buffer_release(buffer); - binder_transaction_buffer_release(proc, buffer, 0, false); + binder_transaction_buffer_release(proc, thread, buffer, 0, false); binder_alloc_free_buf(&proc->alloc, buffer); }
@@ -3803,7 +3814,7 @@ static int binder_thread_write(struct binder_proc *proc, proc->pid, thread->pid, (u64)data_ptr, buffer->debug_id, buffer->transaction ? "active" : "finished"); - binder_free_buf(proc, buffer); + binder_free_buf(proc, thread, buffer); break; }
@@ -4486,7 +4497,7 @@ static int binder_thread_read(struct binder_proc *proc, buffer->transaction = NULL; binder_cleanup_transaction(t, "fd fixups failed", BR_FAILED_REPLY); - binder_free_buf(proc, buffer); + binder_free_buf(proc, thread, buffer); binder_debug(BINDER_DEBUG_FAILED_TRANSACTION, "%d:%d %stransaction %d fd fixups failed %d/%d, line %d\n", proc->pid, thread->pid,
From: Todd Kjos tkjos@google.com
stable inclusion from stable-v5.10.79 commit 07d1db141e478917600fa32be11e5b828ff9ed13 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit 32e9f56a96d8d0f23cb2aeb2a3cd18d40393e787 upstream.
When freeing txn buffers, binder_transaction_buffer_release() attempts to detect whether the current context is the target by comparing current->group_leader to proc->tsk. This is an unreliable test. Instead explicitly pass an 'is_failure' boolean.
Detecting the sender was being used as a way to tell if the transaction failed to be sent. When cleaning up after failing to send a transaction, there is no need to close the fds associated with a BINDER_TYPE_FDA object. Now 'is_failure' can be used to accurately detect this case.
Fixes: 44d8047f1d87 ("binder: use standard functions to allocate fds") Cc: stable stable@vger.kernel.org Acked-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Todd Kjos tkjos@google.com Link: https://lore.kernel.org/r/20211015233811.3532235-1-tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index fb8be34c83cb..656593c0e9b1 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2305,7 +2305,7 @@ 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 : + 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)) { @@ -2391,9 +2391,8 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, binder_size_t fd_buf_size; binder_size_t num_valid;
- if (proc->tsk != current->group_leader) { + if (is_failure) { /* - * Nothing to do if running in sender context * The fd fixups have not been applied so no * fds need to be closed. */ @@ -3583,6 +3582,7 @@ static void binder_transaction(struct binder_proc *proc, * binder_free_buf() - free the specified buffer * @proc: binder proc that owns buffer * @buffer: buffer to be freed + * @is_failure: failed to send transaction * * If buffer for an async transaction, enqueue the next async * transaction from the node. @@ -3592,7 +3592,7 @@ static void binder_transaction(struct binder_proc *proc, static void binder_free_buf(struct binder_proc *proc, struct binder_thread *thread, - struct binder_buffer *buffer) + struct binder_buffer *buffer, bool is_failure) { binder_inner_proc_lock(proc); if (buffer->transaction) { @@ -3620,7 +3620,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, false); + binder_transaction_buffer_release(proc, thread, buffer, 0, is_failure); binder_alloc_free_buf(&proc->alloc, buffer); }
@@ -3814,7 +3814,7 @@ static int binder_thread_write(struct binder_proc *proc, proc->pid, thread->pid, (u64)data_ptr, buffer->debug_id, buffer->transaction ? "active" : "finished"); - binder_free_buf(proc, thread, buffer); + binder_free_buf(proc, thread, buffer, false); break; }
@@ -4497,7 +4497,7 @@ static int binder_thread_read(struct binder_proc *proc, buffer->transaction = NULL; binder_cleanup_transaction(t, "fd fixups failed", BR_FAILED_REPLY); - binder_free_buf(proc, thread, buffer); + binder_free_buf(proc, thread, buffer, true); binder_debug(BINDER_DEBUG_FAILED_TRANSACTION, "%d:%d %stransaction %d fd fixups failed %d/%d, line %d\n", proc->pid, thread->pid,
From: Todd Kjos tkjos@google.com
stable inclusion from stable-v5.10.80 commit afbec52fbce006a775edb21f87ccae713bc0e7d6 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit 4d5b5539742d2554591751b4248b0204d20dcc9d upstream.
Use the 'struct cred' saved at binder_open() to lookup the security ID via security_cred_getsecid(). This ensures that the security context that opened binder is the one used to generate the secctx.
Cc: stable@vger.kernel.org # 5.4+ Fixes: ec74136ded79 ("binder: create node flag to request sender's security context") Signed-off-by: Todd Kjos tkjos@google.com Suggested-by: Stephen Smalley stephen.smalley.work@gmail.com Reported-by: kernel test robot lkp@intel.com Acked-by: Casey Schaufler casey@schaufler-ca.com Signed-off-by: Paul Moore paul@paul-moore.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 2 +- include/linux/security.h | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 656593c0e9b1..c07e0da4b42d 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -3138,7 +3138,7 @@ static void binder_transaction(struct binder_proc *proc, u32 secid; size_t added_size;
- security_task_getsecid(proc->tsk, &secid); + security_cred_getsecid(proc->cred, &secid); ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); if (ret) { return_error = BR_FAILED_REPLY; diff --git a/include/linux/security.h b/include/linux/security.h index 2960258279cb..016a88f557e8 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -914,6 +914,11 @@ static inline void security_transfer_creds(struct cred *new, { }
+static inline void security_cred_getsecid(const struct cred *c, u32 *secid) +{ + *secid = 0; +} + static inline int security_kernel_act_as(struct cred *cred, u32 secid) { return 0;
From: Todd Kjos tkjos@google.com
stable inclusion from stable-v5.10.94 commit 551a785c26f6ff41cccd527e7bd9f032f91332c2 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
[ Upstream commit fe6b1869243f23a485a106c214bcfdc7aa0ed593 ]
If a memory copy function fails to copy the whole buffer, a positive integar with the remaining bytes is returned. In binder_translate_fd_array() this can result in an fd being skipped due to the failed copy, but the loop continues processing fds since the early return condition expects a negative integer on error.
Fix by returning "ret > 0 ? -EINVAL : ret" to handle this case.
Fixes: bb4a2e48d510 ("binder: return errors from buffer copy functions") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Acked-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Todd Kjos tkjos@google.com Link: https://lore.kernel.org/r/20211130185152.437403-2-tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Sasha Levin sashal@kernel.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index c07e0da4b42d..bc9422423872 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2700,8 +2700,8 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, if (!ret) ret = binder_translate_fd(fd, offset, t, thread, in_reply_to); - if (ret < 0) - return ret; + if (ret) + return ret > 0 ? -EINVAL : ret; } return 0; }
From: Todd Kjos tkjos@google.com
stable inclusion from stable-v5.10.157 commit 23e9d815fad84c1bee3742a8de4bd39510435362 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit 6d98eb95b450a75adb4516a1d33652dc78d2b20c upstream.
Transactions are copied from the sender to the target first and objects like BINDER_TYPE_PTR and BINDER_TYPE_FDA are then fixed up. This means there is a short period where the sender's version of these objects are visible to the target prior to the fixups.
Instead of copying all of the data first, copy data only after any needed fixups have been applied.
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver") Reviewed-by: Martijn Coenen maco@android.com Acked-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Todd Kjos tkjos@google.com Link: https://lore.kernel.org/r/20211130185152.437403-3-tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org [cmllamas: fix trivial merge conflict] Signed-off-by: Carlos Llamas cmllamas@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Conflicts: drivers/android/binder.c Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 94 ++++++++++++++++++++++++++++++---------- 1 file changed, 70 insertions(+), 24 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index bc9422423872..eab3afc0125a 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2043,15 +2043,21 @@ static void binder_cleanup_transaction(struct binder_transaction *t, /** * binder_get_object() - gets object and checks for valid metadata * @proc: binder_proc owning the buffer + * @u: sender's user pointer to base of buffer * @buffer: binder_buffer that we're parsing. * @offset: offset in the @buffer at which to validate an object. * @object: struct binder_object to read into * - * Return: If there's a valid metadata object at @offset in @buffer, the + * Copy the binder object at the given offset into @object. If @u is + * provided then the copy is from the sender's buffer. If not, then + * it is copied from the target's @buffer. + * + * Return: If there's a valid metadata object at @offset, the * size of that object. Otherwise, it returns zero. The object * is read into the struct binder_object pointed to by @object. */ static size_t binder_get_object(struct binder_proc *proc, + const void __user *u, struct binder_buffer *buffer, unsigned long offset, struct binder_object *object) @@ -2061,10 +2067,16 @@ static size_t binder_get_object(struct binder_proc *proc, size_t object_size = 0;
read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset); - if (offset > buffer->data_size || read_size < sizeof(*hdr) || - binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, - offset, read_size)) + if (offset > buffer->data_size || read_size < sizeof(*hdr)) return 0; + if (u) { + if (copy_from_user(object, u + offset, read_size)) + return 0; + } else { + if (binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, + offset, read_size)) + return 0; + }
/* Ok, now see if we read a complete object. */ hdr = &object->hdr; @@ -2137,7 +2149,7 @@ static struct binder_buffer_object *binder_validate_ptr( b, buffer_offset, sizeof(object_offset))) return NULL; - object_size = binder_get_object(proc, b, object_offset, object); + object_size = binder_get_object(proc, NULL, b, object_offset, object); if (!object_size || object->hdr.type != BINDER_TYPE_PTR) return NULL; if (object_offsetp) @@ -2202,7 +2214,8 @@ static bool binder_validate_fixup(struct binder_proc *proc, unsigned long buffer_offset; struct binder_object last_object; struct binder_buffer_object *last_bbo; - size_t object_size = binder_get_object(proc, b, last_obj_offset, + size_t object_size = binder_get_object(proc, NULL, b, + last_obj_offset, &last_object); if (object_size != sizeof(*last_bbo)) return false; @@ -2317,7 +2330,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, if (!binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, buffer, buffer_offset, sizeof(object_offset))) - object_size = binder_get_object(proc, buffer, + object_size = binder_get_object(proc, NULL, buffer, object_offset, &object); if (object_size == 0) { pr_err("transaction release %d bad object at offset %lld, size %zd\n", @@ -2880,6 +2893,7 @@ static void binder_transaction(struct binder_proc *proc, binder_size_t off_start_offset, off_end_offset; binder_size_t off_min; binder_size_t sg_buf_offset, sg_buf_end_offset; + binder_size_t user_offset = 0; struct binder_proc *target_proc = NULL; struct binder_thread *target_thread = NULL; struct binder_node *target_node = NULL; @@ -2894,6 +2908,8 @@ static void binder_transaction(struct binder_proc *proc, int t_debug_id = atomic_inc_return(&binder_last_id); char *secctx = NULL; u32 secctx_sz = 0; + const void __user *user_buffer = (const void __user *) + (uintptr_t)tr->data.ptr.buffer;
e = binder_transaction_log_add(&binder_transaction_log); e->debug_id = t_debug_id; @@ -3196,19 +3212,6 @@ static void binder_transaction(struct binder_proc *proc, t->buffer->target_node = target_node; trace_binder_transaction_alloc_buf(t->buffer);
- if (binder_alloc_copy_user_to_buffer( - &target_proc->alloc, - t->buffer, 0, - (const void __user *) - (uintptr_t)tr->data.ptr.buffer, - tr->data_size)) { - binder_user_error("%d:%d got transaction with invalid data ptr\n", - proc->pid, thread->pid); - return_error = BR_FAILED_REPLY; - return_error_param = -EFAULT; - return_error_line = __LINE__; - goto err_copy_data_failed; - } if (binder_alloc_copy_user_to_buffer( &target_proc->alloc, t->buffer, @@ -3253,6 +3256,7 @@ static void binder_transaction(struct binder_proc *proc, size_t object_size; struct binder_object object; binder_size_t object_offset; + binder_size_t copy_size;
if (binder_alloc_copy_from_buffer(&target_proc->alloc, &object_offset, @@ -3264,8 +3268,27 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_bad_offset; } - object_size = binder_get_object(target_proc, t->buffer, - object_offset, &object); + + /* + * Copy the source user buffer up to the next object + * that will be processed. + */ + copy_size = object_offset - user_offset; + if (copy_size && (user_offset > object_offset || + binder_alloc_copy_user_to_buffer( + &target_proc->alloc, + t->buffer, user_offset, + user_buffer + user_offset, + copy_size))) { + binder_user_error("%d:%d got transaction with invalid data ptr\n", + proc->pid, thread->pid); + return_error = BR_FAILED_REPLY; + return_error_param = -EFAULT; + return_error_line = __LINE__; + goto err_copy_data_failed; + } + object_size = binder_get_object(target_proc, user_buffer, + t->buffer, object_offset, &object); if (object_size == 0 || object_offset < off_min) { binder_user_error("%d:%d got transaction with invalid offset (%lld, min %lld max %lld) or object.\n", proc->pid, thread->pid, @@ -3277,6 +3300,11 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_bad_offset; } + /* + * Set offset to the next buffer fragment to be + * copied + */ + user_offset = object_offset + object_size;
hdr = &object.hdr; off_min = object_offset + object_size; @@ -3372,9 +3400,14 @@ static void binder_transaction(struct binder_proc *proc, } ret = binder_translate_fd_array(fda, parent, t, thread, in_reply_to); - if (ret < 0) { + if (!ret) + ret = binder_alloc_copy_to_buffer(&target_proc->alloc, + t->buffer, + object_offset, + fda, sizeof(*fda)); + if (ret) { return_error = BR_FAILED_REPLY; - return_error_param = ret; + return_error_param = ret > 0 ? -EINVAL : ret; return_error_line = __LINE__; goto err_translate_failed; } @@ -3444,6 +3477,19 @@ static void binder_transaction(struct binder_proc *proc, goto err_bad_object_type; } } + /* Done processing objects, copy the rest of the buffer */ + if (binder_alloc_copy_user_to_buffer( + &target_proc->alloc, + t->buffer, user_offset, + user_buffer + user_offset, + tr->data_size - user_offset)) { + binder_user_error("%d:%d got transaction with invalid data ptr\n", + proc->pid, thread->pid); + return_error = BR_FAILED_REPLY; + return_error_param = -EFAULT; + return_error_line = __LINE__; + goto err_copy_data_failed; + } tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE; t->work.type = BINDER_WORK_TRANSACTION;
From: Todd Kjos tkjos@google.com
stable inclusion from stable-v5.10.157 commit 5204296fc76623552d53f042e2dc411b49c151f2 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit 656e01f3ab54afe71bed066996fc2640881e1220 upstream.
This patch is to prepare for an up coming patch where we read pre-translated fds from the sender buffer and translate them before copying them to the target. It does not change run time.
The patch adds two new parameters to binder_translate_fd_array() to hold the sender buffer and sender buffer parent. These parameters let us call copy_from_user() directly from the sender instead of using binder_alloc_copy_from_buffer() to copy from the target. Also the patch adds some new alignment checks. Previously the alignment checks would have been done in a different place, but this lets us print more useful error messages.
Reviewed-by: Martijn Coenen maco@android.com Acked-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Todd Kjos tkjos@google.com Link: https://lore.kernel.org/r/20211130185152.437403-4-tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Carlos Llamas cmllamas@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 39 ++++++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index eab3afc0125a..9a3fb08018b3 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2665,15 +2665,17 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, }
static int binder_translate_fd_array(struct binder_fd_array_object *fda, + const void __user *sender_ubuffer, struct binder_buffer_object *parent, + struct binder_buffer_object *sender_uparent, struct binder_transaction *t, struct binder_thread *thread, struct binder_transaction *in_reply_to) { binder_size_t fdi, fd_buf_size; binder_size_t fda_offset; + const void __user *sender_ufda_base; struct binder_proc *proc = thread->proc; - struct binder_proc *target_proc = t->to_proc;
fd_buf_size = sizeof(u32) * fda->num_fds; if (fda->num_fds >= SIZE_MAX / sizeof(u32)) { @@ -2697,7 +2699,10 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, */ fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) + fda->parent_offset; - if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32))) { + sender_ufda_base = (void __user *)sender_uparent->buffer + fda->parent_offset; + + if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32)) || + !IS_ALIGNED((unsigned long)sender_ufda_base, sizeof(u32))) { binder_user_error("%d:%d parent offset not aligned correctly.\n", proc->pid, thread->pid); return -EINVAL; @@ -2706,10 +2711,9 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, u32 fd; int ret; binder_size_t offset = fda_offset + fdi * sizeof(fd); + binder_size_t sender_uoffset = fdi * sizeof(fd);
- ret = binder_alloc_copy_from_buffer(&target_proc->alloc, - &fd, t->buffer, - offset, sizeof(fd)); + ret = copy_from_user(&fd, sender_ufda_base + sender_uoffset, sizeof(fd)); if (!ret) ret = binder_translate_fd(fd, offset, t, thread, in_reply_to); @@ -3367,6 +3371,8 @@ static void binder_transaction(struct binder_proc *proc, case BINDER_TYPE_FDA: { struct binder_object ptr_object; binder_size_t parent_offset; + struct binder_object user_object; + size_t user_parent_size; struct binder_fd_array_object *fda = to_binder_fd_array_object(hdr); size_t num_valid = (buffer_offset - off_start_offset) / @@ -3398,8 +3404,27 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_bad_parent; } - ret = binder_translate_fd_array(fda, parent, t, thread, - in_reply_to); + /* + * We need to read the user version of the parent + * object to get the original user offset + */ + user_parent_size = + binder_get_object(proc, user_buffer, t->buffer, + parent_offset, &user_object); + if (user_parent_size != sizeof(user_object.bbo)) { + binder_user_error("%d:%d invalid ptr object size: %zd vs %zd\n", + proc->pid, thread->pid, + user_parent_size, + sizeof(user_object.bbo)); + return_error = BR_FAILED_REPLY; + return_error_param = -EINVAL; + return_error_line = __LINE__; + goto err_bad_parent; + } + ret = binder_translate_fd_array(fda, user_buffer, + parent, + &user_object.bbo, t, + thread, in_reply_to); if (!ret) ret = binder_alloc_copy_to_buffer(&target_proc->alloc, t->buffer,
From: Todd Kjos tkjos@google.com
stable inclusion from stable-v5.10.157 commit c9d3f25a7f4e3aab3dfd91885e3d428bccdcb0e1 category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit 09184ae9b5756cc469db6fd1d1cfdcffbf627c2d upstream.
BINDER_TYPE_PTR objects point to memory areas in the source process to be copied into the target buffer as part of a transaction. This implements a scatter- gather model where non-contiguous memory in a source process is "gathered" into a contiguous region in the target buffer.
The data can include pointers that must be fixed up to correctly point to the copied data. To avoid making source process pointers visible to the target process, this patch defers the copy until the fixups are known and then copies and fixeups are done together.
There is a special case of BINDER_TYPE_FDA which applies the fixup later in the target process context. In this case the user data is skipped (so no untranslated fds become visible to the target).
Reviewed-by: Martijn Coenen maco@android.com Signed-off-by: Todd Kjos tkjos@google.com Link: https://lore.kernel.org/r/20211130185152.437403-5-tkjos@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org [cmllamas: fix trivial merge conflict] Signed-off-by: Carlos Llamas cmllamas@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 299 +++++++++++++++++++++++++++++++++++---- 1 file changed, 274 insertions(+), 25 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 9a3fb08018b3..2786de108dcf 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2664,7 +2664,246 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, return ret; }
-static int binder_translate_fd_array(struct binder_fd_array_object *fda, +/** + * struct binder_ptr_fixup - data to be fixed-up in target buffer + * @offset offset in target buffer to fixup + * @skip_size bytes to skip in copy (fixup will be written later) + * @fixup_data data to write at fixup offset + * @node list node + * + * This is used for the pointer fixup list (pf) which is created and consumed + * during binder_transaction() and is only accessed locally. No + * locking is necessary. + * + * The list is ordered by @offset. + */ +struct binder_ptr_fixup { + binder_size_t offset; + size_t skip_size; + binder_uintptr_t fixup_data; + struct list_head node; +}; + +/** + * struct binder_sg_copy - scatter-gather data to be copied + * @offset offset in target buffer + * @sender_uaddr user address in source buffer + * @length bytes to copy + * @node list node + * + * This is used for the sg copy list (sgc) which is created and consumed + * during binder_transaction() and is only accessed locally. No + * locking is necessary. + * + * The list is ordered by @offset. + */ +struct binder_sg_copy { + binder_size_t offset; + const void __user *sender_uaddr; + size_t length; + struct list_head node; +}; + +/** + * binder_do_deferred_txn_copies() - copy and fixup scatter-gather data + * @alloc: binder_alloc associated with @buffer + * @buffer: binder buffer in target process + * @sgc_head: list_head of scatter-gather copy list + * @pf_head: list_head of pointer fixup list + * + * Processes all elements of @sgc_head, applying fixups from @pf_head + * and copying the scatter-gather data from the source process' user + * buffer to the target's buffer. It is expected that the list creation + * and processing all occurs during binder_transaction() so these lists + * are only accessed in local context. + * + * Return: 0=success, else -errno + */ +static int binder_do_deferred_txn_copies(struct binder_alloc *alloc, + struct binder_buffer *buffer, + struct list_head *sgc_head, + struct list_head *pf_head) +{ + int ret = 0; + struct binder_sg_copy *sgc, *tmpsgc; + struct binder_ptr_fixup *pf = + list_first_entry_or_null(pf_head, struct binder_ptr_fixup, + node); + + list_for_each_entry_safe(sgc, tmpsgc, sgc_head, node) { + size_t bytes_copied = 0; + + while (bytes_copied < sgc->length) { + size_t copy_size; + size_t bytes_left = sgc->length - bytes_copied; + size_t offset = sgc->offset + bytes_copied; + + /* + * We copy up to the fixup (pointed to by pf) + */ + copy_size = pf ? min(bytes_left, (size_t)pf->offset - offset) + : bytes_left; + if (!ret && copy_size) + ret = binder_alloc_copy_user_to_buffer( + alloc, buffer, + offset, + sgc->sender_uaddr + bytes_copied, + copy_size); + bytes_copied += copy_size; + if (copy_size != bytes_left) { + BUG_ON(!pf); + /* we stopped at a fixup offset */ + if (pf->skip_size) { + /* + * we are just skipping. This is for + * BINDER_TYPE_FDA where the translated + * fds will be fixed up when we get + * to target context. + */ + bytes_copied += pf->skip_size; + } else { + /* apply the fixup indicated by pf */ + if (!ret) + ret = binder_alloc_copy_to_buffer( + alloc, buffer, + pf->offset, + &pf->fixup_data, + sizeof(pf->fixup_data)); + bytes_copied += sizeof(pf->fixup_data); + } + list_del(&pf->node); + kfree(pf); + pf = list_first_entry_or_null(pf_head, + struct binder_ptr_fixup, node); + } + } + list_del(&sgc->node); + kfree(sgc); + } + BUG_ON(!list_empty(pf_head)); + BUG_ON(!list_empty(sgc_head)); + + return ret > 0 ? -EINVAL : ret; +} + +/** + * binder_cleanup_deferred_txn_lists() - free specified lists + * @sgc_head: list_head of scatter-gather copy list + * @pf_head: list_head of pointer fixup list + * + * Called to clean up @sgc_head and @pf_head if there is an + * error. + */ +static void binder_cleanup_deferred_txn_lists(struct list_head *sgc_head, + struct list_head *pf_head) +{ + struct binder_sg_copy *sgc, *tmpsgc; + struct binder_ptr_fixup *pf, *tmppf; + + list_for_each_entry_safe(sgc, tmpsgc, sgc_head, node) { + list_del(&sgc->node); + kfree(sgc); + } + list_for_each_entry_safe(pf, tmppf, pf_head, node) { + list_del(&pf->node); + kfree(pf); + } +} + +/** + * binder_defer_copy() - queue a scatter-gather buffer for copy + * @sgc_head: list_head of scatter-gather copy list + * @offset: binder buffer offset in target process + * @sender_uaddr: user address in source process + * @length: bytes to copy + * + * Specify a scatter-gather block to be copied. The actual copy must + * be deferred until all the needed fixups are identified and queued. + * Then the copy and fixups are done together so un-translated values + * from the source are never visible in the target buffer. + * + * We are guaranteed that repeated calls to this function will have + * monotonically increasing @offset values so the list will naturally + * be ordered. + * + * Return: 0=success, else -errno + */ +static int binder_defer_copy(struct list_head *sgc_head, binder_size_t offset, + const void __user *sender_uaddr, size_t length) +{ + struct binder_sg_copy *bc = kzalloc(sizeof(*bc), GFP_KERNEL); + + if (!bc) + return -ENOMEM; + + bc->offset = offset; + bc->sender_uaddr = sender_uaddr; + bc->length = length; + INIT_LIST_HEAD(&bc->node); + + /* + * We are guaranteed that the deferred copies are in-order + * so just add to the tail. + */ + list_add_tail(&bc->node, sgc_head); + + return 0; +} + +/** + * binder_add_fixup() - queue a fixup to be applied to sg copy + * @pf_head: list_head of binder ptr fixup list + * @offset: binder buffer offset in target process + * @fixup: bytes to be copied for fixup + * @skip_size: bytes to skip when copying (fixup will be applied later) + * + * Add the specified fixup to a list ordered by @offset. When copying + * the scatter-gather buffers, the fixup will be copied instead of + * data from the source buffer. For BINDER_TYPE_FDA fixups, the fixup + * will be applied later (in target process context), so we just skip + * the bytes specified by @skip_size. If @skip_size is 0, we copy the + * value in @fixup. + * + * This function is called *mostly* in @offset order, but there are + * exceptions. Since out-of-order inserts are relatively uncommon, + * we insert the new element by searching backward from the tail of + * the list. + * + * Return: 0=success, else -errno + */ +static int binder_add_fixup(struct list_head *pf_head, binder_size_t offset, + binder_uintptr_t fixup, size_t skip_size) +{ + struct binder_ptr_fixup *pf = kzalloc(sizeof(*pf), GFP_KERNEL); + struct binder_ptr_fixup *tmppf; + + if (!pf) + return -ENOMEM; + + pf->offset = offset; + pf->fixup_data = fixup; + pf->skip_size = skip_size; + INIT_LIST_HEAD(&pf->node); + + /* Fixups are *mostly* added in-order, but there are some + * exceptions. Look backwards through list for insertion point. + */ + list_for_each_entry_reverse(tmppf, pf_head, node) { + if (tmppf->offset < pf->offset) { + list_add(&pf->node, &tmppf->node); + return 0; + } + } + /* + * if we get here, then the new offset is the lowest so + * insert at the head + */ + list_add(&pf->node, pf_head); + return 0; +} + +static int binder_translate_fd_array(struct list_head *pf_head, + struct binder_fd_array_object *fda, const void __user *sender_ubuffer, struct binder_buffer_object *parent, struct binder_buffer_object *sender_uparent, @@ -2676,6 +2915,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, binder_size_t fda_offset; const void __user *sender_ufda_base; struct binder_proc *proc = thread->proc; + int ret;
fd_buf_size = sizeof(u32) * fda->num_fds; if (fda->num_fds >= SIZE_MAX / sizeof(u32)) { @@ -2707,9 +2947,12 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, proc->pid, thread->pid); return -EINVAL; } + ret = binder_add_fixup(pf_head, fda_offset, 0, fda->num_fds * sizeof(u32)); + if (ret) + return ret; + for (fdi = 0; fdi < fda->num_fds; fdi++) { u32 fd; - int ret; binder_size_t offset = fda_offset + fdi * sizeof(fd); binder_size_t sender_uoffset = fdi * sizeof(fd);
@@ -2723,7 +2966,8 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, return 0; }
-static int binder_fixup_parent(struct binder_transaction *t, +static int binder_fixup_parent(struct list_head *pf_head, + struct binder_transaction *t, struct binder_thread *thread, struct binder_buffer_object *bp, binder_size_t off_start_offset, @@ -2769,14 +3013,7 @@ static int binder_fixup_parent(struct binder_transaction *t, } buffer_offset = bp->parent_offset + (uintptr_t)parent->buffer - (uintptr_t)b->user_data; - if (binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset, - &bp->buffer, sizeof(bp->buffer))) { - binder_user_error("%d:%d got transaction with invalid parent offset\n", - proc->pid, thread->pid); - return -EINVAL; - } - - return 0; + return binder_add_fixup(pf_head, buffer_offset, bp->buffer, 0); }
/** @@ -2912,8 +3149,12 @@ static void binder_transaction(struct binder_proc *proc, int t_debug_id = atomic_inc_return(&binder_last_id); char *secctx = NULL; u32 secctx_sz = 0; + struct list_head sgc_head; + struct list_head pf_head; const void __user *user_buffer = (const void __user *) (uintptr_t)tr->data.ptr.buffer; + INIT_LIST_HEAD(&sgc_head); + INIT_LIST_HEAD(&pf_head);
e = binder_transaction_log_add(&binder_transaction_log); e->debug_id = t_debug_id; @@ -3421,8 +3662,8 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_bad_parent; } - ret = binder_translate_fd_array(fda, user_buffer, - parent, + ret = binder_translate_fd_array(&pf_head, fda, + user_buffer, parent, &user_object.bbo, t, thread, in_reply_to); if (!ret) @@ -3454,19 +3695,14 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_bad_offset; } - if (binder_alloc_copy_user_to_buffer( - &target_proc->alloc, - t->buffer, - sg_buf_offset, - (const void __user *) - (uintptr_t)bp->buffer, - bp->length)) { - binder_user_error("%d:%d got transaction with invalid offsets ptr\n", - proc->pid, thread->pid); - return_error_param = -EFAULT; + ret = binder_defer_copy(&sgc_head, sg_buf_offset, + (const void __user *)(uintptr_t)bp->buffer, + bp->length); + if (ret) { return_error = BR_FAILED_REPLY; + return_error_param = ret; return_error_line = __LINE__; - goto err_copy_data_failed; + goto err_translate_failed; } /* Fixup buffer pointer to target proc address space */ bp->buffer = (uintptr_t) @@ -3475,7 +3711,8 @@ static void binder_transaction(struct binder_proc *proc,
num_valid = (buffer_offset - off_start_offset) / sizeof(binder_size_t); - ret = binder_fixup_parent(t, thread, bp, + ret = binder_fixup_parent(&pf_head, t, + thread, bp, off_start_offset, num_valid, last_fixup_obj_off, @@ -3515,6 +3752,17 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_copy_data_failed; } + + ret = binder_do_deferred_txn_copies(&target_proc->alloc, t->buffer, + &sgc_head, &pf_head); + if (ret) { + binder_user_error("%d:%d got transaction with invalid offsets ptr\n", + proc->pid, thread->pid); + return_error = BR_FAILED_REPLY; + return_error_param = ret; + return_error_line = __LINE__; + goto err_copy_data_failed; + } tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE; t->work.type = BINDER_WORK_TRANSACTION;
@@ -3581,6 +3829,7 @@ static void binder_transaction(struct binder_proc *proc, err_bad_offset: err_bad_parent: err_copy_data_failed: + binder_cleanup_deferred_txn_lists(&sgc_head, &pf_head); binder_free_txn_fixups(t); trace_binder_transaction_failed_buffer_release(t->buffer); binder_transaction_buffer_release(target_proc, NULL, t->buffer,
From: Arnd Bergmann arnd@arndb.de
stable inclusion from stable-v5.10.157 commit 2e3c27f24173c6f3d799080da82126fa044a2f5e category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit 9a0a930fe2535a76ad70d3f43caeccf0d86a3009 upstream.
binder_uintptr_t is not the same as uintptr_t, so converting it into a pointer requires a second cast:
drivers/android/binder.c: In function 'binder_translate_fd_array': drivers/android/binder.c:2511:28: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] 2511 | sender_ufda_base = (void __user *)sender_uparent->buffer + fda->parent_offset; | ^
Fixes: 656e01f3ab54 ("binder: read pre-translated fds from sender buffer") Acked-by: Todd Kjos tkjos@google.com Acked-by: Randy Dunlap rdunlap@infradead.org # build-tested Acked-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Arnd Bergmann arnd@arndb.de Link: https://lore.kernel.org/r/20211207122448.1185769-1-arnd@kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Carlos Llamas cmllamas@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 2786de108dcf..ef76905a7f46 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2939,7 +2939,8 @@ static int binder_translate_fd_array(struct list_head *pf_head, */ fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) + fda->parent_offset; - sender_ufda_base = (void __user *)sender_uparent->buffer + fda->parent_offset; + sender_ufda_base = (void __user *)(uintptr_t)sender_uparent->buffer + + fda->parent_offset;
if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32)) || !IS_ALIGNED((unsigned long)sender_ufda_base, sizeof(u32))) {
From: Alessandro Astone ales.astone@gmail.com
stable inclusion from stable-v5.10.157 commit 017de842533f4334d646f1d480f591f4ca9f5c7a category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit 2d1746e3fda0c3612143d7c06f8e1d1830c13e23 upstream.
When handling BINDER_TYPE_FDA object we are pushing a parent fixup with a certain skip_size but no scatter-gather copy object, since the copy is handled standalone. If BINDER_TYPE_FDA is the last children the scatter-gather copy loop will never stop to skip it, thus we are left with an item in the parent fixup list. This will trigger the BUG_ON().
This is reproducible in android when playing a video. We receive a transaction that looks like this: obj[0] BINDER_TYPE_PTR, parent obj[1] BINDER_TYPE_PTR, child obj[2] BINDER_TYPE_PTR, child obj[3] BINDER_TYPE_FDA, child
Fixes: 09184ae9b575 ("binder: defer copies of pre-patched txn data") Acked-by: Todd Kjos tkjos@google.com Cc: stable stable@kernel.org Signed-off-by: Alessandro Astone ales.astone@gmail.com Link: https://lore.kernel.org/r/20220415120015.52684-2-ales.astone@gmail.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Carlos Llamas cmllamas@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index ef76905a7f46..512cf08e2c07 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2726,6 +2726,7 @@ static int binder_do_deferred_txn_copies(struct binder_alloc *alloc, { int ret = 0; struct binder_sg_copy *sgc, *tmpsgc; + struct binder_ptr_fixup *tmppf; struct binder_ptr_fixup *pf = list_first_entry_or_null(pf_head, struct binder_ptr_fixup, node); @@ -2780,7 +2781,11 @@ static int binder_do_deferred_txn_copies(struct binder_alloc *alloc, list_del(&sgc->node); kfree(sgc); } - BUG_ON(!list_empty(pf_head)); + list_for_each_entry_safe(pf, tmppf, pf_head, node) { + BUG_ON(pf->skip_size == 0); + list_del(&pf->node); + kfree(pf); + } BUG_ON(!list_empty(sgc_head));
return ret > 0 ? -EINVAL : ret;
From: Alessandro Astone ales.astone@gmail.com
stable inclusion from stable-v5.10.157 commit ae9e0cc973fb7499ea1b1a8dfd0795f728b84faf category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
--------------------------------
commit ef38de9217a04c9077629a24652689d8fdb4c6c6 upstream.
Some android userspace is sending BINDER_TYPE_FDA objects with num_fds=0. Like the previous patch, this is reproducible when playing a video.
Before commit 09184ae9b575 BINDER_TYPE_FDA objects with num_fds=0 were 'correctly handled', as in no fixup was performed.
After commit 09184ae9b575 we aggregate fixup and skip regions in binder_ptr_fixup structs and distinguish between the two by using the skip_size field: if it's 0, then it's a fixup, otherwise skip. When processing BINDER_TYPE_FDA objects with num_fds=0 we add a skip region of skip_size=0, and this causes issues because now binder_do_deferred_txn_copies will think this was a fixup region.
To address that, return early from binder_translate_fd_array to avoid adding an empty skip region.
Fixes: 09184ae9b575 ("binder: defer copies of pre-patched txn data") Acked-by: Todd Kjos tkjos@google.com Cc: stable stable@kernel.org Signed-off-by: Alessandro Astone ales.astone@gmail.com Link: https://lore.kernel.org/r/20220415120015.52684-1-ales.astone@gmail.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Carlos Llamas cmllamas@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- drivers/android/binder.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 512cf08e2c07..720ee4b97879 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2922,6 +2922,9 @@ static int binder_translate_fd_array(struct list_head *pf_head, struct binder_proc *proc = thread->proc; int ret;
+ if (fda->num_fds == 0) + return 0; + fd_buf_size = sizeof(u32) * fda->num_fds; if (fda->num_fds >= SIZE_MAX / sizeof(u32)) { binder_user_error("%d:%d got transaction with invalid number of fds (%lld)\n",
-
Yongqiang Liu