Re: [PATCH] binder: fix UAF of ref->proc caused by race condition

Re: [PATCH] binder: fix UAF of ref->proc caused by race condition

[Carlos Llamas]

On Mon, Aug 01, 2022 at 06:25:11PM +0000, Carlos Llamas wrote:
> A transaction of type BINDER_TYPE_WEAK_HANDLE can fail to increment the
> reference for a node. In this case, the target proc normally releases
> the failed reference upon close as expected. However, if the target is
> dying in parallel the call will race with binder_deferred_release(), so
> the target could have released all of its references by now leaving the
> cleanup of the new failed reference unhandled.
> 
> The transaction then ends and the target proc gets released making the
> ref->proc now a dangling pointer. Later on, ref->node is closed and we
> attempt to take spin_lock(&ref->proc->inner_lock), which leads to the
> use-after-free bug reported below. Let's fix this by cleaning up the
> failed reference on the spot instead of relying on the target to do so.
> 
>   ==================================================================
>   BUG: KASAN: use-after-free in _raw_spin_lock+0xa8/0x150
>   Write of size 4 at addr ffff5ca207094238 by task kworker/1:0/590
> 
>   CPU: 1 PID: 590 Comm: kworker/1:0 Not tainted 5.19.0-rc8 #10
>   Hardware name: linux,dummy-virt (DT)
>   Workqueue: events binder_deferred_func
>   Call trace:
>    dump_backtrace.part.0+0x1d0/0x1e0
>    show_stack+0x18/0x70
>    dump_stack_lvl+0x68/0x84
>    print_report+0x2e4/0x61c
>    kasan_report+0xa4/0x110
>    kasan_check_range+0xfc/0x1a4
>    __kasan_check_write+0x3c/0x50
>    _raw_spin_lock+0xa8/0x150
>    binder_deferred_func+0x5e0/0x9b0
>    process_one_work+0x38c/0x5f0
>    worker_thread+0x9c/0x694
>    kthread+0x188/0x190
>    ret_from_fork+0x10/0x20
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 362c0deb65f1..9d42afe60180 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1361,6 +1361,18 @@ static int binder_inc_ref_for_node(struct binder_proc *proc,
>  	}
>  	ret = binder_inc_ref_olocked(ref, strong, target_list);
>  	*rdata = ref->data;
> +	if (ret && ref == new_ref) {
> +		/*
> +		 * Cleanup the failed reference here as the target
> +		 * could now be dead and have already released its
> +		 * references by now. Calling on the new reference
> +		 * with strong=0 and a tmp_refs will not decrement
> +		 * the node. The new_ref gets kfree'd below.
> +		 */
> +		binder_cleanup_ref_olocked(new_ref);
> +		ref = NULL;
> +	}
> +
>  	binder_proc_unlock(proc);
>  	if (new_ref && ref != new_ref)
>  		/*
> -- 
> 2.37.1.455.g008518b4e5-goog
> 

Sorry, I forgot to CC stable. This patch should be applied to all stable
kernels starting with 4.14 and higher.

Cc: stable@vger.kernel.org # 4.14+

Re: [PATCH] binder: fix UAF of ref->proc caused by race condition

[Greg Kroah-Hartman]

On Tue, Aug 02, 2022 at 07:40:32PM +0000, Carlos Llamas wrote:
> On Mon, Aug 01, 2022 at 06:25:11PM +0000, Carlos Llamas wrote:
> > A transaction of type BINDER_TYPE_WEAK_HANDLE can fail to increment the
> > reference for a node. In this case, the target proc normally releases
> > the failed reference upon close as expected. However, if the target is
> > dying in parallel the call will race with binder_deferred_release(), so
> > the target could have released all of its references by now leaving the
> > cleanup of the new failed reference unhandled.
> > 
> > The transaction then ends and the target proc gets released making the
> > ref->proc now a dangling pointer. Later on, ref->node is closed and we
> > attempt to take spin_lock(&ref->proc->inner_lock), which leads to the
> > use-after-free bug reported below. Let's fix this by cleaning up the
> > failed reference on the spot instead of relying on the target to do so.
> > 
> >   ==================================================================
> >   BUG: KASAN: use-after-free in _raw_spin_lock+0xa8/0x150
> >   Write of size 4 at addr ffff5ca207094238 by task kworker/1:0/590
> > 
> >   CPU: 1 PID: 590 Comm: kworker/1:0 Not tainted 5.19.0-rc8 #10
> >   Hardware name: linux,dummy-virt (DT)
> >   Workqueue: events binder_deferred_func
> >   Call trace:
> >    dump_backtrace.part.0+0x1d0/0x1e0
> >    show_stack+0x18/0x70
> >    dump_stack_lvl+0x68/0x84
> >    print_report+0x2e4/0x61c
> >    kasan_report+0xa4/0x110
> >    kasan_check_range+0xfc/0x1a4
> >    __kasan_check_write+0x3c/0x50
> >    _raw_spin_lock+0xa8/0x150
> >    binder_deferred_func+0x5e0/0x9b0
> >    process_one_work+0x38c/0x5f0
> >    worker_thread+0x9c/0x694
> >    kthread+0x188/0x190
> >    ret_from_fork+0x10/0x20
> > 
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> >  drivers/android/binder.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 362c0deb65f1..9d42afe60180 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -1361,6 +1361,18 @@ static int binder_inc_ref_for_node(struct binder_proc *proc,
> >  	}
> >  	ret = binder_inc_ref_olocked(ref, strong, target_list);
> >  	*rdata = ref->data;
> > +	if (ret && ref == new_ref) {
> > +		/*
> > +		 * Cleanup the failed reference here as the target
> > +		 * could now be dead and have already released its
> > +		 * references by now. Calling on the new reference
> > +		 * with strong=0 and a tmp_refs will not decrement
> > +		 * the node. The new_ref gets kfree'd below.
> > +		 */
> > +		binder_cleanup_ref_olocked(new_ref);
> > +		ref = NULL;
> > +	}
> > +
> >  	binder_proc_unlock(proc);
> >  	if (new_ref && ref != new_ref)
> >  		/*
> > -- 
> > 2.37.1.455.g008518b4e5-goog
> > 
> 
> Sorry, I forgot to CC stable. This patch should be applied to all stable
> kernels starting with 4.14 and higher.
> 
> Cc: stable@vger.kernel.org # 4.14+

Thanks, I'll add this when I queue it up after 5.20-rc1 is out.

greg k-h

Re: [PATCH] binder: fix UAF of ref->proc caused by race condition

[Carlos Llamas]

On Wed, Aug 03, 2022 at 09:29:24AM +0200, Greg Kroah-Hartman wrote:
> 
> Thanks, I'll add this when I queue it up after 5.20-rc1 is out.
> 
> greg k-h

Hi Greg,

I think this might have fallen through the cracks, I can't find it in
char-misc or linux-next trees.

--
Carlos Llamas

Re: [PATCH] binder: fix UAF of ref->proc caused by race condition

[Greg Kroah-Hartman]

On Wed, Aug 17, 2022 at 10:04:50PM +0000, Carlos Llamas wrote:
> On Wed, Aug 03, 2022 at 09:29:24AM +0200, Greg Kroah-Hartman wrote:
> > 
> > Thanks, I'll add this when I queue it up after 5.20-rc1 is out.
> > 
> > greg k-h
> 
> Hi Greg,
> 
> I think this might have fallen through the cracks, I can't find it in
> char-misc or linux-next trees.

-rc1 only came out a few days ago, and I have a lot to catch up on:
	$ mdfrm -c ~/mail/todo/
	1770 messages in /home/gregkh/mail/todo/

Please give me a chance...

thanks,

greg k-h

Re: [PATCH] binder: fix UAF of ref->proc caused by race condition

[Carlos Llamas]

On Thu, Aug 18, 2022 at 07:26:01AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 17, 2022 at 10:04:50PM +0000, Carlos Llamas wrote:
> > On Wed, Aug 03, 2022 at 09:29:24AM +0200, Greg Kroah-Hartman wrote:
> > > 
> > > Thanks, I'll add this when I queue it up after 5.20-rc1 is out.
> > > 
> > > greg k-h
> > 
> > Hi Greg,
> > 
> > I think this might have fallen through the cracks, I can't find it in
> > char-misc or linux-next trees.
> 
> -rc1 only came out a few days ago, and I have a lot to catch up on:
> 	$ mdfrm -c ~/mail/todo/
> 	1770 messages in /home/gregkh/mail/todo/
> 
> Please give me a chance...
> 
> thanks,
> 
> greg k-h

Oh my bad, I got mixed up with 5.20-rc1 and 6.0-rc1. Please disregard my
previous email.