[PATCH] kmsan: introduce test_unpoison_memory()

[PATCH] kmsan: introduce test_unpoison_memory()

[Brian Johannesmeyer]

Add a regression test to ensure that kmsan_unpoison_memory() works the same
as an unpoisoning operation added by the instrumentation. (Of course,
please correct me if I'm misunderstanding how these should work).

The test has two subtests: one that checks the instrumentation, and one
that checks kmsan_unpoison_memory(). Each subtest initializes the first
byte of a 4-byte buffer, then checks that the other 3 bytes are
uninitialized. Unfortunately, the test for kmsan_unpoison_memory() fails to
identify the 3 bytes as uninitialized (i.e., the line with the comment
"Fail: No UMR report").

As to my guess why this is happening: From kmsan_unpoison_memory(), the
backing shadow is indeed correctly overwritten in
kmsan_internal_set_shadow_origin() via `__memset(shadow_start, b, size);`.
Instead, the issue seems to stem from overwriting the backing origin, in
the following `origin_start[i] = origin;` loop; if we return before that
loop on this specific call to kmsan_unpoison_memory(), then the test
passes.

Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
---
 mm/kmsan/kmsan_test.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
index 07d3a3a5a9c5..c3ab90df0abf 100644
--- a/mm/kmsan/kmsan_test.c
+++ b/mm/kmsan/kmsan_test.c
@@ -614,6 +614,30 @@ static void test_stackdepot_roundtrip(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, report_matches(&expect));
 }
 
+/*
+ * Test case: ensure that kmsan_unpoison_memory() and the instrumentation work
+ * the same
+ */
+static void test_unpoison_memory(struct kunit *test)
+{
+	EXPECTATION_UNINIT_VALUE_FN(expect, "test_unpoison_memory");
+	volatile char a[4], b[4];
+
+	kunit_info(
+		test,
+		"unpoisoning via the instrumentation vs. kmsan_unpoison_memory() (2 UMR reports)\n");
+
+	a[0] = 0;                                     // Initialize a[0]
+	kmsan_check_memory((char *)&a[1], 3);         // Check a[1]--a[3]
+	KUNIT_EXPECT_TRUE(test, report_matches(&expect)); // Pass: UMR report
+
+	report_reset();
+
+	kmsan_unpoison_memory((char *)&b[0], 1);  // Initialize b[0]
+	kmsan_check_memory((char *)&b[1], 3);     // Check b[1]--b[3]
+	KUNIT_EXPECT_TRUE(test, report_matches(&expect)); // Fail: No UMR report
+}
+
 static struct kunit_case kmsan_test_cases[] = {
 	KUNIT_CASE(test_uninit_kmalloc),
 	KUNIT_CASE(test_init_kmalloc),
@@ -637,6 +661,7 @@ static struct kunit_case kmsan_test_cases[] = {
 	KUNIT_CASE(test_memset64),
 	KUNIT_CASE(test_long_origin_chain),
 	KUNIT_CASE(test_stackdepot_roundtrip),
+	KUNIT_CASE(test_unpoison_memory),
 	{},
 };
 
-- 
2.34.1

Re: [PATCH] kmsan: introduce test_unpoison_memory()

[Alexander Potapenko]

On Sat, May 25, 2024 at 1:28 AM Brian Johannesmeyer
<bjohannesmeyer@gmail.com> wrote:
>
> Add a regression test to ensure that kmsan_unpoison_memory() works the same
> as an unpoisoning operation added by the instrumentation. (Of course,
> please correct me if I'm misunderstanding how these should work).
>
> The test has two subtests: one that checks the instrumentation, and one
> that checks kmsan_unpoison_memory(). Each subtest initializes the first
> byte of a 4-byte buffer, then checks that the other 3 bytes are
> uninitialized. Unfortunately, the test for kmsan_unpoison_memory() fails to
> identify the 3 bytes as uninitialized (i.e., the line with the comment
> "Fail: No UMR report").
>
> As to my guess why this is happening: From kmsan_unpoison_memory(), the
> backing shadow is indeed correctly overwritten in
> kmsan_internal_set_shadow_origin() via `__memset(shadow_start, b, size);`.
> Instead, the issue seems to stem from overwriting the backing origin, in
> the following `origin_start[i] = origin;` loop; if we return before that
> loop on this specific call to kmsan_unpoison_memory(), then the test
> passes.

Hi Brian,

You are right with your analysis.
KMSAN stores a single origin for every aligned four-byte granule of
memory, so we lose some information when more than one uninitialized
value is combined in that granule.
When writing an uninitialized value to memory, a viable strategy is to
always update the origin. But if we partially initialize the granule
with a store, it is better to preserve that granule's origin to
prevent false negatives, so we need to check the resulting shadow slot
before updating the origin.
This is what the compiler instrumentation does, so
kmsan_internal_set_shadow_origin() should behave in the same way.
I found a similar bug in kmsan_internal_memmove_metadata() last year,
but missed this one.

I am going to send a patch fixing this along with your test (with an
updated description), if you don't object.

> Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> ---
>  mm/kmsan/kmsan_test.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
> index 07d3a3a5a9c5..c3ab90df0abf 100644
> --- a/mm/kmsan/kmsan_test.c
> +++ b/mm/kmsan/kmsan_test.c
> @@ -614,6 +614,30 @@ static void test_stackdepot_roundtrip(struct kunit *test)
>         KUNIT_EXPECT_TRUE(test, report_matches(&expect));
>  }
>
> +/*
> + * Test case: ensure that kmsan_unpoison_memory() and the instrumentation work
> + * the same
> + */
> +static void test_unpoison_memory(struct kunit *test)
> +{
> +       EXPECTATION_UNINIT_VALUE_FN(expect, "test_unpoison_memory");
> +       volatile char a[4], b[4];
> +
> +       kunit_info(
> +               test,
> +               "unpoisoning via the instrumentation vs. kmsan_unpoison_memory() (2 UMR reports)\n");
> +
> +       a[0] = 0;                                     // Initialize a[0]
> +       kmsan_check_memory((char *)&a[1], 3);         // Check a[1]--a[3]
> +       KUNIT_EXPECT_TRUE(test, report_matches(&expect)); // Pass: UMR report
> +
> +       report_reset();
> +
> +       kmsan_unpoison_memory((char *)&b[0], 1);  // Initialize b[0]
> +       kmsan_check_memory((char *)&b[1], 3);     // Check b[1]--b[3]
> +       KUNIT_EXPECT_TRUE(test, report_matches(&expect)); // Fail: No UMR report
> +}
> +
>  static struct kunit_case kmsan_test_cases[] = {
>         KUNIT_CASE(test_uninit_kmalloc),
>         KUNIT_CASE(test_init_kmalloc),
> @@ -637,6 +661,7 @@ static struct kunit_case kmsan_test_cases[] = {
>         KUNIT_CASE(test_memset64),
>         KUNIT_CASE(test_long_origin_chain),
>         KUNIT_CASE(test_stackdepot_roundtrip),
> +       KUNIT_CASE(test_unpoison_memory),
>         {},
>  };
>
> --
> 2.34.1
>
> --
> 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/20240524232804.1984355-1-bjohannesmeyer%40gmail.com.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH] kmsan: introduce test_unpoison_memory()

[Brian Johannesmeyer]

On Tue, May 28, 2024 at 12:20:15PM +0200, Alexander Potapenko wrote:
> You are right with your analysis.
> KMSAN stores a single origin for every aligned four-byte granule of
> memory, so we lose some information when more than one uninitialized
> value is combined in that granule.
> When writing an uninitialized value to memory, a viable strategy is to
> always update the origin. But if we partially initialize the granule
> with a store, it is better to preserve that granule's origin to
> prevent false negatives, so we need to check the resulting shadow slot
> before updating the origin.
> This is what the compiler instrumentation does, so
> kmsan_internal_set_shadow_origin() should behave in the same way.
> I found a similar bug in kmsan_internal_memmove_metadata() last year,
> but missed this one.

I appreciate the explanation. Makes sense.

> I am going to send a patch fixing this along with your test (with an
> updated description), if you don't object.

Yes, that's fine. Thank you.

-Brian