Received-SPF: pass (google.com: domain of linux-kernel+bounces-192102-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240524232804.1984355-1-bjohannesmeyer@gmail.com>
In-Reply-To: <20240524232804.1984355-1-bjohannesmeyer@gmail.com>
From: Alexander Potapenko <glider@google.com>
Date: Tue, 28 May 2024 12:20:15 +0200
Message-ID: <CAG_fn=U2U5j8VxrkKGHEOdbpheVXM08ExFwkqNhz4qv2EtTjWg@mail.gmail.com>
Subject: Re: [PATCH] kmsan: introduce test_unpoison_memory()
To: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
Cc: Marco Elver <elver@google.com>, Dmitry Vyukov <dvyukov@google.com>, 
	Andrew Morton <akpm@linux-foundation.org>, kasan-dev@googlegroups.com, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, May 25, 2024 at 1:28=E2=80=AFAM Brian Johannesmeyer
<bjohannesmeyer@gmail.com> wrote:
>
> Add a regression test to ensure that kmsan_unpoison_memory() works the sa=
me
> 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] =3D origin;` loop; if we return before tha=
t
> 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 instrumentatio=
n 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_m=
emory() (2 UMR reports)\n");
> +
> +       a[0] =3D 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 re=
port
> +
> +       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[] =3D {
>         KUNIT_CASE(test_uninit_kmalloc),
>         KUNIT_CASE(test_init_kmalloc),
> @@ -637,6 +661,7 @@ static struct kunit_case kmsan_test_cases[] =3D {
>         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/msgi=
d/kasan-dev/20240524232804.1984355-1-bjohannesmeyer%40gmail.com.


--=20
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Stra=C3=9Fe, 33
80636 M=C3=BCnchen

Gesch=C3=A4ftsf=C3=BChrer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg