* [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode
@ 2020-10-16 19:33 Andrey Konovalov
2020-10-16 20:05 ` Andrey Konovalov
2020-10-17 7:42 ` David Gow
0 siblings, 2 replies; 4+ messages in thread
From: Andrey Konovalov @ 2020-10-16 19:33 UTC (permalink / raw)
To: Andrew Morton
Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Marco Elver,
Vincenzo Frascino, David Gow, kasan-dev, linux-mm, linux-kernel,
Andrey Konovalov
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@google.com>
---
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);
}
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode
2020-10-16 19:33 [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode Andrey Konovalov
@ 2020-10-16 20:05 ` Andrey Konovalov
2020-10-17 7:42 ` David Gow
1 sibling, 0 replies; 4+ messages in thread
From: Andrey Konovalov @ 2020-10-16 20:05 UTC (permalink / raw)
To: Andrew Morton
Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Marco Elver,
Vincenzo Frascino, David Gow, kasan-dev,
Linux Memory Management List, LKML
On Fri, Oct 16, 2020 at 9:33 PM Andrey Konovalov <andreyknvl@google.com> 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@google.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode
2020-10-16 19:33 [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode Andrey Konovalov
2020-10-16 20:05 ` Andrey Konovalov
@ 2020-10-17 7:42 ` David Gow
2020-10-19 17:33 ` Andrey Konovalov
1 sibling, 1 reply; 4+ messages in thread
From: David Gow @ 2020-10-17 7:42 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Andrew Morton, Andrey Ryabinin, Alexander Potapenko,
Dmitry Vyukov, Marco Elver, Vincenzo Frascino, kasan-dev,
Linux Memory Management List, Linux Kernel Mailing List
On Sat, Oct 17, 2020 at 3:33 AM Andrey Konovalov <andreyknvl@google.com> 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@google.com>
This looks good to me. Though, as you mention, writing to freed memory
might not bode well for system stability after the test runs. I don't
think that needs to be a goal for these tests, though.
One thing which we're hoping to add to KUnit soon is support for
skipping tests: once that's in place, we can use it to mark tests as
explicitly skipped if they rely on the GENERIC mode. That'll take a
little while to get upstream though, so I wouldn't want to hold this
up for it.
Otherwise, from the KUnit side, this looks great.
I also tested it against the GENERIC mode on x86_64 (which is all I
have set up here at the moment), and nothing obviously had broken.
So:
Tested-by: David Gow <davidgow@google.com>
Cheers,
-- David
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode
2020-10-17 7:42 ` David Gow
@ 2020-10-19 17:33 ` Andrey Konovalov
0 siblings, 0 replies; 4+ messages in thread
From: Andrey Konovalov @ 2020-10-19 17:33 UTC (permalink / raw)
To: David Gow
Cc: Andrew Morton, Andrey Ryabinin, Alexander Potapenko,
Dmitry Vyukov, Marco Elver, Vincenzo Frascino, kasan-dev,
Linux Memory Management List, Linux Kernel Mailing List
On Sat, Oct 17, 2020 at 9:42 AM David Gow <davidgow@google.com> wrote:
>
Hi David,
[...]
> This looks good to me. Though, as you mention, writing to freed memory
> might not bode well for system stability after the test runs. I don't
> think that needs to be a goal for these tests, though.
We generally tried to avoid this, since we run multiple tests, and if
one crashes the kernel, the rest won't work. I'll fix this in v2.
> One thing which we're hoping to add to KUnit soon is support for
> skipping tests: once that's in place, we can use it to mark tests as
> explicitly skipped if they rely on the GENERIC mode. That'll take a
> little while to get upstream though, so I wouldn't want to hold this
> up for it.
This will indeed be useful.
> Otherwise, from the KUnit side, this looks great.
>
> I also tested it against the GENERIC mode on x86_64 (which is all I
> have set up here at the moment), and nothing obviously had broken.
> So:
> Tested-by: David Gow <davidgow@google.com>
Perfect, thank you!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-19 17:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 19:33 [PATCH] kasan: adopt KUNIT tests to SW_TAGS mode Andrey Konovalov
2020-10-16 20:05 ` Andrey Konovalov
2020-10-17 7:42 ` David Gow
2020-10-19 17:33 ` Andrey Konovalov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).