Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Carlos Llamas <cmllamas@google.com>]

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@google.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@vger.kernel.org
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> 
> This change LGTM.
> Reviewed-by: Alice Ryhl <aliceryhl@google.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