[PATCH v3] mm/kfence: Add a new kunit test test_use_after_free_read_nofault()

[PATCH v3] mm/kfence: Add a new kunit test test_use_after_free_read_nofault()

[Ritesh Harjani (IBM)]

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.

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

Re: [PATCH v3] mm/kfence: Add a new kunit test test_use_after_free_read_nofault()

[Marco Elver]

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
>

Re: [PATCH v3] mm/kfence: Add a new kunit test test_use_after_free_read_nofault()

[Ritesh Harjani]

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