| Message ID | 20240524232804.1984355-1-bjohannesmeyer@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kmsan: introduce test_unpoison_memory() | expand |
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.
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
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), {}, };
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(+)