All of lore.kernel.org
 help / color / mirror / Atom feed
From: Carlos Llamas <cmllamas@google.com>
To: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Arve Hjønnevåg" <arve@android.com>,
	"Todd Kjos" <tkjos@android.com>,
	"Martijn Coenen" <maco@android.com>,
	"Joel Fernandes" <joel@joelfernandes.org>,
	"Christian Brauner" <brauner@kernel.org>,
	"Carlos Llamas" <cmllamas@google.com>,
	"Suren Baghdasaryan" <surenb@google.com>,
	"Yu-Ting Tseng" <yutingtseng@google.com>
Cc: linux-kernel@vger.kernel.org, kernel-team@android.com,
	 Alice Ryhl <aliceryhl@google.com>,
	stable@vger.kernel.org, Todd Kjos <tkjos@google.com>
Subject: [PATCH v2 2/8] binder: fix OOB in binder_add_freeze_work()
Date: Thu, 26 Sep 2024 23:36:13 +0000	[thread overview]
Message-ID: <20240926233632.821189-3-cmllamas@google.com> (raw)
In-Reply-To: <20240926233632.821189-1-cmllamas@google.com>

In binder_add_freeze_work() we iterate over the proc->nodes with the
proc->inner_lock held. However, this lock is temporarily dropped to
acquire the node->lock first (lock nesting order). This can race with
binder_deferred_release() which removes the nodes from the proc->nodes
rbtree and adds them into binder_dead_nodes list. This leads to a broken
iteration in binder_add_freeze_work() as rb_next() will use data from
binder_dead_nodes, triggering an out-of-bounds access:

  ==================================================================
  BUG: KASAN: global-out-of-bounds in rb_next+0xfc/0x124
  Read of size 8 at addr ffffcb84285f7170 by task freeze/660

  CPU: 8 UID: 0 PID: 660 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #18
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   rb_next+0xfc/0x124
   binder_add_freeze_work+0x344/0x534
   binder_ioctl+0x1e70/0x25ac
   __arm64_sys_ioctl+0x124/0x190

  The buggy address belongs to the variable:
   binder_dead_nodes+0x10/0x40
  [...]
  ==================================================================

This is possible because proc->nodes (rbtree) and binder_dead_nodes
(list) share entries in binder_node through a union:

	struct binder_node {
	[...]
		union {
			struct rb_node rb_node;
			struct hlist_node dead_node;
		};

Fix the race by checking that the proc is still alive. If not, simply
break out of the iteration.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Acked-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4d90203ea048..8bca2de6fa24 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5593,6 +5593,8 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 		prev = node;
 		binder_node_unlock(node);
 		binder_inner_proc_lock(proc);
+		if (proc->is_dead)
+			break;
 	}
 	binder_inner_proc_unlock(proc);
 	if (prev)
-- 
2.46.1.824.gd892dcdcdd-goog


  parent reply	other threads:[~2024-09-26 23:36 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-26 23:36 [PATCH v2 0/8] binder: several fixes for frozen notification Carlos Llamas
2024-09-26 23:36 ` [PATCH v2 1/8] binder: fix node UAF in binder_add_freeze_work() Carlos Llamas
2024-09-26 23:36 ` Carlos Llamas [this message]
2024-09-26 23:36 ` [PATCH v2 3/8] binder: fix freeze UAF in binder_release_work() Carlos Llamas
2024-09-26 23:36 ` [PATCH v2 4/8] binder: fix BINDER_WORK_FROZEN_BINDER debug logs Carlos Llamas
2024-09-27  7:07   ` Alice Ryhl
2024-09-26 23:36 ` [PATCH v2 5/8] binder: fix BINDER_WORK_CLEAR_FREEZE_NOTIFICATION " Carlos Llamas
2024-09-27  0:34   ` Todd Kjos
2024-09-27  7:20   ` Alice Ryhl
2024-09-26 23:36 ` [PATCH v2 6/8] binder: allow freeze notification for dead nodes Carlos Llamas
2024-09-27  0:48   ` Todd Kjos
2024-09-27  7:19   ` Alice Ryhl
2024-09-27 16:13     ` Yu-Ting Tseng
2024-09-27 16:15       ` Alice Ryhl
2024-09-27 16:32         ` Carlos Llamas
2024-09-30 13:30           ` Alice Ryhl
2024-10-08 18:12             ` Carlos Llamas
2024-09-26 23:36 ` [PATCH v2 7/8] binder: fix memleak of proc->delivered_freeze Carlos Llamas
2024-09-27  0:52   ` Todd Kjos
2024-09-27 10:19   ` Alice Ryhl
2024-09-26 23:36 ` [PATCH v2 8/8] binder: add delivered_freeze to debugfs output Carlos Llamas
2024-09-27  0:38   ` Todd Kjos
2024-09-27  7:19   ` Alice Ryhl
2024-09-27 10:20 ` [PATCH v2 0/8] binder: several fixes for frozen notification Alice Ryhl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240926233632.821189-3-cmllamas@google.com \
    --to=cmllamas@google.com \
    --cc=aliceryhl@google.com \
    --cc=arve@android.com \
    --cc=brauner@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=joel@joelfernandes.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maco@android.com \
    --cc=stable@vger.kernel.org \
    --cc=surenb@google.com \
    --cc=tkjos@android.com \
    --cc=tkjos@google.com \
    --cc=yutingtseng@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.