Re: [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode

From: Andrey Konovalov
Date: Fri Oct 16 2020 - 16:05:34 EST

Next message: Christian Eggers: "[PATCH net-next 3/3] net: dsa: trailer: don't allocate additional memory for padding/tagging"
Previous message: Christian Eggers: "[PATCH net-next 2/3] net: dsa: tag_ksz: don't allocate additional memory for padding/tagging"
In reply to: Andrey Konovalov: "[PATCH] kasan: adopt KUNIT tests to SW_TAGS mode"
Next in thread: David Gow: "Re: [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Oct 16, 2020 at 9:33 PM Andrey Konovalov <andreyknvl@xxxxxxxxxx> wrote:
>
> Now that we have KASAN-KUNIT tests integration, it's easy to see that
> some KASAN tests are not adopted to the SW_TAGS mode and are failing.
>
> Adjust the allocation size for kasan_memchr() and kasan_memcmp() by
> roung it up to OOB_TAG_OFF so the bad access ends up in a separate
> memory granule.
>
> Add new kmalloc_uaf_16() and kasan_bitops_uaf() tests that rely on UAFs,
> as it's hard to adopt the existing kmalloc_oob_16() and kasan_bitops_oob()
> (rename from kasan_bitops()) without losing the precision.
>
> Disable kasan_global_oob() and kasan_alloca_oob_left/right() as SW_TAGS
> mode doesn't instrument globals nor dynamic allocas.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> ---
> lib/test_kasan.c | 144 ++++++++++++++++++++++++++++++++---------------
> 1 file changed, 99 insertions(+), 45 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 63c26171a791..3bff25a7fdcc 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -216,6 +216,12 @@ static void kmalloc_oob_16(struct kunit *test)
> u64 words[2];
> } *ptr1, *ptr2;
>
> + /* This test is specifically crafted for the generic mode. */
> + if (!IS_ENABLED(CONFIG_KASAN_GENERIC)) {
> + kunit_info(test, "CONFIG_KASAN_GENERIC required\n");
> + return;
> + }
> +
> ptr1 = kmalloc(sizeof(*ptr1) - 3, GFP_KERNEL);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr1);
>
> @@ -227,6 +233,23 @@ static void kmalloc_oob_16(struct kunit *test)
> kfree(ptr2);
> }
>
> +static void kmalloc_uaf_16(struct kunit *test)
> +{
> + struct {
> + u64 words[2];
> + } *ptr1, *ptr2;
> +
> + ptr1 = kmalloc(sizeof(*ptr1), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr1);
> +
> + ptr2 = kmalloc(sizeof(*ptr2), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr2);
> + kfree(ptr2);
> +
> + KUNIT_EXPECT_KASAN_FAIL(test, *ptr1 = *ptr2);
> + kfree(ptr1);
> +}
> +
> static void kmalloc_oob_memset_2(struct kunit *test)
> {
> char *ptr;
> @@ -429,6 +452,12 @@ static void kasan_global_oob(struct kunit *test)
> volatile int i = 3;
> char *p = &global_array[ARRAY_SIZE(global_array) + i];
>
> + /* Only generic mode instruments globals. */
> + if (!IS_ENABLED(CONFIG_KASAN_GENERIC)) {
> + kunit_info(test, "CONFIG_KASAN_GENERIC required");
> + return;
> + }
> +
> KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)p);
> }
>
> @@ -467,6 +496,12 @@ static void kasan_alloca_oob_left(struct kunit *test)
> char alloca_array[i];
> char *p = alloca_array - 1;
>
> + /* Only generic mode instruments dynamic allocas. */
> + if (!IS_ENABLED(CONFIG_KASAN_GENERIC)) {
> + kunit_info(test, "CONFIG_KASAN_GENERIC required");
> + return;
> + }
> +
> if (!IS_ENABLED(CONFIG_KASAN_STACK)) {
> kunit_info(test, "CONFIG_KASAN_STACK is not enabled");
> return;
> @@ -481,6 +516,12 @@ static void kasan_alloca_oob_right(struct kunit *test)
> char alloca_array[i];
> char *p = alloca_array + i;
>
> + /* Only generic mode instruments dynamic allocas. */
> + if (!IS_ENABLED(CONFIG_KASAN_GENERIC)) {
> + kunit_info(test, "CONFIG_KASAN_GENERIC required");
> + return;
> + }
> +
> if (!IS_ENABLED(CONFIG_KASAN_STACK)) {
> kunit_info(test, "CONFIG_KASAN_STACK is not enabled");
> return;
> @@ -551,6 +592,9 @@ static void kasan_memchr(struct kunit *test)
> return;
> }
>
> + if (OOB_TAG_OFF)
> + size = round_up(size, OOB_TAG_OFF);
> +
> ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> @@ -573,6 +617,9 @@ static void kasan_memcmp(struct kunit *test)
> return;
> }
>
> + if (OOB_TAG_OFF)
> + size = round_up(size, OOB_TAG_OFF);
> +
> ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> memset(arr, 0, sizeof(arr));
> @@ -619,13 +666,50 @@ static void kasan_strings(struct kunit *test)
> KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = strnlen(ptr, 1));
> }
>
> -static void kasan_bitops(struct kunit *test)
> +static void kasan_bitops_modify(struct kunit *test, int nr, void *addr)
> +{
> + KUNIT_EXPECT_KASAN_FAIL(test, set_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, __set_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, clear_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, __clear_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, clear_bit_unlock(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, __clear_bit_unlock(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, change_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, __change_bit(nr, addr));
> +}
> +
> +static void kasan_bitops_test_and_modify(struct kunit *test, int nr, void *addr)
> {
> + KUNIT_EXPECT_KASAN_FAIL(test, test_and_set_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, __test_and_set_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, test_and_set_bit_lock(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, test_and_clear_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, __test_and_clear_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, test_and_change_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, __test_and_change_bit(nr, addr));
> + KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = test_bit(nr, addr));
> +
> +#if defined(clear_bit_unlock_is_negative_byte)
> + KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result =
> + clear_bit_unlock_is_negative_byte(nr, addr));
> +#endif
> +}
> +
> +static void kasan_bitops_oob(struct kunit *test)
> +{
> + long *bits;
> +
> + /* This test is specifically crafted for the generic mode. */
> + if (!IS_ENABLED(CONFIG_KASAN_GENERIC)) {
> + kunit_info(test, "CONFIG_KASAN_GENERIC required\n");
> + return;
> + }
> +
> /*
> * Allocate 1 more byte, which causes kzalloc to round up to 16-bytes;
> * this way we do not actually corrupt other memory.
> */
> - long *bits = kzalloc(sizeof(*bits) + 1, GFP_KERNEL);
> + bits = kzalloc(sizeof(*bits) + 1, GFP_KERNEL);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, bits);
>
> /*
> @@ -633,56 +717,24 @@ static void kasan_bitops(struct kunit *test)
> * below accesses are still out-of-bounds, since bitops are defined to
> * operate on the whole long the bit is in.
> */
> - KUNIT_EXPECT_KASAN_FAIL(test, set_bit(BITS_PER_LONG, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test, __set_bit(BITS_PER_LONG, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test, clear_bit(BITS_PER_LONG, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test, __clear_bit(BITS_PER_LONG, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test, clear_bit_unlock(BITS_PER_LONG, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test, __clear_bit_unlock(BITS_PER_LONG, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test, change_bit(BITS_PER_LONG, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test, __change_bit(BITS_PER_LONG, bits));
> + kasan_bitops_modify(test, BITS_PER_LONG, bits);
>
> /*
> * Below calls try to access bit beyond allocated memory.
> */
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - test_and_set_bit(BITS_PER_LONG + BITS_PER_BYTE, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - __test_and_set_bit(BITS_PER_LONG + BITS_PER_BYTE, bits));
> + kasan_bitops_test_and_modify(test, BITS_PER_LONG + BITS_PER_BYTE, bits);
>
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - test_and_set_bit_lock(BITS_PER_LONG + BITS_PER_BYTE, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - test_and_clear_bit(BITS_PER_LONG + BITS_PER_BYTE, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - __test_and_clear_bit(BITS_PER_LONG + BITS_PER_BYTE, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - test_and_change_bit(BITS_PER_LONG + BITS_PER_BYTE, bits));
> -
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - __test_and_change_bit(BITS_PER_LONG + BITS_PER_BYTE, bits));
> + kfree(bits);
> +}
>
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - kasan_int_result =
> - test_bit(BITS_PER_LONG + BITS_PER_BYTE, bits));
> +static void kasan_bitops_uaf(struct kunit *test)
> +{
> + long *bits = kzalloc(sizeof(*bits), GFP_KERNEL);
>
> -#if defined(clear_bit_unlock_is_negative_byte)
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - kasan_int_result = clear_bit_unlock_is_negative_byte(
> - BITS_PER_LONG + BITS_PER_BYTE, bits));
> -#endif
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, bits);
> kfree(bits);
> + kasan_bitops_modify(test, BITS_PER_LONG, bits);
> + kasan_bitops_test_and_modify(test, BITS_PER_LONG + BITS_PER_BYTE, bits);
> }

This actually ends up modifying the data in a freed object, which
isn't a good idea. I'll change this to do an OOB too, but for the
tag-based mode.

>
> static void kmalloc_double_kzfree(struct kunit *test)
> @@ -728,6 +780,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
> KUNIT_CASE(kmalloc_oob_krealloc_more),
> KUNIT_CASE(kmalloc_oob_krealloc_less),
> KUNIT_CASE(kmalloc_oob_16),
> + KUNIT_CASE(kmalloc_uaf_16),
> KUNIT_CASE(kmalloc_oob_in_memset),
> KUNIT_CASE(kmalloc_oob_memset_2),
> KUNIT_CASE(kmalloc_oob_memset_4),
> @@ -751,7 +804,8 @@ static struct kunit_case kasan_kunit_test_cases[] = {
> KUNIT_CASE(kasan_memchr),
> KUNIT_CASE(kasan_memcmp),
> KUNIT_CASE(kasan_strings),
> - KUNIT_CASE(kasan_bitops),
> + KUNIT_CASE(kasan_bitops_oob),
> + KUNIT_CASE(kasan_bitops_uaf),
> KUNIT_CASE(kmalloc_double_kzfree),
> KUNIT_CASE(vmalloc_oob),
> {}
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>

Next message: Christian Eggers: "[PATCH net-next 3/3] net: dsa: trailer: don't allocate additional memory for padding/tagging"
Previous message: Christian Eggers: "[PATCH net-next 2/3] net: dsa: tag_ksz: don't allocate additional memory for padding/tagging"
In reply to: Andrey Konovalov: "[PATCH] kasan: adopt KUNIT tests to SW_TAGS mode"
Next in thread: David Gow: "Re: [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]