linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] task_work: kasan: record task_work_add() call stack
@ 2021-03-15  1:59 Walter Wu
  2021-03-15  6:58 ` Dmitry Vyukov
  0 siblings, 1 reply; 5+ messages in thread
From: Walter Wu @ 2021-03-15  1:59 UTC (permalink / raw)
  To: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Matthias Brugger, Andrey Konovalov, Andrew Morton, Jens Axboe,
	Oleg Nesterov
  Cc: kasan-dev, linux-mm, linux-kernel, linux-arm-kernel,
	wsd_upstream, linux-mediatek, Walter Wu

Why record task_work_add() call stack?
Syzbot reports many use-after-free issues for task_work, see [1].
After see the free stack and the current auxiliary stack, we think
they are useless, we don't know where register the work, this work
may be the free call stack, so that we miss the root cause and
don't solve the use-after-free.

Add task_work_add() call stack into KASAN auxiliary stack in
order to improve KASAN report. It is useful for programmers
to solve use-after-free issues.

[1]: https://groups.google.com/g/syzkaller-bugs/search?q=kasan%20use-after-free%20task_work_run

Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
Suggested-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 kernel/task_work.c | 3 +++
 mm/kasan/kasan.h   | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/task_work.c b/kernel/task_work.c
index 9cde961875c0..f255294377da 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -55,6 +55,9 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
 		break;
 	}
 
+	/* record the work call stack in order to print it in KASAN reports */
+	kasan_record_aux_stack(work);
+
 	return 0;
 }
 
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 3436c6bf7c0c..d300fe9415bd 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -146,7 +146,7 @@ struct kasan_alloc_meta {
 	struct kasan_track alloc_track;
 #ifdef CONFIG_KASAN_GENERIC
 	/*
-	 * call_rcu() call stack is stored into struct kasan_alloc_meta.
+	 * Auxiliary stack is stored into struct kasan_alloc_meta.
 	 * The free stack is stored into struct kasan_free_meta.
 	 */
 	depot_stack_handle_t aux_stack[2];
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] task_work: kasan: record task_work_add() call stack
  2021-03-15  1:59 [PATCH] task_work: kasan: record task_work_add() call stack Walter Wu
@ 2021-03-15  6:58 ` Dmitry Vyukov
  2021-03-15  9:38   ` Walter Wu
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Vyukov @ 2021-03-15  6:58 UTC (permalink / raw)
  To: Walter Wu
  Cc: Andrey Ryabinin, Alexander Potapenko, Matthias Brugger,
	Andrey Konovalov, Andrew Morton, Jens Axboe, Oleg Nesterov,
	kasan-dev, Linux-MM, LKML, Linux ARM, wsd_upstream,
	linux-mediatek

On Mon, Mar 15, 2021 at 3:00 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> Why record task_work_add() call stack?
> Syzbot reports many use-after-free issues for task_work, see [1].
> After see the free stack and the current auxiliary stack, we think
> they are useless, we don't know where register the work, this work
> may be the free call stack, so that we miss the root cause and
> don't solve the use-after-free.
>
> Add task_work_add() call stack into KASAN auxiliary stack in
> order to improve KASAN report. It is useful for programmers
> to solve use-after-free issues.
>
> [1]: https://groups.google.com/g/syzkaller-bugs/search?q=kasan%20use-after-free%20task_work_run
>
> Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/task_work.c | 3 +++
>  mm/kasan/kasan.h   | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/task_work.c b/kernel/task_work.c
> index 9cde961875c0..f255294377da 100644
> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -55,6 +55,9 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
>                 break;
>         }
>
> +       /* record the work call stack in order to print it in KASAN reports */
> +       kasan_record_aux_stack(work);

I think this call should be done _before_ we actually queue the work,
because this function may operate on non-current task.
Consider, we queue the work, the other task already executes it and
triggers use-after-free, now only now we record the stack.
Moreover, I think we can trigger use-after-free here ourselves while
recording the aux stack. We queued the work, and the work can cause
own free, so it's not necessary live by now.

>         return 0;
>  }
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 3436c6bf7c0c..d300fe9415bd 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -146,7 +146,7 @@ struct kasan_alloc_meta {
>         struct kasan_track alloc_track;
>  #ifdef CONFIG_KASAN_GENERIC
>         /*
> -        * call_rcu() call stack is stored into struct kasan_alloc_meta.
> +        * Auxiliary stack is stored into struct kasan_alloc_meta.
>          * The free stack is stored into struct kasan_free_meta.
>          */
>         depot_stack_handle_t aux_stack[2];
> --
> 2.18.0

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] task_work: kasan: record task_work_add() call stack
  2021-03-15  6:58 ` Dmitry Vyukov
@ 2021-03-15  9:38   ` Walter Wu
  2021-03-15 10:03     ` Dmitry Vyukov
  0 siblings, 1 reply; 5+ messages in thread
From: Walter Wu @ 2021-03-15  9:38 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Andrey Ryabinin, Alexander Potapenko, Matthias Brugger,
	Andrey Konovalov, Andrew Morton, Jens Axboe, Oleg Nesterov,
	kasan-dev, Linux-MM, LKML, Linux ARM, wsd_upstream,
	linux-mediatek

On Mon, 2021-03-15 at 07:58 +0100, 'Dmitry Vyukov' via kasan-dev wrote:
> On Mon, Mar 15, 2021 at 3:00 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> >
> > Why record task_work_add() call stack?
> > Syzbot reports many use-after-free issues for task_work, see [1].
> > After see the free stack and the current auxiliary stack, we think
> > they are useless, we don't know where register the work, this work
> > may be the free call stack, so that we miss the root cause and
> > don't solve the use-after-free.
> >
> > Add task_work_add() call stack into KASAN auxiliary stack in
> > order to improve KASAN report. It is useful for programmers
> > to solve use-after-free issues.
> >
> > [1]: https://groups.google.com/g/syzkaller-bugs/search?q=kasan%20use-after-free%20task_work_run
> >
> > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Andrey Konovalov <andreyknvl@google.com>
> > Cc: Alexander Potapenko <glider@google.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  kernel/task_work.c | 3 +++
> >  mm/kasan/kasan.h   | 2 +-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/task_work.c b/kernel/task_work.c
> > index 9cde961875c0..f255294377da 100644
> > --- a/kernel/task_work.c
> > +++ b/kernel/task_work.c
> > @@ -55,6 +55,9 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
> >                 break;
> >         }
> >
> > +       /* record the work call stack in order to print it in KASAN reports */
> > +       kasan_record_aux_stack(work);
> 
> I think this call should be done _before_ we actually queue the work,
> because this function may operate on non-current task.
> Consider, we queue the work, the other task already executes it and
> triggers use-after-free, now only now we record the stack.

agree, what do you think below change?

--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -34,6 +34,9 @@ int task_work_add(struct task_struct *task, struct
callback_head *work,
 {
    struct callback_head *head;

+   /* record the work call stack in order to print it in KASAN reports
*/
+   kasan_record_aux_stack(work);
+
    do {
        head = READ_ONCE(task->task_works);
        if (unlikely(head == &work_exited))
@@ -55,9 +58,6 @@ int task_work_add(struct task_struct *task, struct
callback_head *work,
        break;
    }

-   /* record the work call stack in order to print it in KASAN reports
*/
-   kasan_record_aux_stack(work);
-
    return 0;
 }

> Moreover, I think we can trigger use-after-free here ourselves while
> recording the aux stack. We queued the work, and the work can cause
> own free, so it's not necessary live by now.

Sorry, I don't fully know your meaning, do you mean we should add an
abort when detect use-after-free?

> 
> >         return 0;
> >  }
> >
> > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > index 3436c6bf7c0c..d300fe9415bd 100644
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -146,7 +146,7 @@ struct kasan_alloc_meta {
> >         struct kasan_track alloc_track;
> >  #ifdef CONFIG_KASAN_GENERIC
> >         /*
> > -        * call_rcu() call stack is stored into struct kasan_alloc_meta.
> > +        * Auxiliary stack is stored into struct kasan_alloc_meta.
> >          * The free stack is stored into struct kasan_free_meta.
> >          */
> >         depot_stack_handle_t aux_stack[2];
> > --
> > 2.18.0
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] task_work: kasan: record task_work_add() call stack
  2021-03-15  9:38   ` Walter Wu
@ 2021-03-15 10:03     ` Dmitry Vyukov
  2021-03-15 10:13       ` Walter Wu
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Vyukov @ 2021-03-15 10:03 UTC (permalink / raw)
  To: Walter Wu
  Cc: Andrey Ryabinin, Alexander Potapenko, Matthias Brugger,
	Andrey Konovalov, Andrew Morton, Jens Axboe, Oleg Nesterov,
	kasan-dev, Linux-MM, LKML, Linux ARM, wsd_upstream,
	linux-mediatek

On Mon, Mar 15, 2021 at 10:38 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> On Mon, 2021-03-15 at 07:58 +0100, 'Dmitry Vyukov' via kasan-dev wrote:
> > On Mon, Mar 15, 2021 at 3:00 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> > >
> > > Why record task_work_add() call stack?
> > > Syzbot reports many use-after-free issues for task_work, see [1].
> > > After see the free stack and the current auxiliary stack, we think
> > > they are useless, we don't know where register the work, this work
> > > may be the free call stack, so that we miss the root cause and
> > > don't solve the use-after-free.
> > >
> > > Add task_work_add() call stack into KASAN auxiliary stack in
> > > order to improve KASAN report. It is useful for programmers
> > > to solve use-after-free issues.
> > >
> > > [1]: https://groups.google.com/g/syzkaller-bugs/search?q=kasan%20use-after-free%20task_work_run
> > >
> > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > > Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> > > Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > Cc: Andrey Konovalov <andreyknvl@google.com>
> > > Cc: Alexander Potapenko <glider@google.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > Cc: Jens Axboe <axboe@kernel.dk>
> > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > ---
> > >  kernel/task_work.c | 3 +++
> > >  mm/kasan/kasan.h   | 2 +-
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/task_work.c b/kernel/task_work.c
> > > index 9cde961875c0..f255294377da 100644
> > > --- a/kernel/task_work.c
> > > +++ b/kernel/task_work.c
> > > @@ -55,6 +55,9 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
> > >                 break;
> > >         }
> > >
> > > +       /* record the work call stack in order to print it in KASAN reports */
> > > +       kasan_record_aux_stack(work);
> >
> > I think this call should be done _before_ we actually queue the work,
> > because this function may operate on non-current task.
> > Consider, we queue the work, the other task already executes it and
> > triggers use-after-free, now only now we record the stack.
>
> agree, what do you think below change?
>
> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -34,6 +34,9 @@ int task_work_add(struct task_struct *task, struct
> callback_head *work,
>  {
>     struct callback_head *head;
>
> +   /* record the work call stack in order to print it in KASAN reports
> */
> +   kasan_record_aux_stack(work);
> +

This looks good to me.


>     do {
>         head = READ_ONCE(task->task_works);
>         if (unlikely(head == &work_exited))
> @@ -55,9 +58,6 @@ int task_work_add(struct task_struct *task, struct
> callback_head *work,
>         break;
>     }
>
> -   /* record the work call stack in order to print it in KASAN reports
> */
> -   kasan_record_aux_stack(work);
> -
>     return 0;
>  }
>
> > Moreover, I think we can trigger use-after-free here ourselves while
> > recording the aux stack. We queued the work, and the work can cause
> > own free, so it's not necessary live by now.
>
> Sorry, I don't fully know your meaning, do you mean we should add an
> abort when detect use-after-free?

I meant that where we had the kasan_record_aux_stack(work) call in the
first version of the patch, work can be already freed. We must not
access work after queueing it.

> > >         return 0;
> > >  }
> > >
> > > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > > index 3436c6bf7c0c..d300fe9415bd 100644
> > > --- a/mm/kasan/kasan.h
> > > +++ b/mm/kasan/kasan.h
> > > @@ -146,7 +146,7 @@ struct kasan_alloc_meta {
> > >         struct kasan_track alloc_track;
> > >  #ifdef CONFIG_KASAN_GENERIC
> > >         /*
> > > -        * call_rcu() call stack is stored into struct kasan_alloc_meta.
> > > +        * Auxiliary stack is stored into struct kasan_alloc_meta.
> > >          * The free stack is stored into struct kasan_free_meta.
> > >          */
> > >         depot_stack_handle_t aux_stack[2];
> > > --
> > > 2.18.0
> >
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1615801102.24887.4.camel%40mtksdccf07.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] task_work: kasan: record task_work_add() call stack
  2021-03-15 10:03     ` Dmitry Vyukov
@ 2021-03-15 10:13       ` Walter Wu
  0 siblings, 0 replies; 5+ messages in thread
From: Walter Wu @ 2021-03-15 10:13 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Andrey Ryabinin, Alexander Potapenko, Matthias Brugger,
	Andrey Konovalov, Andrew Morton, Jens Axboe, Oleg Nesterov,
	kasan-dev, Linux-MM, LKML, Linux ARM, wsd_upstream,
	linux-mediatek

On Mon, 2021-03-15 at 11:03 +0100, 'Dmitry Vyukov' via kasan-dev wrote:
> On Mon, Mar 15, 2021 at 10:38 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> >
> > On Mon, 2021-03-15 at 07:58 +0100, 'Dmitry Vyukov' via kasan-dev wrote:
> > > On Mon, Mar 15, 2021 at 3:00 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> > > >
> > > > Why record task_work_add() call stack?
> > > > Syzbot reports many use-after-free issues for task_work, see [1].
> > > > After see the free stack and the current auxiliary stack, we think
> > > > they are useless, we don't know where register the work, this work
> > > > may be the free call stack, so that we miss the root cause and
> > > > don't solve the use-after-free.
> > > >
> > > > Add task_work_add() call stack into KASAN auxiliary stack in
> > > > order to improve KASAN report. It is useful for programmers
> > > > to solve use-after-free issues.
> > > >
> > > > [1]: https://groups.google.com/g/syzkaller-bugs/search?q=kasan%20use-after-free%20task_work_run
> > > >
> > > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > > > Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> > > > Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > Cc: Andrey Konovalov <andreyknvl@google.com>
> > > > Cc: Alexander Potapenko <glider@google.com>
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > > Cc: Jens Axboe <axboe@kernel.dk>
> > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > ---
> > > >  kernel/task_work.c | 3 +++
> > > >  mm/kasan/kasan.h   | 2 +-
> > > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/task_work.c b/kernel/task_work.c
> > > > index 9cde961875c0..f255294377da 100644
> > > > --- a/kernel/task_work.c
> > > > +++ b/kernel/task_work.c
> > > > @@ -55,6 +55,9 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
> > > >                 break;
> > > >         }
> > > >
> > > > +       /* record the work call stack in order to print it in KASAN reports */
> > > > +       kasan_record_aux_stack(work);
> > >
> > > I think this call should be done _before_ we actually queue the work,
> > > because this function may operate on non-current task.
> > > Consider, we queue the work, the other task already executes it and
> > > triggers use-after-free, now only now we record the stack.
> >
> > agree, what do you think below change?
> >
> > --- a/kernel/task_work.c
> > +++ b/kernel/task_work.c
> > @@ -34,6 +34,9 @@ int task_work_add(struct task_struct *task, struct
> > callback_head *work,
> >  {
> >     struct callback_head *head;
> >
> > +   /* record the work call stack in order to print it in KASAN reports
> > */
> > +   kasan_record_aux_stack(work);
> > +
> 
> This looks good to me.
> 
> 
> >     do {
> >         head = READ_ONCE(task->task_works);
> >         if (unlikely(head == &work_exited))
> > @@ -55,9 +58,6 @@ int task_work_add(struct task_struct *task, struct
> > callback_head *work,
> >         break;
> >     }
> >
> > -   /* record the work call stack in order to print it in KASAN reports
> > */
> > -   kasan_record_aux_stack(work);
> > -
> >     return 0;
> >  }
> >
> > > Moreover, I think we can trigger use-after-free here ourselves while
> > > recording the aux stack. We queued the work, and the work can cause
> > > own free, so it's not necessary live by now.
> >
> > Sorry, I don't fully know your meaning, do you mean we should add an
> > abort when detect use-after-free?
> 
> I meant that where we had the kasan_record_aux_stack(work) call in the
> first version of the patch, work can be already freed. We must not
> access work after queueing it.
> 

Got it. Now I must treat urgent issue, I will send v2 patch tomorrow.

Thanks for your review.

> > > >         return 0;
> > > >  }
> > > >
> > > > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > > > index 3436c6bf7c0c..d300fe9415bd 100644
> > > > --- a/mm/kasan/kasan.h
> > > > +++ b/mm/kasan/kasan.h
> > > > @@ -146,7 +146,7 @@ struct kasan_alloc_meta {
> > > >         struct kasan_track alloc_track;
> > > >  #ifdef CONFIG_KASAN_GENERIC
> > > >         /*
> > > > -        * call_rcu() call stack is stored into struct kasan_alloc_meta.
> > > > +        * Auxiliary stack is stored into struct kasan_alloc_meta.
> > > >          * The free stack is stored into struct kasan_free_meta.
> > > >          */
> > > >         depot_stack_handle_t aux_stack[2];
> > > > --
> > > > 2.18.0
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1615801102.24887.4.camel%40mtksdccf07.
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-03-15 10:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15  1:59 [PATCH] task_work: kasan: record task_work_add() call stack Walter Wu
2021-03-15  6:58 ` Dmitry Vyukov
2021-03-15  9:38   ` Walter Wu
2021-03-15 10:03     ` Dmitry Vyukov
2021-03-15 10:13       ` Walter Wu

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).