| Message ID | 20240301194037.532117-3-mic@digikod.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Handle faults in KUnit tests | expand |
On Fri, Mar 1, 2024 at 2:40 PM Mickaël Salaün <mic@digikod.net> wrote: > > There is a race condition when a kthread finishes after the deadline and > before the call to kthread_stop(), which may lead to use after free. Hello! I have tested this patch and it looks good to me. Thanks! -Rae Reviewed-by: Rae Moar <rmoar@google.com> > > Cc: Brendan Higgins <brendanhiggins@google.com> > Cc: David Gow <davidgow@google.com> > Cc: Rae Moar <rmoar@google.com> > Cc: Shuah Khan <skhan@linuxfoundation.org> > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Mickaël Salaün <mic@digikod.net> > Link: https://lore.kernel.org/r/20240301194037.532117-3-mic@digikod.net > --- > > Changes since v1: > * Added Kees's Reviewed-by. > --- > lib/kunit/try-catch.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c > index a5cb2ef70a25..73f5007f20ea 100644 > --- a/lib/kunit/try-catch.c > +++ b/lib/kunit/try-catch.c > @@ -11,6 +11,7 @@ > #include <linux/completion.h> > #include <linux/kernel.h> > #include <linux/kthread.h> > +#include <linux/sched/task.h> > > #include "try-catch-impl.h" > > @@ -65,14 +66,15 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > try_catch->context = context; > try_catch->try_completion = &try_completion; > try_catch->try_result = 0; > - task_struct = kthread_run(kunit_generic_run_threadfn_adapter, > - try_catch, > - "kunit_try_catch_thread"); > + task_struct = kthread_create(kunit_generic_run_threadfn_adapter, > + try_catch, "kunit_try_catch_thread"); > if (IS_ERR(task_struct)) { > try_catch->try_result = PTR_ERR(task_struct); > try_catch->catch(try_catch->context); > return; > } > + get_task_struct(task_struct); > + wake_up_process(task_struct); > > time_remaining = wait_for_completion_timeout(&try_completion, > kunit_test_timeout()); > @@ -82,6 +84,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > kthread_stop(task_struct); > } > > + put_task_struct(task_struct); > exit_code = try_catch->try_result; > > if (!exit_code) > -- > 2.44.0 >
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün <mic@digikod.net> wrote: > > There is a race condition when a kthread finishes after the deadline and > before the call to kthread_stop(), which may lead to use after free. > > Cc: Brendan Higgins <brendanhiggins@google.com> > Cc: David Gow <davidgow@google.com> > Cc: Rae Moar <rmoar@google.com> > Cc: Shuah Khan <skhan@linuxfoundation.org> > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Mickaël Salaün <mic@digikod.net> > Link: https://lore.kernel.org/r/20240301194037.532117-3-mic@digikod.net > --- Thanks: I've never been unlucky enough to hit this, but it's definitely a bug. We should ideally mark it with a fixes tag: Fixes: adf505457032 ("kunit: fix UAF when run kfence test case test_gfpzero") Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > > Changes since v1: > * Added Kees's Reviewed-by. > --- > lib/kunit/try-catch.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c > index a5cb2ef70a25..73f5007f20ea 100644 > --- a/lib/kunit/try-catch.c > +++ b/lib/kunit/try-catch.c > @@ -11,6 +11,7 @@ > #include <linux/completion.h> > #include <linux/kernel.h> > #include <linux/kthread.h> > +#include <linux/sched/task.h> > > #include "try-catch-impl.h" > > @@ -65,14 +66,15 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > try_catch->context = context; > try_catch->try_completion = &try_completion; > try_catch->try_result = 0; > - task_struct = kthread_run(kunit_generic_run_threadfn_adapter, > - try_catch, > - "kunit_try_catch_thread"); > + task_struct = kthread_create(kunit_generic_run_threadfn_adapter, > + try_catch, "kunit_try_catch_thread"); > if (IS_ERR(task_struct)) { > try_catch->try_result = PTR_ERR(task_struct); > try_catch->catch(try_catch->context); > return; > } > + get_task_struct(task_struct); > + wake_up_process(task_struct); > > time_remaining = wait_for_completion_timeout(&try_completion, > kunit_test_timeout()); > @@ -82,6 +84,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > kthread_stop(task_struct); > } > > + put_task_struct(task_struct); > exit_code = try_catch->try_result; > > if (!exit_code) > -- > 2.44.0 >
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c index a5cb2ef70a25..73f5007f20ea 100644 --- a/lib/kunit/try-catch.c +++ b/lib/kunit/try-catch.c @@ -11,6 +11,7 @@ #include <linux/completion.h> #include <linux/kernel.h> #include <linux/kthread.h> +#include <linux/sched/task.h> #include "try-catch-impl.h" @@ -65,14 +66,15 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) try_catch->context = context; try_catch->try_completion = &try_completion; try_catch->try_result = 0; - task_struct = kthread_run(kunit_generic_run_threadfn_adapter, - try_catch, - "kunit_try_catch_thread"); + task_struct = kthread_create(kunit_generic_run_threadfn_adapter, + try_catch, "kunit_try_catch_thread"); if (IS_ERR(task_struct)) { try_catch->try_result = PTR_ERR(task_struct); try_catch->catch(try_catch->context); return; } + get_task_struct(task_struct); + wake_up_process(task_struct); time_remaining = wait_for_completion_timeout(&try_completion, kunit_test_timeout()); @@ -82,6 +84,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) kthread_stop(task_struct); } + put_task_struct(task_struct); exit_code = try_catch->try_result; if (!exit_code)