mm/kasan/kasan_test_c.c | 37 +++++++++++++++++++++++++++++++++++++ mm/kmsan/kmsan_test.c | 17 +++++++++++++++++ mm/maccess.c | 10 ++++++++-- 3 files changed, 62 insertions(+), 2 deletions(-)
Instrument copy_from_kernel_nofault() with KMSAN for uninitialized kernel
memory check and copy_to_kernel_nofault() with KASAN, KCSAN to detect
the memory corruption.
syzbot reported that bpf_probe_read_kernel() kernel helper triggered
KASAN report via kasan_check_range() which is not the expected behaviour
as copy_from_kernel_nofault() is meant to be a non-faulting helper.
Solution is, suggested by Marco Elver, to replace KASAN, KCSAN check in
copy_from_kernel_nofault() with KMSAN detection of copying uninitilaized
kernel memory. In copy_to_kernel_nofault() we can retain
instrument_write() explicitly for the memory corruption instrumentation.
copy_to_kernel_nofault() is tested on x86_64 and arm64 with
CONFIG_KASAN_SW_TAGS. On arm64 with CONFIG_KASAN_HW_TAGS,
kunit test currently fails. Need more clarification on it.
Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
Reviewed-by: Marco Elver <elver@google.com>
Suggested-by: Marco Elver <elver@google.com>
Reported-by: syzbot+61123a5daeb9f7454599@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=61123a5daeb9f7454599
Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
v2:
- squashed previous submitted in -mm tree 2 patches based on Linus tree
v3:
- moved checks to *_nofault_loop macros per Marco's comments
- edited the commit message
v4:
- replaced Suggested-by with Reviewed-by
v5:
- addressed Andrey's comment on deleting CONFIG_KASAN_HW_TAGS check in
mm/kasan/kasan_test_c.c
- added explanatory comment in kasan_test_c.c
- added Suggested-by: Marco Elver back per Andrew's comment.
---
mm/kasan/kasan_test_c.c | 37 +++++++++++++++++++++++++++++++++++++
mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
mm/maccess.c | 10 ++++++++--
3 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index a181e4780d9d..cb6ad84641ec 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -1954,6 +1954,42 @@ static void rust_uaf(struct kunit *test)
KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf());
}
+static void copy_to_kernel_nofault_oob(struct kunit *test)
+{
+ char *ptr;
+ char buf[128];
+ size_t size = sizeof(buf);
+
+ /* This test currently fails with the HW_TAGS mode.
+ * The reason is unknown and needs to be investigated. */
+ ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+ OPTIMIZER_HIDE_VAR(ptr);
+
+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS)) {
+ /* Check that the returned pointer is tagged. */
+ KUNIT_EXPECT_GE(test, (u8)get_tag(ptr), (u8)KASAN_TAG_MIN);
+ KUNIT_EXPECT_LT(test, (u8)get_tag(ptr), (u8)KASAN_TAG_KERNEL);
+ }
+
+ /*
+ * We test copy_to_kernel_nofault() to detect corrupted memory that is
+ * being written into the kernel. In contrast, copy_from_kernel_nofault()
+ * is primarily used in kernel helper functions where the source address
+ * might be random or uninitialized. Applying KASAN instrumentation to
+ * copy_from_kernel_nofault() could lead to false positives.
+ * By focusing KASAN checks only on copy_to_kernel_nofault(),
+ * we ensure that only valid memory is written to the kernel,
+ * minimizing the risk of kernel corruption while avoiding
+ * false positives in the reverse case.
+ */
+ KUNIT_EXPECT_KASAN_FAIL(test,
+ copy_to_kernel_nofault(&buf[0], ptr, size));
+ KUNIT_EXPECT_KASAN_FAIL(test,
+ copy_to_kernel_nofault(ptr, &buf[0], size));
+ kfree(ptr);
+}
+
static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(kmalloc_oob_right),
KUNIT_CASE(kmalloc_oob_left),
@@ -2027,6 +2063,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(match_all_not_assigned),
KUNIT_CASE(match_all_ptr_tag),
KUNIT_CASE(match_all_mem_tag),
+ KUNIT_CASE(copy_to_kernel_nofault_oob),
KUNIT_CASE(rust_uaf),
{}
};
diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
index 13236d579eba..9733a22c46c1 100644
--- a/mm/kmsan/kmsan_test.c
+++ b/mm/kmsan/kmsan_test.c
@@ -640,6 +640,22 @@ static void test_unpoison_memory(struct kunit *test)
KUNIT_EXPECT_TRUE(test, report_matches(&expect));
}
+static void test_copy_from_kernel_nofault(struct kunit *test)
+{
+ long ret;
+ char buf[4], src[4];
+ size_t size = sizeof(buf);
+
+ EXPECTATION_UNINIT_VALUE_FN(expect, "copy_from_kernel_nofault");
+ kunit_info(
+ test,
+ "testing copy_from_kernel_nofault with uninitialized memory\n");
+
+ ret = copy_from_kernel_nofault((char *)&buf[0], (char *)&src[0], size);
+ USE(ret);
+ KUNIT_EXPECT_TRUE(test, report_matches(&expect));
+}
+
static struct kunit_case kmsan_test_cases[] = {
KUNIT_CASE(test_uninit_kmalloc),
KUNIT_CASE(test_init_kmalloc),
@@ -664,6 +680,7 @@ static struct kunit_case kmsan_test_cases[] = {
KUNIT_CASE(test_long_origin_chain),
KUNIT_CASE(test_stackdepot_roundtrip),
KUNIT_CASE(test_unpoison_memory),
+ KUNIT_CASE(test_copy_from_kernel_nofault),
{},
};
diff --git a/mm/maccess.c b/mm/maccess.c
index 518a25667323..3ca55ec63a6a 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -13,9 +13,14 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
return true;
}
+/*
+ * The below only uses kmsan_check_memory() to ensure uninitialized kernel
+ * memory isn't leaked.
+ */
#define copy_from_kernel_nofault_loop(dst, src, len, type, err_label) \
while (len >= sizeof(type)) { \
- __get_kernel_nofault(dst, src, type, err_label); \
+ __get_kernel_nofault(dst, src, type, err_label); \
+ kmsan_check_memory(src, sizeof(type)); \
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
@@ -49,7 +54,8 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
#define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \
while (len >= sizeof(type)) { \
- __put_kernel_nofault(dst, src, type, err_label); \
+ __put_kernel_nofault(dst, src, type, err_label); \
+ instrument_write(dst, sizeof(type)); \
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
--
2.34.1On Thu, Oct 10, 2024 at 3:10 PM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
>
> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index a181e4780d9d..cb6ad84641ec 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
> @@ -1954,6 +1954,42 @@ static void rust_uaf(struct kunit *test)
> KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf());
> }
>
> +static void copy_to_kernel_nofault_oob(struct kunit *test)
> +{
> + char *ptr;
> + char buf[128];
> + size_t size = sizeof(buf);
> +
> + /* This test currently fails with the HW_TAGS mode.
> + * The reason is unknown and needs to be investigated. */
> + ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> + OPTIMIZER_HIDE_VAR(ptr);
> +
> + if (IS_ENABLED(CONFIG_KASAN_SW_TAGS)) {
> + /* Check that the returned pointer is tagged. */
> + KUNIT_EXPECT_GE(test, (u8)get_tag(ptr), (u8)KASAN_TAG_MIN);
> + KUNIT_EXPECT_LT(test, (u8)get_tag(ptr), (u8)KASAN_TAG_KERNEL);
> + }
It appears you deleted a wrong check. I meant the checks above, not
the CONFIG_KASAN_HW_TAGS one.
> +
> + /*
> + * We test copy_to_kernel_nofault() to detect corrupted memory that is
> + * being written into the kernel. In contrast, copy_from_kernel_nofault()
> + * is primarily used in kernel helper functions where the source address
> + * might be random or uninitialized. Applying KASAN instrumentation to
> + * copy_from_kernel_nofault() could lead to false positives.
> + * By focusing KASAN checks only on copy_to_kernel_nofault(),
> + * we ensure that only valid memory is written to the kernel,
> + * minimizing the risk of kernel corruption while avoiding
> + * false positives in the reverse case.
> + */
> + KUNIT_EXPECT_KASAN_FAIL(test,
> + copy_to_kernel_nofault(&buf[0], ptr, size));
> + KUNIT_EXPECT_KASAN_FAIL(test,
> + copy_to_kernel_nofault(ptr, &buf[0], size));
Nit: empty line before kfree.
> + kfree(ptr);
> +}
Instrument copy_from_kernel_nofault() with KMSAN for uninitialized kernel
memory check and copy_to_kernel_nofault() with KASAN, KCSAN to detect
the memory corruption.
syzbot reported that bpf_probe_read_kernel() kernel helper triggered
KASAN report via kasan_check_range() which is not the expected behaviour
as copy_from_kernel_nofault() is meant to be a non-faulting helper.
Solution is, suggested by Marco Elver, to replace KASAN, KCSAN check in
copy_from_kernel_nofault() with KMSAN detection of copying uninitilaized
kernel memory. In copy_to_kernel_nofault() we can retain
instrument_write() explicitly for the memory corruption instrumentation.
copy_to_kernel_nofault() is tested on x86_64 and arm64 with
CONFIG_KASAN_SW_TAGS. On arm64 with CONFIG_KASAN_HW_TAGS,
kunit test currently fails. Need more clarification on it.
Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
Reviewed-by: Marco Elver <elver@google.com>
Reported-by: syzbot+61123a5daeb9f7454599@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=61123a5daeb9f7454599
Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
v2:
- squashed previous submitted in -mm tree 2 patches based on Linus tree
v3:
- moved checks to *_nofault_loop macros per Marco's comments
- edited the commit message
v4:
- replaced Suggested-by with Reviewed-by
v5:
- addressed Andrey's comment on deleting CONFIG_KASAN_HW_TAGS check in
mm/kasan/kasan_test_c.c
- added explanatory comment in kasan_test_c.c
- added Suggested-by: Marco Elver back per Andrew's comment.
v6:
- deleted checks KASAN_TAG_MIN, KASAN_TAG_KERNEL per Andrey's comment.
- added empty line before kfree.
---
mm/kasan/kasan_test_c.c | 34 ++++++++++++++++++++++++++++++++++
mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
mm/maccess.c | 10 ++++++++--
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index a181e4780d9d..716f2cac9708 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -1954,6 +1954,39 @@ static void rust_uaf(struct kunit *test)
KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf());
}
+static void copy_to_kernel_nofault_oob(struct kunit *test)
+{
+ char *ptr;
+ char buf[128];
+ size_t size = sizeof(buf);
+
+ /* This test currently fails with the HW_TAGS mode.
+ * The reason is unknown and needs to be investigated. */
+ KASAN_TEST_NEEDS_CONFIG_OFF(test, CONFIG_KASAN_HW_TAGS);
+
+ ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+ OPTIMIZER_HIDE_VAR(ptr);
+
+ /*
+ * We test copy_to_kernel_nofault() to detect corrupted memory that is
+ * being written into the kernel. In contrast, copy_from_kernel_nofault()
+ * is primarily used in kernel helper functions where the source address
+ * might be random or uninitialized. Applying KASAN instrumentation to
+ * copy_from_kernel_nofault() could lead to false positives.
+ * By focusing KASAN checks only on copy_to_kernel_nofault(),
+ * we ensure that only valid memory is written to the kernel,
+ * minimizing the risk of kernel corruption while avoiding
+ * false positives in the reverse case.
+ */
+ KUNIT_EXPECT_KASAN_FAIL(test,
+ copy_to_kernel_nofault(&buf[0], ptr, size));
+ KUNIT_EXPECT_KASAN_FAIL(test,
+ copy_to_kernel_nofault(ptr, &buf[0], size));
+
+ kfree(ptr);
+}
+
static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(kmalloc_oob_right),
KUNIT_CASE(kmalloc_oob_left),
@@ -2027,6 +2060,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(match_all_not_assigned),
KUNIT_CASE(match_all_ptr_tag),
KUNIT_CASE(match_all_mem_tag),
+ KUNIT_CASE(copy_to_kernel_nofault_oob),
KUNIT_CASE(rust_uaf),
{}
};
diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
index 13236d579eba..9733a22c46c1 100644
--- a/mm/kmsan/kmsan_test.c
+++ b/mm/kmsan/kmsan_test.c
@@ -640,6 +640,22 @@ static void test_unpoison_memory(struct kunit *test)
KUNIT_EXPECT_TRUE(test, report_matches(&expect));
}
+static void test_copy_from_kernel_nofault(struct kunit *test)
+{
+ long ret;
+ char buf[4], src[4];
+ size_t size = sizeof(buf);
+
+ EXPECTATION_UNINIT_VALUE_FN(expect, "copy_from_kernel_nofault");
+ kunit_info(
+ test,
+ "testing copy_from_kernel_nofault with uninitialized memory\n");
+
+ ret = copy_from_kernel_nofault((char *)&buf[0], (char *)&src[0], size);
+ USE(ret);
+ KUNIT_EXPECT_TRUE(test, report_matches(&expect));
+}
+
static struct kunit_case kmsan_test_cases[] = {
KUNIT_CASE(test_uninit_kmalloc),
KUNIT_CASE(test_init_kmalloc),
@@ -664,6 +680,7 @@ static struct kunit_case kmsan_test_cases[] = {
KUNIT_CASE(test_long_origin_chain),
KUNIT_CASE(test_stackdepot_roundtrip),
KUNIT_CASE(test_unpoison_memory),
+ KUNIT_CASE(test_copy_from_kernel_nofault),
{},
};
diff --git a/mm/maccess.c b/mm/maccess.c
index 518a25667323..3ca55ec63a6a 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -13,9 +13,14 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
return true;
}
+/*
+ * The below only uses kmsan_check_memory() to ensure uninitialized kernel
+ * memory isn't leaked.
+ */
#define copy_from_kernel_nofault_loop(dst, src, len, type, err_label) \
while (len >= sizeof(type)) { \
- __get_kernel_nofault(dst, src, type, err_label); \
+ __get_kernel_nofault(dst, src, type, err_label); \
+ kmsan_check_memory(src, sizeof(type)); \
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
@@ -49,7 +54,8 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
#define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \
while (len >= sizeof(type)) { \
- __put_kernel_nofault(dst, src, type, err_label); \
+ __put_kernel_nofault(dst, src, type, err_label); \
+ instrument_write(dst, sizeof(type)); \
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
--
2.34.1On Fri, Oct 11, 2024 at 5:52 AM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
>
> Instrument copy_from_kernel_nofault() with KMSAN for uninitialized kernel
> memory check and copy_to_kernel_nofault() with KASAN, KCSAN to detect
> the memory corruption.
>
> syzbot reported that bpf_probe_read_kernel() kernel helper triggered
> KASAN report via kasan_check_range() which is not the expected behaviour
> as copy_from_kernel_nofault() is meant to be a non-faulting helper.
>
> Solution is, suggested by Marco Elver, to replace KASAN, KCSAN check in
> copy_from_kernel_nofault() with KMSAN detection of copying uninitilaized
> kernel memory. In copy_to_kernel_nofault() we can retain
> instrument_write() explicitly for the memory corruption instrumentation.
For future reference: please write commit messages in a way that is
readable standalone. I.e. without obscured references to the
discussions or problems in the previous versions of the patch. It's
fine to give such references in itself, but you need to give enough
context in the commit message to make it understandable without
looking up those discussions.
> copy_to_kernel_nofault() is tested on x86_64 and arm64 with
> CONFIG_KASAN_SW_TAGS. On arm64 with CONFIG_KASAN_HW_TAGS,
> kunit test currently fails. Need more clarification on it.
>
> Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
> Reviewed-by: Marco Elver <elver@google.com>
> Reported-by: syzbot+61123a5daeb9f7454599@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=61123a5daeb9f7454599
> Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> ---
> v2:
> - squashed previous submitted in -mm tree 2 patches based on Linus tree
> v3:
> - moved checks to *_nofault_loop macros per Marco's comments
> - edited the commit message
> v4:
> - replaced Suggested-by with Reviewed-by
> v5:
> - addressed Andrey's comment on deleting CONFIG_KASAN_HW_TAGS check in
> mm/kasan/kasan_test_c.c
> - added explanatory comment in kasan_test_c.c
> - added Suggested-by: Marco Elver back per Andrew's comment.
> v6:
> - deleted checks KASAN_TAG_MIN, KASAN_TAG_KERNEL per Andrey's comment.
> - added empty line before kfree.
> ---
> mm/kasan/kasan_test_c.c | 34 ++++++++++++++++++++++++++++++++++
> mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
> mm/maccess.c | 10 ++++++++--
> 3 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index a181e4780d9d..716f2cac9708 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
> @@ -1954,6 +1954,39 @@ static void rust_uaf(struct kunit *test)
> KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf());
> }
>
> +static void copy_to_kernel_nofault_oob(struct kunit *test)
> +{
> + char *ptr;
> + char buf[128];
> + size_t size = sizeof(buf);
> +
> + /* This test currently fails with the HW_TAGS mode.
> + * The reason is unknown and needs to be investigated. */
> + KASAN_TEST_NEEDS_CONFIG_OFF(test, CONFIG_KASAN_HW_TAGS);
> +
> + ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> + OPTIMIZER_HIDE_VAR(ptr);
> +
> + /*
> + * We test copy_to_kernel_nofault() to detect corrupted memory that is
> + * being written into the kernel. In contrast, copy_from_kernel_nofault()
> + * is primarily used in kernel helper functions where the source address
> + * might be random or uninitialized. Applying KASAN instrumentation to
> + * copy_from_kernel_nofault() could lead to false positives.
> + * By focusing KASAN checks only on copy_to_kernel_nofault(),
> + * we ensure that only valid memory is written to the kernel,
> + * minimizing the risk of kernel corruption while avoiding
> + * false positives in the reverse case.
> + */
> + KUNIT_EXPECT_KASAN_FAIL(test,
> + copy_to_kernel_nofault(&buf[0], ptr, size));
> + KUNIT_EXPECT_KASAN_FAIL(test,
> + copy_to_kernel_nofault(ptr, &buf[0], size));
> +
> + kfree(ptr);
> +}
> +
> static struct kunit_case kasan_kunit_test_cases[] = {
> KUNIT_CASE(kmalloc_oob_right),
> KUNIT_CASE(kmalloc_oob_left),
> @@ -2027,6 +2060,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
> KUNIT_CASE(match_all_not_assigned),
> KUNIT_CASE(match_all_ptr_tag),
> KUNIT_CASE(match_all_mem_tag),
> + KUNIT_CASE(copy_to_kernel_nofault_oob),
> KUNIT_CASE(rust_uaf),
> {}
> };
> diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
> index 13236d579eba..9733a22c46c1 100644
> --- a/mm/kmsan/kmsan_test.c
> +++ b/mm/kmsan/kmsan_test.c
> @@ -640,6 +640,22 @@ static void test_unpoison_memory(struct kunit *test)
> KUNIT_EXPECT_TRUE(test, report_matches(&expect));
> }
>
> +static void test_copy_from_kernel_nofault(struct kunit *test)
> +{
> + long ret;
> + char buf[4], src[4];
> + size_t size = sizeof(buf);
> +
> + EXPECTATION_UNINIT_VALUE_FN(expect, "copy_from_kernel_nofault");
> + kunit_info(
> + test,
> + "testing copy_from_kernel_nofault with uninitialized memory\n");
> +
> + ret = copy_from_kernel_nofault((char *)&buf[0], (char *)&src[0], size);
> + USE(ret);
> + KUNIT_EXPECT_TRUE(test, report_matches(&expect));
> +}
> +
> static struct kunit_case kmsan_test_cases[] = {
> KUNIT_CASE(test_uninit_kmalloc),
> KUNIT_CASE(test_init_kmalloc),
> @@ -664,6 +680,7 @@ static struct kunit_case kmsan_test_cases[] = {
> KUNIT_CASE(test_long_origin_chain),
> KUNIT_CASE(test_stackdepot_roundtrip),
> KUNIT_CASE(test_unpoison_memory),
> + KUNIT_CASE(test_copy_from_kernel_nofault),
> {},
> };
>
> diff --git a/mm/maccess.c b/mm/maccess.c
> index 518a25667323..3ca55ec63a6a 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -13,9 +13,14 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
> return true;
> }
>
> +/*
> + * The below only uses kmsan_check_memory() to ensure uninitialized kernel
> + * memory isn't leaked.
> + */
> #define copy_from_kernel_nofault_loop(dst, src, len, type, err_label) \
> while (len >= sizeof(type)) { \
> - __get_kernel_nofault(dst, src, type, err_label); \
> + __get_kernel_nofault(dst, src, type, err_label); \
> + kmsan_check_memory(src, sizeof(type)); \
> dst += sizeof(type); \
> src += sizeof(type); \
> len -= sizeof(type); \
> @@ -49,7 +54,8 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
>
> #define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \
> while (len >= sizeof(type)) { \
> - __put_kernel_nofault(dst, src, type, err_label); \
> + __put_kernel_nofault(dst, src, type, err_label); \
> + instrument_write(dst, sizeof(type)); \
> dst += sizeof(type); \
> src += sizeof(type); \
> len -= sizeof(type); \
> --
> 2.34.1
>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
Tested-by: Andrey Konovalov <andreyknvl@gmail.com>
For KASAN parts.
Thank you!
On Sun, Oct 13, 2024 at 3:45 AM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> On Fri, Oct 11, 2024 at 5:52 AM Sabyrzhan Tasbolatov
> <snovitoll@gmail.com> wrote:
> >
> > Instrument copy_from_kernel_nofault() with KMSAN for uninitialized kernel
> > memory check and copy_to_kernel_nofault() with KASAN, KCSAN to detect
> > the memory corruption.
> >
> > syzbot reported that bpf_probe_read_kernel() kernel helper triggered
> > KASAN report via kasan_check_range() which is not the expected behaviour
> > as copy_from_kernel_nofault() is meant to be a non-faulting helper.
> >
> > Solution is, suggested by Marco Elver, to replace KASAN, KCSAN check in
> > copy_from_kernel_nofault() with KMSAN detection of copying uninitilaized
> > kernel memory. In copy_to_kernel_nofault() we can retain
> > instrument_write() explicitly for the memory corruption instrumentation.
>
> For future reference: please write commit messages in a way that is
> readable standalone. I.e. without obscured references to the
> discussions or problems in the previous versions of the patch. It's
> fine to give such references in itself, but you need to give enough
> context in the commit message to make it understandable without
> looking up those discussions.
>
> > copy_to_kernel_nofault() is tested on x86_64 and arm64 with
> > CONFIG_KASAN_SW_TAGS. On arm64 with CONFIG_KASAN_HW_TAGS,
> > kunit test currently fails. Need more clarification on it.
> >
> > Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
> > Reviewed-by: Marco Elver <elver@google.com>
> > Reported-by: syzbot+61123a5daeb9f7454599@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=61123a5daeb9f7454599
> > Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
> > Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> > ---
> > v2:
> > - squashed previous submitted in -mm tree 2 patches based on Linus tree
> > v3:
> > - moved checks to *_nofault_loop macros per Marco's comments
> > - edited the commit message
> > v4:
> > - replaced Suggested-by with Reviewed-by
> > v5:
> > - addressed Andrey's comment on deleting CONFIG_KASAN_HW_TAGS check in
> > mm/kasan/kasan_test_c.c
> > - added explanatory comment in kasan_test_c.c
> > - added Suggested-by: Marco Elver back per Andrew's comment.
> > v6:
> > - deleted checks KASAN_TAG_MIN, KASAN_TAG_KERNEL per Andrey's comment.
> > - added empty line before kfree.
> > ---
> > mm/kasan/kasan_test_c.c | 34 ++++++++++++++++++++++++++++++++++
> > mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
> > mm/maccess.c | 10 ++++++++--
> > 3 files changed, 59 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> > index a181e4780d9d..716f2cac9708 100644
> > --- a/mm/kasan/kasan_test_c.c
> > +++ b/mm/kasan/kasan_test_c.c
> > @@ -1954,6 +1954,39 @@ static void rust_uaf(struct kunit *test)
> > KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf());
> > }
> >
> > +static void copy_to_kernel_nofault_oob(struct kunit *test)
> > +{
> > + char *ptr;
> > + char buf[128];
> > + size_t size = sizeof(buf);
> > +
> > + /* This test currently fails with the HW_TAGS mode.
> > + * The reason is unknown and needs to be investigated. */
> > + KASAN_TEST_NEEDS_CONFIG_OFF(test, CONFIG_KASAN_HW_TAGS);
> > +
> > + ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> > + OPTIMIZER_HIDE_VAR(ptr);
> > +
> > + /*
> > + * We test copy_to_kernel_nofault() to detect corrupted memory that is
> > + * being written into the kernel. In contrast, copy_from_kernel_nofault()
> > + * is primarily used in kernel helper functions where the source address
> > + * might be random or uninitialized. Applying KASAN instrumentation to
> > + * copy_from_kernel_nofault() could lead to false positives.
> > + * By focusing KASAN checks only on copy_to_kernel_nofault(),
> > + * we ensure that only valid memory is written to the kernel,
> > + * minimizing the risk of kernel corruption while avoiding
> > + * false positives in the reverse case.
> > + */
> > + KUNIT_EXPECT_KASAN_FAIL(test,
> > + copy_to_kernel_nofault(&buf[0], ptr, size));
> > + KUNIT_EXPECT_KASAN_FAIL(test,
> > + copy_to_kernel_nofault(ptr, &buf[0], size));
> > +
> > + kfree(ptr);
> > +}
> > +
> > static struct kunit_case kasan_kunit_test_cases[] = {
> > KUNIT_CASE(kmalloc_oob_right),
> > KUNIT_CASE(kmalloc_oob_left),
> > @@ -2027,6 +2060,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
> > KUNIT_CASE(match_all_not_assigned),
> > KUNIT_CASE(match_all_ptr_tag),
> > KUNIT_CASE(match_all_mem_tag),
> > + KUNIT_CASE(copy_to_kernel_nofault_oob),
> > KUNIT_CASE(rust_uaf),
> > {}
> > };
> > diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
> > index 13236d579eba..9733a22c46c1 100644
> > --- a/mm/kmsan/kmsan_test.c
> > +++ b/mm/kmsan/kmsan_test.c
> > @@ -640,6 +640,22 @@ static void test_unpoison_memory(struct kunit *test)
> > KUNIT_EXPECT_TRUE(test, report_matches(&expect));
> > }
> >
> > +static void test_copy_from_kernel_nofault(struct kunit *test)
> > +{
> > + long ret;
> > + char buf[4], src[4];
> > + size_t size = sizeof(buf);
> > +
> > + EXPECTATION_UNINIT_VALUE_FN(expect, "copy_from_kernel_nofault");
> > + kunit_info(
> > + test,
> > + "testing copy_from_kernel_nofault with uninitialized memory\n");
> > +
> > + ret = copy_from_kernel_nofault((char *)&buf[0], (char *)&src[0], size);
> > + USE(ret);
> > + KUNIT_EXPECT_TRUE(test, report_matches(&expect));
> > +}
> > +
> > static struct kunit_case kmsan_test_cases[] = {
> > KUNIT_CASE(test_uninit_kmalloc),
> > KUNIT_CASE(test_init_kmalloc),
> > @@ -664,6 +680,7 @@ static struct kunit_case kmsan_test_cases[] = {
> > KUNIT_CASE(test_long_origin_chain),
> > KUNIT_CASE(test_stackdepot_roundtrip),
> > KUNIT_CASE(test_unpoison_memory),
> > + KUNIT_CASE(test_copy_from_kernel_nofault),
> > {},
> > };
> >
> > diff --git a/mm/maccess.c b/mm/maccess.c
> > index 518a25667323..3ca55ec63a6a 100644
> > --- a/mm/maccess.c
> > +++ b/mm/maccess.c
> > @@ -13,9 +13,14 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
> > return true;
> > }
> >
> > +/*
> > + * The below only uses kmsan_check_memory() to ensure uninitialized kernel
> > + * memory isn't leaked.
> > + */
> > #define copy_from_kernel_nofault_loop(dst, src, len, type, err_label) \
> > while (len >= sizeof(type)) { \
> > - __get_kernel_nofault(dst, src, type, err_label); \
> > + __get_kernel_nofault(dst, src, type, err_label); \
> > + kmsan_check_memory(src, sizeof(type)); \
> > dst += sizeof(type); \
> > src += sizeof(type); \
> > len -= sizeof(type); \
> > @@ -49,7 +54,8 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
> >
> > #define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \
> > while (len >= sizeof(type)) { \
> > - __put_kernel_nofault(dst, src, type, err_label); \
> > + __put_kernel_nofault(dst, src, type, err_label); \
> > + instrument_write(dst, sizeof(type)); \
> > dst += sizeof(type); \
> > src += sizeof(type); \
> > len -= sizeof(type); \
> > --
> > 2.34.1
> >
>
> Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
> Tested-by: Andrey Konovalov <andreyknvl@gmail.com>
>
> For KASAN parts.
Andrew,
Please let me know if the last v6 is ready for -mm tree.
Previous version was removed here:
https://lore.kernel.org/mm-commits/20241010214955.DBEB7C4CEC5@smtp.kernel.org/
Hopefully, they won't conflict in mm/kasan/kasan_test_c.c per another patch:
https://lore.kernel.org/linux-mm/20241014025701.3096253-3-snovitoll@gmail.com/
Thanks
>
> Thank you!
© 2016 - 2024 Red Hat, Inc.