* Re: [PATCH] binder: fix UAF of ref->proc caused by race condition [not found] <20220801182511.3371447-1-cmllamas@google.com> @ 2022-08-02 19:40 ` Carlos Llamas 2022-08-03 7:29 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Carlos Llamas @ 2022-08-02 19:40 UTC (permalink / raw) To: Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos, Martijn Coenen, Christian Brauner Cc: Joel Fernandes, Suren Baghdasaryan, kernel-team, linux-kernel, stable 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+ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] binder: fix UAF of ref->proc caused by race condition 2022-08-02 19:40 ` [PATCH] binder: fix UAF of ref->proc caused by race condition Carlos Llamas @ 2022-08-03 7:29 ` Greg Kroah-Hartman 2022-08-17 22:04 ` Carlos Llamas 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2022-08-03 7:29 UTC (permalink / raw) To: Carlos Llamas Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen, Christian Brauner, Joel Fernandes, Suren Baghdasaryan, kernel-team, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] binder: fix UAF of ref->proc caused by race condition 2022-08-03 7:29 ` Greg Kroah-Hartman @ 2022-08-17 22:04 ` Carlos Llamas 2022-08-18 5:26 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Carlos Llamas @ 2022-08-17 22:04 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen, Christian Brauner, Joel Fernandes, Suren Baghdasaryan, kernel-team, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] binder: fix UAF of ref->proc caused by race condition 2022-08-17 22:04 ` Carlos Llamas @ 2022-08-18 5:26 ` Greg Kroah-Hartman 2022-08-18 15:48 ` Carlos Llamas 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2022-08-18 5:26 UTC (permalink / raw) To: Carlos Llamas Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen, Christian Brauner, Joel Fernandes, Suren Baghdasaryan, kernel-team, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] binder: fix UAF of ref->proc caused by race condition 2022-08-18 5:26 ` Greg Kroah-Hartman @ 2022-08-18 15:48 ` Carlos Llamas 0 siblings, 0 replies; 5+ messages in thread From: Carlos Llamas @ 2022-08-18 15:48 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen, Christian Brauner, Joel Fernandes, Suren Baghdasaryan, kernel-team, linux-kernel, stable 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-18 15:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220801182511.3371447-1-cmllamas@google.com>
2022-08-02 19:40 ` [PATCH] binder: fix UAF of ref->proc caused by race condition Carlos Llamas
2022-08-03 7:29 ` Greg Kroah-Hartman
2022-08-17 22:04 ` Carlos Llamas
2022-08-18 5:26 ` Greg Kroah-Hartman
2022-08-18 15:48 ` Carlos Llamas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).