From 4b836a1426cb0f1ef2a6e211d7e553221594f8fc Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Mon, 27 Jul 2020 14:04:24 +0200 Subject: binder: Prevent context manager from incrementing ref 0 Binder is designed such that a binder_proc never has references to itself. If this rule is violated, memory corruption can occur when a process sends a transaction to itself; see e.g. . There is a remaining edgecase through which such a transaction-to-self can still occur from the context of a task with BINDER_SET_CONTEXT_MGR access: - task A opens /dev/binder twice, creating binder_proc instances P1 and P2 - P1 becomes context manager - P2 calls ACQUIRE on the magic handle 0, allocating index 0 in its handle table - P1 dies (by closing the /dev/binder fd and waiting a bit) - P2 becomes context manager - P2 calls ACQUIRE on the magic handle 0, allocating index 1 in its handle table [this triggers a warning: "binder: 1974:1974 tried to acquire reference to desc 0, got 1 instead"] - task B opens /dev/binder once, creating binder_proc instance P3 - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way transaction) - P2 receives the handle and uses it to call P3 (two-way transaction) - P3 calls P2 (via magic handle 0) (two-way transaction) - P2 calls P2 (via handle 1) (two-way transaction) And then, if P2 does *NOT* accept the incoming transaction work, but instead closes the binder fd, we get a crash. Solve it by preventing the context manager from using ACQUIRE on ref 0. There shouldn't be any legitimate reason for the context manager to do that. Additionally, print a warning if someone manages to find another way to trigger a transaction-to-self bug in the future. Cc: stable@vger.kernel.org Fixes: 457b9a6f09f0 ("Staging: android: add binder driver") Acked-by: Todd Kjos Signed-off-by: Jann Horn Reviewed-by: Martijn Coenen Link: https://lore.kernel.org/r/20200727120424.1627555-1-jannh@google.com Signed-off-by: Greg Kroah-Hartman --- drivers/android/binder.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) (limited to 'drivers/android') diff --git a/drivers/android/binder.c b/drivers/android/binder.c index f50c5f182bb5..5b310eea9e52 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2982,6 +2982,12 @@ static void binder_transaction(struct binder_proc *proc, goto err_dead_binder; } e->to_node = target_node->debug_id; + if (WARN_ON(proc == target_proc)) { + return_error = BR_FAILED_REPLY; + return_error_param = -EINVAL; + return_error_line = __LINE__; + goto err_invalid_target_handle; + } if (security_binder_transaction(proc->tsk, target_proc->tsk) < 0) { return_error = BR_FAILED_REPLY; @@ -3635,10 +3641,17 @@ static int binder_thread_write(struct binder_proc *proc, struct binder_node *ctx_mgr_node; mutex_lock(&context->context_mgr_node_lock); ctx_mgr_node = context->binder_context_mgr_node; - if (ctx_mgr_node) + if (ctx_mgr_node) { + if (ctx_mgr_node->proc == proc) { + binder_user_error("%d:%d context manager tried to acquire desc 0\n", + proc->pid, thread->pid); + mutex_unlock(&context->context_mgr_node_lock); + return -EINVAL; + } ret = binder_inc_ref_for_node( proc, ctx_mgr_node, strong, NULL, &rdata); + } mutex_unlock(&context->context_mgr_node_lock); } if (ret) -- cgit v1.2.3 From 4df9772c8489b7d626db0ee307e029aea66db260 Mon Sep 17 00:00:00 2001 From: Mrinal Pandey Date: Fri, 24 Jul 2020 18:42:54 +0530 Subject: drivers: android: Fix a variable declaration coding style issue Add a blank line after variable declarations as suggested by checkpatch. Signed-off-by: Mrinal Pandey Link: https://lore.kernel.org/r/20200724131254.qxbvderrws36dzzq@mrinalpandey Signed-off-by: Greg Kroah-Hartman --- drivers/android/binder_alloc.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/android') diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index cbe6aa77d50d..69609696a843 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -547,6 +547,7 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc, { struct binder_buffer *prev, *next = NULL; bool to_free = true; + BUG_ON(alloc->buffers.next == &buffer->entry); prev = binder_buffer_prev(buffer); BUG_ON(!prev->free); -- cgit v1.2.3 From 72b93c79dbbe261371abb1bf3cf7302cfe31e8d9 Mon Sep 17 00:00:00 2001 From: Mrinal Pandey Date: Fri, 24 Jul 2020 18:43:48 +0530 Subject: drivers: android: Remove the use of else after return Remove the unnecessary else branch after return statement as suggested by checkpatch. Signed-off-by: Mrinal Pandey Link: https://lore.kernel.org/r/20200724131348.haz4ocxcferdcsgn@mrinalpandey Signed-off-by: Greg Kroah-Hartman --- drivers/android/binder.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/android') diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 5b310eea9e52..2123655ebb7c 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -1969,9 +1969,8 @@ static void binder_send_failed_reply(struct binder_transaction *t, binder_thread_dec_tmpref(target_thread); binder_free_transaction(t); return; - } else { - __release(&target_thread->proc->inner_lock); } + __release(&target_thread->proc->inner_lock); next = t->from_parent; binder_debug(BINDER_DEBUG_FAILED_TRANSACTION, -- cgit v1.2.3 From 8df5b9492202e9cac9917465e945fcf478d55404 Mon Sep 17 00:00:00 2001 From: Mrinal Pandey Date: Fri, 24 Jul 2020 18:44:03 +0530 Subject: drivers: android: Remove braces for a single statement if-else block Remove braces for both if and else block as suggested by checkpatch. Signed-off-by: Mrinal Pandey Link: https://lore.kernel.org/r/20200724131403.dahfhdwa3wirzkxj@mrinalpandey Signed-off-by: Greg Kroah-Hartman --- drivers/android/binder.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/android') diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 2123655ebb7c..f936530a19b0 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2759,11 +2759,10 @@ static bool binder_proc_transaction(struct binder_transaction *t, binder_node_lock(node); if (oneway) { BUG_ON(thread); - if (node->has_async_transaction) { + if (node->has_async_transaction) pending_async = true; - } else { + else node->has_async_transaction = true; - } } binder_inner_proc_lock(proc); -- cgit v1.2.3 From 81195f9689ac16c01c894c756b925e28e546b123 Mon Sep 17 00:00:00 2001 From: Mrinal Pandey Date: Fri, 24 Jul 2020 18:44:33 +0530 Subject: drivers: android: Fix a variable declaration coding style issue Add a blank line after variable declarations as suggested by checkpatch. Signed-off-by: Mrinal Pandey Link: https://lore.kernel.org/r/20200724131433.stf3ycooogawyzb3@mrinalpandey Signed-off-by: Greg Kroah-Hartman --- drivers/android/binderfs.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/android') diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index 7cf566aafe1f..e218360de58d 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -351,6 +351,7 @@ static const struct super_operations binderfs_super_ops = { static inline bool is_binderfs_control_device(const struct dentry *dentry) { struct binderfs_info *info = dentry->d_sb->s_fs_info; + return info->control_dentry == dentry; } -- cgit v1.2.3 From 7e84522cd089c6ef3e6adc7f1c9a5b2f705ccd9b Mon Sep 17 00:00:00 2001 From: Mrinal Pandey Date: Fri, 24 Jul 2020 18:44:49 +0530 Subject: drivers: android: Fix the SPDX comment style C source files should have `//` as SPDX comment and not `/**/`. Fix this by running checkpatch on the file. Signed-off-by: Mrinal Pandey Link: https://lore.kernel.org/r/20200724131449.zvjutbemg3vqhrzh@mrinalpandey Signed-off-by: Greg Kroah-Hartman --- drivers/android/binderfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/android') diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index e218360de58d..7b76fefde3f8 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -1,4 +1,4 @@ -/* SPDX-License-Identifier: GPL-2.0 */ +// SPDX-License-Identifier: GPL-2.0 #include #include -- cgit v1.2.3