diff mbox series

[2/8] kasan: test: avoid writing invalid memory

Message ID c3cd2a383e757e27dd9131635fc7d09a48a49cf9.1628709663.git.andreyknvl@gmail.com (mailing list archive)
State New
Headers show
Series kasan: test: avoid crashing the kernel with HW_TAGS | expand

Commit Message

andrey.konovalov@linux.dev Aug. 11, 2021, 7:21 p.m. UTC 
From: Andrey Konovalov <andreyknvl@gmail.com>

Multiple KASAN tests do writes past the allocated objects or writes to
freed memory. Turn these writes into reads to avoid corrupting memory.
Otherwise, these tests might lead to crashes with the HW_TAGS mode, as it
neither uses quarantine nor redzones.

Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com>
---
 lib/test_kasan.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Marco Elver Aug. 12, 2021, 8:57 a.m. UTC | #1 
On Wed, 11 Aug 2021 at 21:21, <andrey.konovalov@linux.dev> wrote:
> From: Andrey Konovalov <andreyknvl@gmail.com>
>
> Multiple KASAN tests do writes past the allocated objects or writes to
> freed memory. Turn these writes into reads to avoid corrupting memory.
> Otherwise, these tests might lead to crashes with the HW_TAGS mode, as it
> neither uses quarantine nor redzones.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com>

Reviewed-by: Marco Elver <elver@google.com>

although if you need a write primitive somewhere that doesn't corrupt
memory, you could use atomic_add() or atomic_or() of 0. Although
technically that's a read-modify-write. For generic mode one issue is
that these are explicitly instrumented and not through the compiler,
which is only a problem if you're testing the compiler emits the right
instrumentation.


> ---
>  lib/test_kasan.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 1bc3cdd2957f..c82a82eb5393 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -167,7 +167,7 @@ static void kmalloc_node_oob_right(struct kunit *test)
>         ptr = kmalloc_node(size, GFP_KERNEL, 0);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> -       KUNIT_EXPECT_KASAN_FAIL(test, ptr[size] = 0);
> +       KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] = ptr[size]);
>         kfree(ptr);
>  }
>
> @@ -203,7 +203,7 @@ static void kmalloc_pagealloc_uaf(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>         kfree(ptr);
>
> -       KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] = 0);
> +       KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]);
>  }
>
>  static void kmalloc_pagealloc_invalid_free(struct kunit *test)
> @@ -237,7 +237,7 @@ static void pagealloc_oob_right(struct kunit *test)
>         ptr = page_address(pages);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> -       KUNIT_EXPECT_KASAN_FAIL(test, ptr[size] = 0);
> +       KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] = ptr[size]);
>         free_pages((unsigned long)ptr, order);
>  }
>
> @@ -252,7 +252,7 @@ static void pagealloc_uaf(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>         free_pages((unsigned long)ptr, order);
>
> -       KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] = 0);
> +       KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]);
>  }
>
>  static void kmalloc_large_oob_right(struct kunit *test)
> @@ -514,7 +514,7 @@ static void kmalloc_uaf(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
>         kfree(ptr);
> -       KUNIT_EXPECT_KASAN_FAIL(test, *(ptr + 8) = 'x');
> +       KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[8]);
>  }
>
>  static void kmalloc_uaf_memset(struct kunit *test)
> @@ -553,7 +553,7 @@ static void kmalloc_uaf2(struct kunit *test)
>                 goto again;
>         }
>
> -       KUNIT_EXPECT_KASAN_FAIL(test, ptr1[40] = 'x');
> +       KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr1)[40]);
>         KUNIT_EXPECT_PTR_NE(test, ptr1, ptr2);
>
>         kfree(ptr2);
> @@ -700,7 +700,7 @@ static void ksize_unpoisons_memory(struct kunit *test)
>         ptr[size] = 'x';
>
>         /* This one must. */
> -       KUNIT_EXPECT_KASAN_FAIL(test, ptr[real_size] = 'y');
> +       KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]);
>
>         kfree(ptr);
>  }
> --
> 2.25.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/msgid/kasan-dev/c3cd2a383e757e27dd9131635fc7d09a48a49cf9.1628709663.git.andreyknvl%40gmail.com.
Andrey Konovalov Aug. 12, 2021, 1:02 p.m. UTC | #2 
On Thu, Aug 12, 2021 at 10:57 AM Marco Elver <elver@google.com> wrote:
>
> On Wed, 11 Aug 2021 at 21:21, <andrey.konovalov@linux.dev> wrote:
> > From: Andrey Konovalov <andreyknvl@gmail.com>
> >
> > Multiple KASAN tests do writes past the allocated objects or writes to
> > freed memory. Turn these writes into reads to avoid corrupting memory.
> > Otherwise, these tests might lead to crashes with the HW_TAGS mode, as it
> > neither uses quarantine nor redzones.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com>
>
> Reviewed-by: Marco Elver <elver@google.com>
>
> although if you need a write primitive somewhere that doesn't corrupt
> memory, you could use atomic_add() or atomic_or() of 0. Although
> technically that's a read-modify-write.

Interesting idea. I'd say let's keep the volatile reads for now, and
change them if we encounter any problem with those.

> For generic mode one issue is
> that these are explicitly instrumented and not through the compiler,
> which is only a problem if you're testing the compiler emits the right
> instrumentation.

On a related point, it seems we have no KASAN tests to check atomic operations.

Filed https://bugzilla.kernel.org/show_bug.cgi?id=214055 for this.

Thanks!
diff mbox series

Patch

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 1bc3cdd2957f..c82a82eb5393 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -167,7 +167,7 @@  static void kmalloc_node_oob_right(struct kunit *test)
 	ptr = kmalloc_node(size, GFP_KERNEL, 0);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
 
-	KUNIT_EXPECT_KASAN_FAIL(test, ptr[size] = 0);
+	KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] = ptr[size]);
 	kfree(ptr);
 }
 
@@ -203,7 +203,7 @@  static void kmalloc_pagealloc_uaf(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
 	kfree(ptr);
 
-	KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] = 0);
+	KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]);
 }
 
 static void kmalloc_pagealloc_invalid_free(struct kunit *test)
@@ -237,7 +237,7 @@  static void pagealloc_oob_right(struct kunit *test)
 	ptr = page_address(pages);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
 
-	KUNIT_EXPECT_KASAN_FAIL(test, ptr[size] = 0);
+	KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] = ptr[size]);
 	free_pages((unsigned long)ptr, order);
 }
 
@@ -252,7 +252,7 @@  static void pagealloc_uaf(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
 	free_pages((unsigned long)ptr, order);
 
-	KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] = 0);
+	KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]);
 }
 
 static void kmalloc_large_oob_right(struct kunit *test)
@@ -514,7 +514,7 @@  static void kmalloc_uaf(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
 
 	kfree(ptr);
-	KUNIT_EXPECT_KASAN_FAIL(test, *(ptr + 8) = 'x');
+	KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[8]);
 }
 
 static void kmalloc_uaf_memset(struct kunit *test)
@@ -553,7 +553,7 @@  static void kmalloc_uaf2(struct kunit *test)
 		goto again;
 	}
 
-	KUNIT_EXPECT_KASAN_FAIL(test, ptr1[40] = 'x');
+	KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr1)[40]);
 	KUNIT_EXPECT_PTR_NE(test, ptr1, ptr2);
 
 	kfree(ptr2);
@@ -700,7 +700,7 @@  static void ksize_unpoisons_memory(struct kunit *test)
 	ptr[size] = 'x';
 
 	/* This one must. */
-	KUNIT_EXPECT_KASAN_FAIL(test, ptr[real_size] = 'y');
+	KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]);
 
 	kfree(ptr);
 }