linux-kernel - Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZvRM6RHstUiTSsk4@google.com>
Date: Wed, 25 Sep 2024 17:48:25 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>,
	Joel Fernandes <joel@...lfernandes.org>,
	Christian Brauner <brauner@...nel.org>,
	Suren Baghdasaryan <surenb@...gle.com>,
	Yu-Ting Tseng <yutingtseng@...gle.com>,
	linux-kernel@...r.kernel.org, kernel-team@...roid.com,
	stable@...r.kernel.org
Subject: Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

On Wed, Sep 25, 2024 at 10:02:51AM +0200, 'Alice Ryhl' via kernel-team wrote:
> On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@...gle.com> wrote:
> >
> > 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@...r.kernel.org
> > Signed-off-by: Carlos Llamas <cmllamas@...gle.com>
> 
> This change LGTM.
> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
> 
> I reviewed some other code paths to verify whether there are other
> problems with processes dying concurrently with operations on freeze
> notifications. I didn't notice any other memory safety issues, but I

Yeah most other paths are protected with binder_procs_lock mutex.

> noticed that binder_request_freeze_notification returns EINVAL if you
> try to use it with a node from a dead process. That seems problematic,
> as this means that there's no way to invoke that command without
> risking an EINVAL error if the remote process dies. We should not
> return EINVAL errors on correct usage of the driver.

Agreed, this should probably be -ESRCH or something. I'll add it to v2,
thanks for the suggestion.

Cheers,
Carlos Llamas