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 <[email protected]>
---
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
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 <[email protected]>
> ---
Seems sane,
Acked-by: Christian Brauner (Microsoft) <[email protected]>
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 <[email protected]>
> ---
> 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: [email protected] # 4.14+
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 <[email protected]>
> > ---
> > 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: [email protected] # 4.14+
Thanks, I'll add this when I queue it up after 5.20-rc1 is out.
greg k-h
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
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
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.