Re: [PATCH -tip v4 3/3] kasan: test: Fix test for new meminstrinsic instrumentation

[Andrey Konovalov]

On Fri, Feb 17, 2023 at 12:45 AM Marco Elver <elver@xxxxxxxxxx> wrote:
>
> The tests for memset/memmove have been failing since they haven't been
> instrumented in 69d4c0d32186.
>
> Fix the test to recognize when memintrinsics aren't instrumented, and
> skip test cases accordingly. We also need to conditionally pass
> -fno-builtin to the test, otherwise the instrumentation pass won't
> recognize memintrinsics and end up not instrumenting them either.
>
> Fixes: 69d4c0d32186 ("entry, kasan, x86: Disallow overriding mem*() functions")
> Reported-by: Linux Kernel Functional Testing <lkft@xxxxxxxxxx>
> Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> ---
> v4:
> * New patch.
> ---
>  mm/kasan/Makefile     |  9 ++++++++-
>  mm/kasan/kasan_test.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
> index d4837bff3b60..7634dd2a6128 100644
> --- a/mm/kasan/Makefile
> +++ b/mm/kasan/Makefile
> @@ -35,7 +35,14 @@ CFLAGS_shadow.o := $(CC_FLAGS_KASAN_RUNTIME)
>  CFLAGS_hw_tags.o := $(CC_FLAGS_KASAN_RUNTIME)
>  CFLAGS_sw_tags.o := $(CC_FLAGS_KASAN_RUNTIME)
>
> -CFLAGS_KASAN_TEST := $(CFLAGS_KASAN) -fno-builtin $(call cc-disable-warning, vla)
> +CFLAGS_KASAN_TEST := $(CFLAGS_KASAN) $(call cc-disable-warning, vla)
> +ifndef CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX
> +# If compiler instruments memintrinsics by prefixing them with __asan/__hwasan,
> +# we need to treat them normally (as builtins), otherwise the compiler won't
> +# recognize them as instrumentable. If it doesn't instrument them, we need to
> +# pass -fno-builtin, so the compiler doesn't inline them.
> +CFLAGS_KASAN_TEST += -fno-builtin
> +endif
>
>  CFLAGS_kasan_test.o := $(CFLAGS_KASAN_TEST)
>  CFLAGS_kasan_test_module.o := $(CFLAGS_KASAN_TEST)
> diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
> index 74cd80c12b25..627eaf1ee1db 100644
> --- a/mm/kasan/kasan_test.c
> +++ b/mm/kasan/kasan_test.c
> @@ -165,6 +165,15 @@ static void kasan_test_exit(struct kunit *test)
>                 kunit_skip((test), "Test requires " #config "=n");      \
>  } while (0)
>
> +#define KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test) do {              \
> +       if (IS_ENABLED(CONFIG_KASAN_HW_TAGS))                           \
> +               break;  /* No compiler instrumentation. */              \
> +       if (IS_ENABLED(CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX))        \
> +               break;  /* Should always be instrumented! */            \
> +       if (IS_ENABLED(CONFIG_GENERIC_ENTRY))                           \
> +               kunit_skip((test), "Test requires checked mem*()");     \
> +} while (0)
> +
>  static void kmalloc_oob_right(struct kunit *test)
>  {
>         char *ptr;
> @@ -454,6 +463,8 @@ static void kmalloc_oob_16(struct kunit *test)
>                 u64 words[2];
>         } *ptr1, *ptr2;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         /* This test is specifically crafted for the generic mode. */
>         KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
>
> @@ -476,6 +487,8 @@ static void kmalloc_uaf_16(struct kunit *test)
>                 u64 words[2];
>         } *ptr1, *ptr2;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         ptr1 = kmalloc(sizeof(*ptr1), GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr1);
>
> @@ -498,6 +511,8 @@ static void kmalloc_oob_memset_2(struct kunit *test)
>         char *ptr;
>         size_t size = 128 - KASAN_GRANULE_SIZE;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         ptr = kmalloc(size, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> @@ -511,6 +526,8 @@ static void kmalloc_oob_memset_4(struct kunit *test)
>         char *ptr;
>         size_t size = 128 - KASAN_GRANULE_SIZE;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         ptr = kmalloc(size, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> @@ -524,6 +541,8 @@ static void kmalloc_oob_memset_8(struct kunit *test)
>         char *ptr;
>         size_t size = 128 - KASAN_GRANULE_SIZE;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         ptr = kmalloc(size, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> @@ -537,6 +556,8 @@ static void kmalloc_oob_memset_16(struct kunit *test)
>         char *ptr;
>         size_t size = 128 - KASAN_GRANULE_SIZE;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         ptr = kmalloc(size, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> @@ -550,6 +571,8 @@ static void kmalloc_oob_in_memset(struct kunit *test)
>         char *ptr;
>         size_t size = 128 - KASAN_GRANULE_SIZE;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         ptr = kmalloc(size, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> @@ -566,6 +589,8 @@ static void kmalloc_memmove_negative_size(struct kunit *test)
>         size_t size = 64;
>         size_t invalid_size = -2;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         /*
>          * Hardware tag-based mode doesn't check memmove for negative size.
>          * As a result, this test introduces a side-effect memory corruption,
> @@ -590,6 +615,8 @@ static void kmalloc_memmove_invalid_size(struct kunit *test)
>         size_t size = 64;
>         size_t invalid_size = size;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         ptr = kmalloc(size, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> @@ -618,6 +645,8 @@ static void kmalloc_uaf_memset(struct kunit *test)
>         char *ptr;
>         size_t size = 33;
>
> +       KASAN_TEST_NEEDS_CHECKED_MEMINTRINSICS(test);
> +
>         /*
>          * Only generic KASAN uses quarantine, which is required to avoid a
>          * kernel memory corruption this test causes.
> --
> 2.39.2.637.g21b0678d19-goog
>

Reviewed-by: Andrey Konovalov <andreyknvl@xxxxxxxxx>

Thank you for taking care of all of this, Marco!