linux-kernel - Re: [PATCH] kmsan: introduce test_unpoison

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG_fn=U2U5j8VxrkKGHEOdbpheVXM08ExFwkqNhz4qv2EtTjWg@mail.gmail.com>
Date: Tue, 28 May 2024 12:20:15 +0200
From: Alexander Potapenko <glider@...gle.com>
To: Brian Johannesmeyer <bjohannesmeyer@...il.com>
Cc: Marco Elver <elver@...gle.com>, Dmitry Vyukov <dvyukov@...gle.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, kasan-dev@...glegroups.com, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kmsan: introduce test_unpoison_memory()

On Sat, May 25, 2024 at 1:28 AM Brian Johannesmeyer
<bjohannesmeyer@...il.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@...il.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@...glegroups.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