[PATCH 0/8] kasan: test: avoid crashing the kernel with HW_TAGS

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

KASAN tests do out-of-bounds and use-after-free accesses. Running the
tests works fine for the GENERIC mode, as it uses qurantine and redzones.
But the HW_TAGS mode uses neither, and running the tests might crash
the kernel.

Rework the tests to avoid corrupting kernel memory.

Andrey Konovalov (8):
kasan: test: rework kmalloc_oob_right
kasan: test: avoid writing invalid memory
kasan: test: avoid corrupting memory via memset
kasan: test: disable kmalloc_memmove_invalid_size for HW_TAGS
kasan: test: only do kmalloc_uaf_memset for generic mode
kasan: test: clean up ksize_uaf
kasan: test: avoid corrupting memory in copy_user_test
kasan: test: avoid corrupting memory in kasan_rcu_uaf

lib/test_kasan.c | 74 ++++++++++++++++++++++++++++-------------
lib/test_kasan_module.c | 20 +++++------
2 files changed, 60 insertions(+), 34 deletions(-)

--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

Rework kmalloc_oob_right() to do these bad access checks:

1. An unaligned access one byte past the requested kmalloc size
(can only be detected by KASAN_GENERIC).
2. An aligned access into the first out-of-bounds granule that falls
within the aligned kmalloc object.
3. Out-of-bounds access past the aligned kmalloc object.

Test #3 deliberately uses a read access to avoid corrupting memory.
Otherwise, this test might lead to crashes with the HW_TAGS mode, as it
neither uses quarantine nor redzones.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---
lib/test_kasan.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 8f7b0b2f6e11..1bc3cdd2957f 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -122,12 +122,28 @@ static void kasan_test_exit(struct kunit *test)
static void kmalloc_oob_right(struct kunit *test)
{
char *ptr;
- size_t size = 123;
+ size_t size = 128 - KASAN_GRANULE_SIZE - 5;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, ptr[size + OOB_TAG_OFF] = 'x');
+ /*
+ * An unaligned access past the requested kmalloc size.
+ * Only generic KASAN can precisely detect these.
+ */
+ if (IS_ENABLED(CONFIG_KASAN_GENERIC))
+ KUNIT_EXPECT_KASAN_FAIL(test, ptr[size] = 'x');
+
+ /*
+ * An aligned access into the first out-of-bounds granule that falls
+ * within the aligned kmalloc object.
+ */
+ KUNIT_EXPECT_KASAN_FAIL(test, ptr[size + 5] = 'y');
+
+ /* Out-of-bounds access past the aligned kmalloc object. */
+ KUNIT_EXPECT_KASAN_FAIL(test, ptr[0] =
+ ptr[size + KASAN_GRANULE_SIZE + 5]);
+
kfree(ptr);
}

--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@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 <andre...@gmail.com>
---

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

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

kmalloc_oob_memset_*() tests do writes past the allocated objects.
As the result, they corrupt memory, which might lead to crashes with the

HW_TAGS mode, as it neither uses quarantine nor redzones.

Adjust the tests to only write memory within the aligned kmalloc objects.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index c82a82eb5393..fd00cd35e82c 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -431,61 +431,61 @@ static void kmalloc_uaf_16(struct kunit *test)
static void kmalloc_oob_memset_2(struct kunit *test)
{
char *ptr;
- size_t size = 8;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 7 + OOB_TAG_OFF, 0, 2));
+ KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size, 0, 2));
kfree(ptr);
}

static void kmalloc_oob_memset_4(struct kunit *test)
{
char *ptr;
- size_t size = 8;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 5 + OOB_TAG_OFF, 0, 4));
+ KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size, 0, 4));
kfree(ptr);
}

-
static void kmalloc_oob_memset_8(struct kunit *test)
{
char *ptr;
- size_t size = 8;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 1 + OOB_TAG_OFF, 0, 8));
+ KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size, 0, 8));
kfree(ptr);
}

static void kmalloc_oob_memset_16(struct kunit *test)
{
char *ptr;
- size_t size = 16;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 1 + OOB_TAG_OFF, 0, 16));
+ KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size, 0, 16));
kfree(ptr);
}

static void kmalloc_oob_in_memset(struct kunit *test)
{
char *ptr;
- size_t size = 666;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr, 0, size + 5 + OOB_TAG_OFF));
+ KUNIT_EXPECT_KASAN_FAIL(test,
+ memset(ptr, 0, size + KASAN_GRANULE_SIZE));
kfree(ptr);
}

--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

The HW_TAGS mode doesn't check memmove for negative size. As a result,
the kmalloc_memmove_invalid_size test corrupts memory, which can result
in a crash.

Disable this test with HW_TAGS KASAN.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index fd00cd35e82c..0b5698cd7d1d 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -495,11 +495,17 @@ static void kmalloc_memmove_invalid_size(struct kunit *test)
size_t size = 64;
volatile size_t invalid_size = -2;

+ /*
+ * Hardware tag-based mode doesn't check memmove for negative size.
+ * As a result, this test introduces a side-effect memory corruption,
+ * which can result in a crash.
+ */
+ KASAN_TEST_NEEDS_CONFIG_OFF(test, CONFIG_KASAN_HW_TAGS);
+

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

memset((char *)ptr, 0, 64);
-
KUNIT_EXPECT_KASAN_FAIL(test,
memmove((char *)ptr, (char *)ptr + 4, invalid_size));
kfree(ptr);
--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

kmalloc_uaf_memset() writes to freed memory, which is only safe with the
GENERIC mode (as it uses quarantine). For other modes, this test corrupts
kernel memory, which might result in a crash.

Only enable kmalloc_uaf_memset() for the GENERIC mode.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 0b5698cd7d1d..efd0da5c750f 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -528,6 +528,12 @@ static void kmalloc_uaf_memset(struct kunit *test)
char *ptr;
size_t size = 33;

+ /*
+ * Only generic KASAN uses quarantine, which is required to avoid a
+ * kernel memory corruption this test causes.
+ */
+ KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);

+
ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

Some KASAN tests use global variables to store function returns values
so that the compiler doesn't optimize away these functions.

ksize_uaf() doesn't call any functions, so it doesn't need to use
kasan_int_result. Use volatile accesses instead, to be consistent with
other similar tests.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index efd0da5c750f..e159d24b3b49 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -731,8 +731,8 @@ static void ksize_uaf(struct kunit *test)
kfree(ptr);

KUNIT_EXPECT_KASAN_FAIL(test, ksize(ptr));
- KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = *ptr);
- KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = *(ptr + size));

+ KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]);

+ KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[size]);
}

static void kasan_stack_oob(struct kunit *test)
--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

copy_user_test() does writes past the allocated object. As the result,
it corrupts kernel memory, which might lead to crashes with the HW_TAGS

mode, as it neither uses quarantine nor redzones.

(Technically, this test can't yet be enabled with the HW_TAGS mode, but
this will be implemented in the future.)

Adjust the test to only write memory within the aligned kmalloc object.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan_module.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/lib/test_kasan_module.c b/lib/test_kasan_module.c
index f1017f345d6c..fa73b9df0be4 100644
--- a/lib/test_kasan_module.c
+++ b/lib/test_kasan_module.c
@@ -15,13 +15,11 @@

#include "../mm/kasan/kasan.h"

-#define OOB_TAG_OFF (IS_ENABLED(CONFIG_KASAN_GENERIC) ? 0 : KASAN_GRANULE_SIZE)
-
static noinline void __init copy_user_test(void)
{
char *kmem;
char __user *usermem;
- size_t size = 10;

+ size_t size = 128 - KASAN_GRANULE_SIZE;

int __maybe_unused unused;

kmem = kmalloc(size, GFP_KERNEL);
@@ -38,25 +36,25 @@ static noinline void __init copy_user_test(void)
}

pr_info("out-of-bounds in copy_from_user()\n");
- unused = copy_from_user(kmem, usermem, size + 1 + OOB_TAG_OFF);
+ unused = copy_from_user(kmem, usermem, size + 1);

pr_info("out-of-bounds in copy_to_user()\n");
- unused = copy_to_user(usermem, kmem, size + 1 + OOB_TAG_OFF);
+ unused = copy_to_user(usermem, kmem, size + 1);

pr_info("out-of-bounds in __copy_from_user()\n");
- unused = __copy_from_user(kmem, usermem, size + 1 + OOB_TAG_OFF);
+ unused = __copy_from_user(kmem, usermem, size + 1);

pr_info("out-of-bounds in __copy_to_user()\n");
- unused = __copy_to_user(usermem, kmem, size + 1 + OOB_TAG_OFF);
+ unused = __copy_to_user(usermem, kmem, size + 1);

pr_info("out-of-bounds in __copy_from_user_inatomic()\n");
- unused = __copy_from_user_inatomic(kmem, usermem, size + 1 + OOB_TAG_OFF);
+ unused = __copy_from_user_inatomic(kmem, usermem, size + 1);

pr_info("out-of-bounds in __copy_to_user_inatomic()\n");
- unused = __copy_to_user_inatomic(usermem, kmem, size + 1 + OOB_TAG_OFF);
+ unused = __copy_to_user_inatomic(usermem, kmem, size + 1);

pr_info("out-of-bounds in strncpy_from_user()\n");
- unused = strncpy_from_user(kmem, usermem, size + 1 + OOB_TAG_OFF);
+ unused = strncpy_from_user(kmem, usermem, size + 1);

vm_munmap((unsigned long)usermem, PAGE_SIZE);
kfree(kmem);
--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

kasan_rcu_uaf() writes to freed memory via kasan_rcu_reclaim(), which is

only safe with the GENERIC mode (as it uses quarantine). For other modes,

this test corrupts kernel memory, which might result in a crash.

Turn the write into a read.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan_module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_kasan_module.c b/lib/test_kasan_module.c
index fa73b9df0be4..7ebf433edef3 100644
--- a/lib/test_kasan_module.c
+++ b/lib/test_kasan_module.c
@@ -71,7 +71,7 @@ static noinline void __init kasan_rcu_reclaim(struct rcu_head *rp)
struct kasan_rcu_info, rcu);

kfree(fp);
- fp->i = 1;
+ ((volatile struct kasan_rcu_info *)fp)->i;
}

static noinline void __init kasan_rcu_uaf(void)
--
2.25.1

[Marco Elver]

On Wed, 11 Aug 2021 at 21:34, <andrey.k...@linux.dev> wrote:
>
> From: Andrey Konovalov <andre...@gmail.com>
>
> kasan_rcu_uaf() writes to freed memory via kasan_rcu_reclaim(), which is
> only safe with the GENERIC mode (as it uses quarantine). For other modes,
> this test corrupts kernel memory, which might result in a crash.
>
> Turn the write into a read.
>
> Signed-off-by: Andrey Konovalov <andre...@gmail.com>

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

[Marco Elver]

On Wed, 11 Aug 2021 at 21:30, <andrey.k...@linux.dev> wrote:
> From: Andrey Konovalov <andre...@gmail.com>
>
> copy_user_test() does writes past the allocated object. As the result,
> it corrupts kernel memory, which might lead to crashes with the HW_TAGS
> mode, as it neither uses quarantine nor redzones.
>
> (Technically, this test can't yet be enabled with the HW_TAGS mode, but
> this will be implemented in the future.)
>
> Adjust the test to only write memory within the aligned kmalloc object.
>
> Signed-off-by: Andrey Konovalov <andre...@gmail.com>

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

[Marco Elver]

On Wed, 11 Aug 2021 at 21:23, <andrey.k...@linux.dev> wrote:
>
> From: Andrey Konovalov <andre...@gmail.com>
>
> Some KASAN tests use global variables to store function returns values
> so that the compiler doesn't optimize away these functions.
>
> ksize_uaf() doesn't call any functions, so it doesn't need to use
> kasan_int_result. Use volatile accesses instead, to be consistent with
> other similar tests.
>
> Signed-off-by: Andrey Konovalov <andre...@gmail.com>

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

Although I do wonder if the compiler might one day mess with the
volatile reads. At least this way we might also catch if the compiler
messes up volatile reads. ;-)

[Marco Elver]

On Wed, 11 Aug 2021 at 21:21, <andrey.k...@linux.dev> wrote:
> From: Andrey Konovalov <andre...@gmail.com>
>
> kmalloc_uaf_memset() writes to freed memory, which is only safe with the
> GENERIC mode (as it uses quarantine). For other modes, this test corrupts
> kernel memory, which might result in a crash.
>
> Only enable kmalloc_uaf_memset() for the GENERIC mode.
>
> Signed-off-by: Andrey Konovalov <andre...@gmail.com>

Acked-by: Marco Elver <el...@google.com>

[Marco Elver]

On Wed, 11 Aug 2021 at 21:21, <andrey.k...@linux.dev> wrote:

> From: Andrey Konovalov <andre...@gmail.com>
>
> kmalloc_oob_memset_*() tests do writes past the allocated objects.
> As the result, they corrupt memory, which might lead to crashes with the
> HW_TAGS mode, as it neither uses quarantine nor redzones.
>
> Adjust the tests to only write memory within the aligned kmalloc objects.
>
> Signed-off-by: Andrey Konovalov <andre...@gmail.com>
> ---
> lib/test_kasan.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index c82a82eb5393..fd00cd35e82c 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -431,61 +431,61 @@ static void kmalloc_uaf_16(struct kunit *test)
> static void kmalloc_oob_memset_2(struct kunit *test)
> {
> char *ptr;
> - size_t size = 8;
> + size_t size = 128 - KASAN_GRANULE_SIZE;
>
> ptr = kmalloc(size, GFP_KERNEL);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> - KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 7 + OOB_TAG_OFF, 0, 2));
> + KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size, 0, 2));

I think one important aspect of these tests in generic mode is that
the written range touches both valid and invalid memory. I think that
was meant to test any explicit instrumentation isn't just looking at
the starting address, but at the whole range.

It seems that with these changes that is no longer tested. Could we
somehow make it still test that?

> --
> 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+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/e9e2f7180f96e2496f0249ac81887376c6171e8f.1628709663.git.andreyknvl%40gmail.com.

[Marco Elver]

On Wed, 11 Aug 2021 at 21:21, <andrey.k...@linux.dev> wrote:

> From: Andrey Konovalov <andre...@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 <andre...@gmail.com>

Reviewed-by: Marco Elver <el...@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.

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/c3cd2a383e757e27dd9131635fc7d09a48a49cf9.1628709663.git.andreyknvl%40gmail.com.

[Marco Elver]

On Wed, 11 Aug 2021 at 21:21, <andrey.k...@linux.dev> wrote:

> From: Andrey Konovalov <andre...@gmail.com>
>
> Rework kmalloc_oob_right() to do these bad access checks:
>
> 1. An unaligned access one byte past the requested kmalloc size
> (can only be detected by KASAN_GENERIC).
> 2. An aligned access into the first out-of-bounds granule that falls
> within the aligned kmalloc object.
> 3. Out-of-bounds access past the aligned kmalloc object.
>
> Test #3 deliberately uses a read access to avoid corrupting memory.
> Otherwise, this test might lead to crashes with the HW_TAGS mode, as it
> neither uses quarantine nor redzones.
>
> Signed-off-by: Andrey Konovalov <andre...@gmail.com>

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

[Marco Elver]

On Wed, 11 Aug 2021 at 21:21, <andrey.k...@linux.dev> wrote:

> From: Andrey Konovalov <andre...@gmail.com>
>
> The HW_TAGS mode doesn't check memmove for negative size. As a result,
> the kmalloc_memmove_invalid_size test corrupts memory, which can result
> in a crash.
>
> Disable this test with HW_TAGS KASAN.
>
> Signed-off-by: Andrey Konovalov <andre...@gmail.com>

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

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/408c63e4a0353633a13403aab4ff25a505e03d93.1628709663.git.andreyknvl%40gmail.com.

[Marco Elver]

On Wed, 11 Aug 2021 at 21:21, <andrey.k...@linux.dev> wrote:
>

> From: Andrey Konovalov <andre...@gmail.com>
>
> KASAN tests do out-of-bounds and use-after-free accesses. Running the
> tests works fine for the GENERIC mode, as it uses qurantine and redzones.
> But the HW_TAGS mode uses neither, and running the tests might crash
> the kernel.
>
> Rework the tests to avoid corrupting kernel memory.

Thanks for this!

I think only 1 change is questionable ("kasan: test: avoid corrupting
memory via memset") because it no longer checks overlapping valid to
invalid range writes.

[Andrey Konovalov]

Good point!

> It seems that with these changes that is no longer tested. Could we
> somehow make it still test that?

Yes, will do in v2.

Thanks, Marco!

[Andrey Konovalov]

On Thu, Aug 12, 2021 at 10:57 AM Marco Elver <el...@google.com> wrote:
>

> On Wed, 11 Aug 2021 at 21:21, <andrey.k...@linux.dev> wrote:
> > From: Andrey Konovalov <andre...@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 <andre...@gmail.com>
>
> Reviewed-by: Marco Elver <el...@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!

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

Rework kmalloc_oob_right() to do these bad access checks:

1. An unaligned access one byte past the requested kmalloc size
(can only be detected by KASAN_GENERIC).
2. An aligned access into the first out-of-bounds granule that falls
within the aligned kmalloc object.
3. Out-of-bounds access past the aligned kmalloc object.

Test #3 deliberately uses a read access to avoid corrupting memory.
Otherwise, this test might lead to crashes with the HW_TAGS mode, as it

neither uses quarantine nor redzones.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>

---
lib/test_kasan.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 8f7b0b2f6e11..1bc3cdd2957f 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c

@@ -122,12 +122,28 @@ static void kasan_test_exit(struct kunit *test)

static void kmalloc_oob_right(struct kunit *test)
{
char *ptr;
- size_t size = 123;
+ size_t size = 128 - KASAN_GRANULE_SIZE - 5;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

KASAN tests do out-of-bounds and use-after-free accesses. Running the
tests works fine for the GENERIC mode, as it uses qurantine and redzones.
But the HW_TAGS mode uses neither, and running the tests might crash
the kernel.

Rework the tests to avoid corrupting kernel memory.

Changes v1->v2:
- Touch both good and bad memory in memset tests as suggested by Marco.

Andrey Konovalov (8):
kasan: test: rework kmalloc_oob_right
kasan: test: avoid writing invalid memory
kasan: test: avoid corrupting memory via memset
kasan: test: disable kmalloc_memmove_invalid_size for HW_TAGS
kasan: test: only do kmalloc_uaf_memset for generic mode
kasan: test: clean up ksize_uaf
kasan: test: avoid corrupting memory in copy_user_test
kasan: test: avoid corrupting memory in kasan_rcu_uaf

lib/test_kasan.c | 80 +++++++++++++++++++++++++++++------------
lib/test_kasan_module.c | 20 +++++------
2 files changed, 66 insertions(+), 34 deletions(-)

--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

The HW_TAGS mode doesn't check memmove for negative size. As a result,
the kmalloc_memmove_invalid_size test corrupts memory, which can result
in a crash.

Disable this test with HW_TAGS KASAN.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index db73bc9e3fa2..1f533a7346d9 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -501,11 +501,17 @@ static void kmalloc_memmove_invalid_size(struct kunit *test)

size_t size = 64;
volatile size_t invalid_size = -2;

+ /*
+ * Hardware tag-based mode doesn't check memmove for negative size.
+ * As a result, this test introduces a side-effect memory corruption,
+ * which can result in a crash.
+ */
+ KASAN_TEST_NEEDS_CONFIG_OFF(test, CONFIG_KASAN_HW_TAGS);
+

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

kmalloc_oob_memset_*() tests do writes past the allocated objects.

As the result, they corrupt memory, which might lead to crashes with the

HW_TAGS mode, as it neither uses quarantine nor redzones.

Adjust the tests to only write memory within the aligned kmalloc objects.

Also add a comment mentioning that memset tests are designed to touch

both valid and invalid memory.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index c82a82eb5393..db73bc9e3fa2 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -428,64 +428,70 @@ static void kmalloc_uaf_16(struct kunit *test)
kfree(ptr1);
}

+/*
+ * Note: in the memset tests below, the written range touches both valid and
+ * invalid memory. This makes sure that the instrumentation does not only check
+ * the starting address but the whole range.
+ */
+
static void kmalloc_oob_memset_2(struct kunit *test)
{
char *ptr;
- size_t size = 8;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 7 + OOB_TAG_OFF, 0, 2));

+ KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size - 1, 0, 2));
kfree(ptr);
}

static void kmalloc_oob_memset_4(struct kunit *test)
{
char *ptr;
- size_t size = 8;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 5 + OOB_TAG_OFF, 0, 4));

+ KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size - 3, 0, 4));
kfree(ptr);
}

-
static void kmalloc_oob_memset_8(struct kunit *test)
{
char *ptr;
- size_t size = 8;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 1 + OOB_TAG_OFF, 0, 8));

+ KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size - 7, 0, 8));
kfree(ptr);
}

static void kmalloc_oob_memset_16(struct kunit *test)
{
char *ptr;
- size_t size = 16;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

- KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + 1 + OOB_TAG_OFF, 0, 16));

+ KUNIT_EXPECT_KASAN_FAIL(test, memset(ptr + size - 15, 0, 16));
kfree(ptr);
}

static void kmalloc_oob_in_memset(struct kunit *test)
{
char *ptr;
- size_t size = 666;
+ size_t size = 128 - KASAN_GRANULE_SIZE;

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@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 <andre...@gmail.com>
---

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);

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

kmalloc_uaf_memset() writes to freed memory, which is only safe with the
GENERIC mode (as it uses quarantine). For other modes, this test corrupts
kernel memory, which might result in a crash.

Only enable kmalloc_uaf_memset() for the GENERIC mode.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 1f533a7346d9..1dcba6dbfc97 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -534,6 +534,12 @@ static void kmalloc_uaf_memset(struct kunit *test)

char *ptr;
size_t size = 33;

+ /*
+ * Only generic KASAN uses quarantine, which is required to avoid a
+ * kernel memory corruption this test causes.
+ */
+ KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
+

ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);

--
2.25.1

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

Some KASAN tests use global variables to store function returns values
so that the compiler doesn't optimize away these functions.

ksize_uaf() doesn't call any functions, so it doesn't need to use
kasan_int_result. Use volatile accesses instead, to be consistent with
other similar tests.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 1dcba6dbfc97..30f2cde96e81 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -737,8 +737,8 @@ static void ksize_uaf(struct kunit *test)

kfree(ptr);

KUNIT_EXPECT_KASAN_FAIL(test, ksize(ptr));
- KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = *ptr);
- KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = *(ptr + size));

+ KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]);

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

copy_user_test() does writes past the allocated object. As the result,

it corrupts kernel memory, which might lead to crashes with the HW_TAGS

mode, as it neither uses quarantine nor redzones.

(Technically, this test can't yet be enabled with the HW_TAGS mode, but
this will be implemented in the future.)

Adjust the test to only write memory within the aligned kmalloc object.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan_module.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/lib/test_kasan_module.c b/lib/test_kasan_module.c
index f1017f345d6c..fa73b9df0be4 100644
--- a/lib/test_kasan_module.c
+++ b/lib/test_kasan_module.c
@@ -15,13 +15,11 @@

#include "../mm/kasan/kasan.h"

-#define OOB_TAG_OFF (IS_ENABLED(CONFIG_KASAN_GENERIC) ? 0 : KASAN_GRANULE_SIZE)
-
static noinline void __init copy_user_test(void)
{
char *kmem;
char __user *usermem;
- size_t size = 10;

+ size_t size = 128 - KASAN_GRANULE_SIZE;

[andrey.k...@linux.dev]

From: Andrey Konovalov <andre...@gmail.com>

kasan_rcu_uaf() writes to freed memory via kasan_rcu_reclaim(), which is

only safe with the GENERIC mode (as it uses quarantine). For other modes,

this test corrupts kernel memory, which might result in a crash.

Turn the write into a read.

Signed-off-by: Andrey Konovalov <andre...@gmail.com>
---

lib/test_kasan_module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_kasan_module.c b/lib/test_kasan_module.c
index fa73b9df0be4..7ebf433edef3 100644
--- a/lib/test_kasan_module.c
+++ b/lib/test_kasan_module.c

[Andrey Konovalov]

On Thu, Aug 12, 2021 at 4:53 PM <andrey.k...@linux.dev> wrote:
>
> From: Andrey Konovalov <andre...@gmail.com>
>
> KASAN tests do out-of-bounds and use-after-free accesses. Running the
> tests works fine for the GENERIC mode, as it uses qurantine and redzones.
> But the HW_TAGS mode uses neither, and running the tests might crash
> the kernel.
>
> Rework the tests to avoid corrupting kernel memory.
>
> Changes v1->v2:
> - Touch both good and bad memory in memset tests as suggested by Marco.

Ah, I forgot to include your reviews/acks, Marco.

Perhaps you can give one for the whole series now.

Thanks!

[Marco Elver]

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

Looks good, thank you!