| Message ID | 210e561f7845697a32de44b643393890f180069f.1729272697.git.ritesh.list@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mm/kfence: Add a new kunit test test_use_after_free_read_nofault() | expand |
On Fri, 18 Oct 2024 at 19:46, Ritesh Harjani (IBM) <ritesh.list@gmail.com> wrote: > > From: Nirjhar Roy <nirjhar@linux.ibm.com> > > Faults from copy_from_kernel_nofault() needs to be handled by fixup > table and should not be handled by kfence. Otherwise while reading > /proc/kcore which uses copy_from_kernel_nofault(), kfence can generate > false negatives. This can happen when /proc/kcore ends up reading an > unmapped address from kfence pool. > > Let's add a testcase to cover this case. > > Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > --- > > Will be nice if we can get some feedback on this. There was some discussion recently how sanitizers should behave around these nofault helpers when accessing invalid memory (including freed memory): https://lore.kernel.org/all/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/ It should be similar for KFENCE, i.e. no report should be generated. Definitely a good thing to test. Tested-by: Marco Elver <elver@google.com> Reviewed-by: Marco Elver <elver@google.com> > v2 -> v3: > ========= > 1. Separated out this kfence kunit test from the larger powerpc+kfence+v3 series. > 2. Dropped RFC tag > > [v2]: https://lore.kernel.org/linuxppc-dev/cover.1728954719.git.ritesh.list@gmail.com > [powerpc+kfence+v3]: https://lore.kernel.org/linuxppc-dev/cover.1729271995.git.ritesh.list@gmail.com > > mm/kfence/kfence_test.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > index 00fd17285285..f65fb182466d 100644 > --- a/mm/kfence/kfence_test.c > +++ b/mm/kfence/kfence_test.c > @@ -383,6 +383,22 @@ static void test_use_after_free_read(struct kunit *test) > KUNIT_EXPECT_TRUE(test, report_matches(&expect)); > } > > +static void test_use_after_free_read_nofault(struct kunit *test) > +{ > + const size_t size = 32; > + char *addr; > + char dst; > + int ret; > + > + setup_test_cache(test, size, 0, NULL); > + addr = test_alloc(test, size, GFP_KERNEL, ALLOCATE_ANY); > + test_free(addr); > + /* Use after free with *_nofault() */ > + ret = copy_from_kernel_nofault(&dst, addr, 1); > + KUNIT_EXPECT_EQ(test, ret, -EFAULT); > + KUNIT_EXPECT_FALSE(test, report_available()); > +} > + > static void test_double_free(struct kunit *test) > { > const size_t size = 32; > @@ -780,6 +796,7 @@ static struct kunit_case kfence_test_cases[] = { > KFENCE_KUNIT_CASE(test_out_of_bounds_read), > KFENCE_KUNIT_CASE(test_out_of_bounds_write), > KFENCE_KUNIT_CASE(test_use_after_free_read), > + KFENCE_KUNIT_CASE(test_use_after_free_read_nofault), > KFENCE_KUNIT_CASE(test_double_free), > KFENCE_KUNIT_CASE(test_invalid_addr_free), > KFENCE_KUNIT_CASE(test_corruption), > -- > 2.46.0 >
Marco Elver <elver@google.com> writes: > On Fri, 18 Oct 2024 at 19:46, Ritesh Harjani (IBM) > <ritesh.list@gmail.com> wrote: >> >> From: Nirjhar Roy <nirjhar@linux.ibm.com> >> >> Faults from copy_from_kernel_nofault() needs to be handled by fixup >> table and should not be handled by kfence. Otherwise while reading >> /proc/kcore which uses copy_from_kernel_nofault(), kfence can generate >> false negatives. This can happen when /proc/kcore ends up reading an >> unmapped address from kfence pool. >> >> Let's add a testcase to cover this case. >> >> Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >> Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >> --- >> >> Will be nice if we can get some feedback on this. > > There was some discussion recently how sanitizers should behave around > these nofault helpers when accessing invalid memory (including freed > memory): > https://lore.kernel.org/all/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/ > > It should be similar for KFENCE, i.e. no report should be generated. > Definitely a good thing to test. > > Tested-by: Marco Elver <elver@google.com> > Reviewed-by: Marco Elver <elver@google.com> > Gentle ping. Is this going into -next? -ritesh >> v2 -> v3: >> ========= >> 1. Separated out this kfence kunit test from the larger powerpc+kfence+v3 series. >> 2. Dropped RFC tag >> >> [v2]: https://lore.kernel.org/linuxppc-dev/cover.1728954719.git.ritesh.list@gmail.com >> [powerpc+kfence+v3]: https://lore.kernel.org/linuxppc-dev/cover.1729271995.git.ritesh.list@gmail.com >> >> mm/kfence/kfence_test.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c >> index 00fd17285285..f65fb182466d 100644 >> --- a/mm/kfence/kfence_test.c >> +++ b/mm/kfence/kfence_test.c >> @@ -383,6 +383,22 @@ static void test_use_after_free_read(struct kunit *test) >> KUNIT_EXPECT_TRUE(test, report_matches(&expect)); >> } >> >> +static void test_use_after_free_read_nofault(struct kunit *test) >> +{ >> + const size_t size = 32; >> + char *addr; >> + char dst; >> + int ret; >> + >> + setup_test_cache(test, size, 0, NULL); >> + addr = test_alloc(test, size, GFP_KERNEL, ALLOCATE_ANY); >> + test_free(addr); >> + /* Use after free with *_nofault() */ >> + ret = copy_from_kernel_nofault(&dst, addr, 1); >> + KUNIT_EXPECT_EQ(test, ret, -EFAULT); >> + KUNIT_EXPECT_FALSE(test, report_available()); >> +} >> + >> static void test_double_free(struct kunit *test) >> { >> const size_t size = 32; >> @@ -780,6 +796,7 @@ static struct kunit_case kfence_test_cases[] = { >> KFENCE_KUNIT_CASE(test_out_of_bounds_read), >> KFENCE_KUNIT_CASE(test_out_of_bounds_write), >> KFENCE_KUNIT_CASE(test_use_after_free_read), >> + KFENCE_KUNIT_CASE(test_use_after_free_read_nofault), >> KFENCE_KUNIT_CASE(test_double_free), >> KFENCE_KUNIT_CASE(test_invalid_addr_free), >> KFENCE_KUNIT_CASE(test_corruption), >> -- >> 2.46.0 >>
diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c index 00fd17285285..f65fb182466d 100644 --- a/mm/kfence/kfence_test.c +++ b/mm/kfence/kfence_test.c @@ -383,6 +383,22 @@ static void test_use_after_free_read(struct kunit *test) KUNIT_EXPECT_TRUE(test, report_matches(&expect)); } +static void test_use_after_free_read_nofault(struct kunit *test) +{ + const size_t size = 32; + char *addr; + char dst; + int ret; + + setup_test_cache(test, size, 0, NULL); + addr = test_alloc(test, size, GFP_KERNEL, ALLOCATE_ANY); + test_free(addr); + /* Use after free with *_nofault() */ + ret = copy_from_kernel_nofault(&dst, addr, 1); + KUNIT_EXPECT_EQ(test, ret, -EFAULT); + KUNIT_EXPECT_FALSE(test, report_available()); +} + static void test_double_free(struct kunit *test) { const size_t size = 32; @@ -780,6 +796,7 @@ static struct kunit_case kfence_test_cases[] = { KFENCE_KUNIT_CASE(test_out_of_bounds_read), KFENCE_KUNIT_CASE(test_out_of_bounds_write), KFENCE_KUNIT_CASE(test_use_after_free_read), + KFENCE_KUNIT_CASE(test_use_after_free_read_nofault), KFENCE_KUNIT_CASE(test_double_free), KFENCE_KUNIT_CASE(test_invalid_addr_free), KFENCE_KUNIT_CASE(test_corruption),