mm/kasan/kasan_test_c.c | 8 ++------ mm/kmsan/kmsan_test.c | 17 +++++++++++++++++ mm/maccess.c | 5 +++-- 3 files changed, 22 insertions(+), 8 deletions(-)
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() for the memory corruption instrumentation but before
pagefault_disable().
Added KMSAN and modified KASAN kunit tests and tested on x86_64.
This is the part of PATCH series attempting to properly address bugzilla
issue.
Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
Suggested-by: Marco Elver <elver@google.com>
Reported-by: syzbot+61123a5daeb9f7454599@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=61123a5daeb9f7454599
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
mm/kasan/kasan_test_c.c | 8 ++------
mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
mm/maccess.c | 5 +++--
3 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index 0a226ab032d..5cff90f831d 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -1954,7 +1954,7 @@ static void rust_uaf(struct kunit *test)
KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf());
}
-static void copy_from_to_kernel_nofault_oob(struct kunit *test)
+static void copy_to_kernel_nofault_oob(struct kunit *test)
{
char *ptr;
char buf[128];
@@ -1973,10 +1973,6 @@ static void copy_from_to_kernel_nofault_oob(struct kunit *test)
KUNIT_EXPECT_LT(test, (u8)get_tag(ptr), (u8)KASAN_TAG_KERNEL);
}
- KUNIT_EXPECT_KASAN_FAIL(test,
- copy_from_kernel_nofault(&buf[0], ptr, size));
- KUNIT_EXPECT_KASAN_FAIL(test,
- copy_from_kernel_nofault(ptr, &buf[0], size));
KUNIT_EXPECT_KASAN_FAIL(test,
copy_to_kernel_nofault(&buf[0], ptr, size));
KUNIT_EXPECT_KASAN_FAIL(test,
@@ -2057,7 +2053,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_from_to_kernel_nofault_oob),
+ 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 13236d579eb..9733a22c46c 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 f752f0c0fa3..a91a39a56cf 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -31,8 +31,9 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
if (!copy_from_kernel_nofault_allowed(src, size))
return -ERANGE;
+ /* Make sure uninitialized kernel memory isn't copied. */
+ kmsan_check_memory(src, size);
pagefault_disable();
- instrument_read(src, size);
if (!(align & 7))
copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
if (!(align & 3))
@@ -63,8 +64,8 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
align = (unsigned long)dst | (unsigned long)src;
- pagefault_disable();
instrument_write(dst, size);
+ pagefault_disable();
if (!(align & 7))
copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
if (!(align & 3))
--
2.34.1On Sat, 5 Oct 2024 at 11:22, Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote:
>
> 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() for the memory corruption instrumentation but before
> pagefault_disable().
>
> Added KMSAN and modified KASAN kunit tests and tested on x86_64.
>
> This is the part of PATCH series attempting to properly address bugzilla
> issue.
>
> Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
> Suggested-by: Marco Elver <elver@google.com>
> Reported-by: syzbot+61123a5daeb9f7454599@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=61123a5daeb9f7454599
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
I'm getting confused which parts are already picked up by Andrew into
-mm, and which aren't.
To clarify we have:
1. https://lore.kernel.org/mm-commits/20240927171751.D1BD9C4CEC4@smtp.kernel.org/
2. https://lore.kernel.org/mm-commits/20240930162435.9B6CBC4CED0@smtp.kernel.org/
And this is the 3rd patch, which applies on top of the other 2.
If my understanding is correct, rather than just adding fix on top of
fix, in the interest of having one clean patch which can also be
backported more easily, would it make sense to drop the first 2
patches from -mm, and you send out one clean patch series?
Thanks,
-- Marco
> ---
> mm/kasan/kasan_test_c.c | 8 ++------
> mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
> mm/maccess.c | 5 +++--
> 3 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index 0a226ab032d..5cff90f831d 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
> @@ -1954,7 +1954,7 @@ static void rust_uaf(struct kunit *test)
> KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf());
> }
>
> -static void copy_from_to_kernel_nofault_oob(struct kunit *test)
> +static void copy_to_kernel_nofault_oob(struct kunit *test)
> {
> char *ptr;
> char buf[128];
> @@ -1973,10 +1973,6 @@ static void copy_from_to_kernel_nofault_oob(struct kunit *test)
> KUNIT_EXPECT_LT(test, (u8)get_tag(ptr), (u8)KASAN_TAG_KERNEL);
> }
>
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - copy_from_kernel_nofault(&buf[0], ptr, size));
> - KUNIT_EXPECT_KASAN_FAIL(test,
> - copy_from_kernel_nofault(ptr, &buf[0], size));
> KUNIT_EXPECT_KASAN_FAIL(test,
> copy_to_kernel_nofault(&buf[0], ptr, size));
> KUNIT_EXPECT_KASAN_FAIL(test,
> @@ -2057,7 +2053,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_from_to_kernel_nofault_oob),
> + 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 13236d579eb..9733a22c46c 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 f752f0c0fa3..a91a39a56cf 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -31,8 +31,9 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
> if (!copy_from_kernel_nofault_allowed(src, size))
> return -ERANGE;
>
> + /* Make sure uninitialized kernel memory isn't copied. */
> + kmsan_check_memory(src, size);
> pagefault_disable();
> - instrument_read(src, size);
> if (!(align & 7))
> copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> if (!(align & 3))
> @@ -63,8 +64,8 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
> if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
> align = (unsigned long)dst | (unsigned long)src;
>
> - pagefault_disable();
> instrument_write(dst, size);
> + pagefault_disable();
> if (!(align & 7))
> copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
> if (!(align & 3))
> --
> 2.34.1
>
This patch consolidates the changes from the two previously submitted patches: 1. https://lore.kernel.org/mm-commits/20240927171751.D1BD9C4CEC4@smtp.kernel.org 2. https://lore.kernel.org/mm-commits/20240930162435.9B6CBC4CED0@smtp.kernel.org In the interest of clarity and easier backporting, this single patch includes all changes and replaces the two previous patches. Andrew, Please drop the two previous patches from the -mm tree in favor of this one. Apologies for the confusion. Will try to minimize it in future. The patch is based on the latest Linus tree, where I've squashed the latest 2 patches merged in -mm tree. Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com> Sabyrzhan Tasbolatov (1): mm, kasan, kmsan: copy_from/to_kernel_nofault mm/kasan/kasan_test_c.c | 27 +++++++++++++++++++++++++++ mm/kmsan/kmsan_test.c | 17 +++++++++++++++++ mm/maccess.c | 7 +++++-- 3 files changed, 49 insertions(+), 2 deletions(-) -- 2.34.1
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() for the memory corruption instrumentation but before
pagefault_disable().
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
- currently, disabled in kunit test.
Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.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
---
mm/kasan/kasan_test_c.c | 27 +++++++++++++++++++++++++++
mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
mm/maccess.c | 7 +++++--
3 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index a181e4780d9d..5cff90f831db 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -1954,6 +1954,32 @@ 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);
+
+ /* Not detecting fails currently with HW_TAGS */
+ 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);
+
+ 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);
+ }
+
+ 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 +2053,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..a91a39a56cfd 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -15,7 +15,7 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
#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); \
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
@@ -31,6 +31,8 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
if (!copy_from_kernel_nofault_allowed(src, size))
return -ERANGE;
+ /* Make sure uninitialized kernel memory isn't copied. */
+ kmsan_check_memory(src, size);
pagefault_disable();
if (!(align & 7))
copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
@@ -49,7 +51,7 @@ 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); \
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
@@ -62,6 +64,7 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
align = (unsigned long)dst | (unsigned long)src;
+ instrument_write(dst, size);
pagefault_disable();
if (!(align & 7))
copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
--
2.34.1On Sat, Oct 05, 2024 at 09:48PM +0500, Sabyrzhan Tasbolatov 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() for the memory corruption instrumentation but before
> pagefault_disable().
I don't understand why it has to be before the whole copy i.e. before
pagefault_disable()?
I think my suggestion was to only check the memory where no fault
occurred. See below.
> diff --git a/mm/maccess.c b/mm/maccess.c
> index 518a25667323..a91a39a56cfd 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -15,7 +15,7 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
>
> #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); \
> dst += sizeof(type); \
> src += sizeof(type); \
> len -= sizeof(type); \
> @@ -31,6 +31,8 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
> if (!copy_from_kernel_nofault_allowed(src, size))
> return -ERANGE;
>
> + /* Make sure uninitialized kernel memory isn't copied. */
> + kmsan_check_memory(src, size);
> pagefault_disable();
> if (!(align & 7))
> copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> @@ -49,7 +51,7 @@ 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); \
> dst += sizeof(type); \
> src += sizeof(type); \
> len -= sizeof(type); \
> @@ -62,6 +64,7 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
> if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
> align = (unsigned long)dst | (unsigned long)src;
>
> + instrument_write(dst, size);
> pagefault_disable();
So this will check the whole range before the access. But if the copy
aborts because of a fault, then we may still end up with false
positives.
Why not something like the below - normally we check the accesses
before, but these are debug kernels anyway, so I see no harm in making
an exception in this case and checking the memory if there was no fault
i.e. it didn't jump to err_label yet. It's also slower because of
repeated calls, but these helpers aren't frequently used.
The alternative is to do the sanitizer check after the entire copy if we
know there was no fault at all. But that may still hide real bugs if
e.g. it starts copying some partial memory and then accesses an
unfaulted page.
diff --git a/mm/maccess.c b/mm/maccess.c
index a91a39a56cfd..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); \
+ kmsan_check_memory(src, sizeof(type)); \
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
@@ -31,8 +36,6 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
if (!copy_from_kernel_nofault_allowed(src, size))
return -ERANGE;
- /* Make sure uninitialized kernel memory isn't copied. */
- kmsan_check_memory(src, size);
pagefault_disable();
if (!(align & 7))
copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
@@ -52,6 +55,7 @@ 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); \
+ instrument_write(dst, sizeof(type)); \
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
@@ -64,7 +68,6 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
align = (unsigned long)dst | (unsigned long)src;
- instrument_write(dst, size);
pagefault_disable();
if (!(align & 7))
copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
On Tue, Oct 8, 2024 at 1:32 PM Marco Elver <elver@google.com> wrote:
>
> On Sat, Oct 05, 2024 at 09:48PM +0500, Sabyrzhan Tasbolatov 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() for the memory corruption instrumentation but before
> > pagefault_disable().
>
> I don't understand why it has to be before the whole copy i.e. before
> pagefault_disable()?
>
I was unsure about this decision as well - I should've waited for your response
before sending the PATCH when I was asking for clarification. Sorry
for the confusion,
I thought that what you meant as the instrumentation was already done after
pagefault_disable().
Let me send the v3 with your suggested diff, I will also ask Andrew to drop
merged to -mm patch.
https://lore.kernel.org/all/20241008020150.4795AC4CEC6@smtp.kernel.org/
Thanks for the review.
> I think my suggestion was to only check the memory where no fault
> occurred. See below.
>
> > diff --git a/mm/maccess.c b/mm/maccess.c
> > index 518a25667323..a91a39a56cfd 100644
> > --- a/mm/maccess.c
> > +++ b/mm/maccess.c
> > @@ -15,7 +15,7 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
> >
> > #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); \
> > dst += sizeof(type); \
> > src += sizeof(type); \
> > len -= sizeof(type); \
> > @@ -31,6 +31,8 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
> > if (!copy_from_kernel_nofault_allowed(src, size))
> > return -ERANGE;
> >
> > + /* Make sure uninitialized kernel memory isn't copied. */
> > + kmsan_check_memory(src, size);
> > pagefault_disable();
> > if (!(align & 7))
> > copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> > @@ -49,7 +51,7 @@ 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); \
> > dst += sizeof(type); \
> > src += sizeof(type); \
> > len -= sizeof(type); \
> > @@ -62,6 +64,7 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
> > if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
> > align = (unsigned long)dst | (unsigned long)src;
> >
> > + instrument_write(dst, size);
> > pagefault_disable();
>
> So this will check the whole range before the access. But if the copy
> aborts because of a fault, then we may still end up with false
> positives.
>
> Why not something like the below - normally we check the accesses
> before, but these are debug kernels anyway, so I see no harm in making
> an exception in this case and checking the memory if there was no fault
> i.e. it didn't jump to err_label yet. It's also slower because of
> repeated calls, but these helpers aren't frequently used.
>
> The alternative is to do the sanitizer check after the entire copy if we
> know there was no fault at all. But that may still hide real bugs if
> e.g. it starts copying some partial memory and then accesses an
> unfaulted page.
>
>
> diff --git a/mm/maccess.c b/mm/maccess.c
> index a91a39a56cfd..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); \
> + kmsan_check_memory(src, sizeof(type)); \
> dst += sizeof(type); \
> src += sizeof(type); \
> len -= sizeof(type); \
> @@ -31,8 +36,6 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
> if (!copy_from_kernel_nofault_allowed(src, size))
> return -ERANGE;
>
> - /* Make sure uninitialized kernel memory isn't copied. */
> - kmsan_check_memory(src, size);
> pagefault_disable();
> if (!(align & 7))
> copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> @@ -52,6 +55,7 @@ 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); \
> + instrument_write(dst, sizeof(type)); \
> dst += sizeof(type); \
> src += sizeof(type); \
> len -= sizeof(type); \
> @@ -64,7 +68,6 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
> if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
> align = (unsigned long)dst | (unsigned long)src;
>
> - instrument_write(dst, size);
> pagefault_disable();
> if (!(align & 7))
> copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
On Tue, Oct 08, 2024 at 01:46PM +0500, Sabyrzhan Tasbolatov wrote:
> On Tue, Oct 8, 2024 at 1:32 PM Marco Elver <elver@google.com> wrote:
> >
> > On Sat, Oct 05, 2024 at 09:48PM +0500, Sabyrzhan Tasbolatov 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() for the memory corruption instrumentation but before
> > > pagefault_disable().
> >
> > I don't understand why it has to be before the whole copy i.e. before
> > pagefault_disable()?
> >
>
> I was unsure about this decision as well - I should've waited for your response
> before sending the PATCH when I was asking for clarification. Sorry
> for the confusion,
> I thought that what you meant as the instrumentation was already done after
> pagefault_disable().
I just did some digging and there is some existing instrumentation, but
not for what we want. The accesses in the loop on x86 do this:
copy_to_kernel_nofault:
#define __put_kernel_nofault(dst, src, type, err_label) \
__put_user_size(*((type *)(src)), (__force type __user *)(dst), \
sizeof(type), err_label)
and __put_user_size:
#define __put_user_size(x, ptr, size, label) \
do { \
__typeof__(*(ptr)) __x = (x); /* eval x once */ \
__typeof__(ptr) __ptr = (ptr); /* eval ptr once */ \
__chk_user_ptr(__ptr); \
switch (size) { \
case 1: \
__put_user_goto(__x, __ptr, "b", "iq", label); \
break; \
case 2: \
__put_user_goto(__x, __ptr, "w", "ir", label); \
break; \
case 4: \
__put_user_goto(__x, __ptr, "l", "ir", label); \
break; \
case 8: \
__put_user_goto_u64(__x, __ptr, label); \
break; \
default: \
__put_user_bad(); \
} \
instrument_put_user(__x, __ptr, size); \
} while (0)
which already has an instrument_put_user, which expands to this:
#define instrument_put_user(from, ptr, size) \
({ \
kmsan_copy_to_user(ptr, &from, sizeof(from), 0); \
})
So this is already instrumented for KMSAN, to check no uninitialized
memory is accessed - but that's only useful if copying to user space.
__put_kernel_nofault is "abusing" the same helper to copy to the kernel,
so adding explicit instrumentation as proposed still makes sense.
Thanks,
-- Marco
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
- currently, disabled in kunit test.
Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.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
---
mm/kasan/kasan_test_c.c | 27 +++++++++++++++++++++++++++
mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
mm/maccess.c | 10 ++++++++--
3 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index a181e4780d9d..5cff90f831db 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -1954,6 +1954,32 @@ 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);
+
+ /* Not detecting fails currently with HW_TAGS */
+ 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);
+
+ 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);
+ }
+
+ 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 +2053,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 Tue, 8 Oct 2024 at 12:14, 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.
>
> 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
> - currently, disabled in kunit test.
I assume you retested. Did you also test the bpf_probe_read_kernel()
false positive no longer appears?
> Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
> Suggested-by: Marco Elver <elver@google.com>
This looks more reasonable:
Reviewed-by: Marco Elver <elver@google.com>
This looks like the most conservative thing to do for now.
> 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
> ---
> mm/kasan/kasan_test_c.c | 27 +++++++++++++++++++++++++++
> mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
> mm/maccess.c | 10 ++++++++--
> 3 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index a181e4780d9d..5cff90f831db 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
> @@ -1954,6 +1954,32 @@ 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);
> +
> + /* Not detecting fails currently with HW_TAGS */
> + 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);
> +
> + 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);
> + }
> +
> + 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 +2053,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
>
On Tue, Oct 8, 2024 at 4:36 PM Marco Elver <elver@google.com> wrote:
>
> On Tue, 8 Oct 2024 at 12:14, 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.
> >
> > 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
> > - currently, disabled in kunit test.
>
> I assume you retested. Did you also test the bpf_probe_read_kernel()
> false positive no longer appears?
I've tested on:
- x86_64 with KMSAN
- x86_64 with KASAN
- arm64 with HW_TAGS -- still failing
- arm64 with SW_TAGS
Please see the testing result in the following link:
https://gist.github.com/novitoll/e2ccb2162340f7f8a63b63ee3e0f9994
I've also tested bpf_probe_read_kernel() in x86_64 KMSAN build,
it does trigger KMSAN, though I don't see explicitly copy_from_kernel*
in stack frame. AFAIU, it's checked prior to it in text_poke_copy().
Attached the PoC in the comment of the link above:
root@syzkaller:/tmp# uname -a
Linux syzkaller 6.12.0-rc2-g441b500abd70 #10 SMP PREEMPT_DYNAMIC Wed
Oct 9 00:17:59 +05 2024 x86_64 GNU/Linux
root@syzkaller:/tmp# ./exploit
[*] exploit start
[+] program loaded!
[ 139.778255] =====================================================
[ 139.778846] BUG: KMSAN: uninit-value in bcmp+0x155/0x290
[ 139.779311] bcmp+0x155/0x290
[ 139.779591] __text_poke+0xe2d/0x1120
[ 139.779950] text_poke_copy+0x1e7/0x2b0
[ 139.780297] bpf_arch_text_copy+0x41/0xa0
[ 139.780665] bpf_dispatcher_change_prog+0x12dd/0x16b0
[ 139.781324] bpf_prog_test_run_xdp+0xbf0/0x1d20
[ 139.781898] bpf_prog_test_run+0x5d6/0x9a0
[ 139.782372] __sys_bpf+0x758/0xf10
[ 139.782759] __x64_sys_bpf+0xdd/0x130
[ 139.783178] x64_sys_call+0x1a21/0x4e10
[ 139.783610] do_syscall_64+0xcd/0x1b0
[ 139.784039] entry_SYSCALL_64_after_hwframe+0x67/0x6f
[ 139.784597]
[ 139.784779] Uninit was created at:
[ 139.785197] __alloc_pages_noprof+0x717/0xe70
[ 139.785689] alloc_pages_bulk_noprof+0x17e1/0x20e0
[ 139.786223] alloc_pages_bulk_array_mempolicy_noprof+0x49e/0x5b0
[ 139.786873] __vmalloc_node_range_noprof+0xef2/0x24f0
[ 139.787414] execmem_alloc+0x1ec/0x4c0
[ 139.787841] bpf_jit_alloc_exec+0x3e/0x40
[ 139.788299] bpf_dispatcher_change_prog+0x430/0x16b0
[ 139.788837] bpf_prog_test_run_xdp+0xbf0/0x1d20
[ 139.789324] bpf_prog_test_run+0x5d6/0x9a0
[ 139.789774] __sys_bpf+0x758/0xf10
[ 139.790167] __x64_sys_bpf+0xdd/0x130
[ 139.790580] x64_sys_call+0x1a21/0x4e10
[ 139.791007] do_syscall_64+0xcd/0x1b0
[ 139.791423] entry_SYSCALL_64_after_hwframe+0x67/0x6f
>
> > Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
> > Suggested-by: Marco Elver <elver@google.com>
>
> This looks more reasonable:
>
> Reviewed-by: Marco Elver <elver@google.com>
>
> This looks like the most conservative thing to do for now.
Done.
>
> > 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
> > ---
> > mm/kasan/kasan_test_c.c | 27 +++++++++++++++++++++++++++
> > mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
> > mm/maccess.c | 10 ++++++++--
> > 3 files changed, 52 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> > index a181e4780d9d..5cff90f831db 100644
> > --- a/mm/kasan/kasan_test_c.c
> > +++ b/mm/kasan/kasan_test_c.c
> > @@ -1954,6 +1954,32 @@ 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);
> > +
> > + /* Not detecting fails currently with HW_TAGS */
> > + 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);
> > +
> > + 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);
> > + }
> > +
> > + 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 +2053,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
> >
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
- currently, disabled in kunit test.
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: Marco Elver
---
mm/kasan/kasan_test_c.c | 27 +++++++++++++++++++++++++++
mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
mm/maccess.c | 10 ++++++++--
3 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index a181e4780d9d..5cff90f831db 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -1954,6 +1954,32 @@ 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);
+
+ /* Not detecting fails currently with HW_TAGS */
+ 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);
+
+ 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);
+ }
+
+ 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 +2053,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 Tue, Oct 8, 2024 at 9:28 PM 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.
>
> 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
> - currently, disabled in kunit test.
>
> 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>
(Back from travels, looking at the patches again.)
> ---
> 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: Marco Elver
> ---
> mm/kasan/kasan_test_c.c | 27 +++++++++++++++++++++++++++
> mm/kmsan/kmsan_test.c | 17 +++++++++++++++++
> mm/maccess.c | 10 ++++++++--
> 3 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index a181e4780d9d..5cff90f831db 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
> @@ -1954,6 +1954,32 @@ 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);
> +
> + /* Not detecting fails currently with HW_TAGS */
Let's reword this to:
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);
> +
> + 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);
> + }
Let's drop the checks above: if pointers returned by kmalloc are not
tagged, the checks below (and many other tests) will fail.
> +
Please add a comment here explaining why we only check
copy_to_kernel_nofault and not copy_from_kernel_nofault (is this
because we cannot add KASAN instrumentation to
copy_from_kernel_nofault?).
> + 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 +2053,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
>
On Wed, 9 Oct 2024 at 22:19, Andrey Konovalov <andreyknvl@gmail.com> wrote: [...] > Please add a comment here explaining why we only check > copy_to_kernel_nofault and not copy_from_kernel_nofault (is this > because we cannot add KASAN instrumentation to > copy_from_kernel_nofault?). Just to clarify: Unless we can prove that there won't be any false positives, I proposed to err on the side of being conservative here. The new way of doing it after we already checked that the accessed location is on a faulted-in page may be amenable to also KASAN instrumentation, but you can also come up with cases that would be a false positive: e.g. some copy_from_kernel_nofault() for a large range, knowing that if it accesses bad memory at least one page is not faulted in, but some initial pages may be faulted in; in that case there'd be some error handling that then deals with the failure. Again, this might be something that an eBPF program could legally do. On the other hand, we may want to know if we are leaking random uninitialized kernel memory with KMSAN to avoid infoleaks. Only copy_to_kernel_nofault should really have valid memory, otherwise we risk corrupting the kernel. But these checks should only happen after we know we're accessing faulted-in memory, again to avoid false positives.
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!
On Tue, 8 Oct 2024 at 21:28, 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. > > 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 > - currently, disabled in kunit test. > > 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: Marco Elver For future reference: No need to send v+1 just for this tag. Usually maintainers pick up tags from the last round without the original author having to send out a v+1 with the tags. Of course, if you make other corrections and need to send a v+1, then it is appropriate to collect tags where those tags would remain valid (such as on unchanged patches part of the series, or for simpler corrections).
On Wed, Oct 9, 2024 at 12:34 AM Marco Elver <elver@google.com> wrote: > > On Tue, 8 Oct 2024 at 21:28, 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. > > > > 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 > > - currently, disabled in kunit test. > > > > 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: Marco Elver > > For future reference: No need to send v+1 just for this tag. Usually > maintainers pick up tags from the last round without the original > author having to send out a v+1 with the tags. Of course, if you make > other corrections and need to send a v+1, then it is appropriate to > collect tags where those tags would remain valid (such as on unchanged > patches part of the series, or for simpler corrections). Thanks! Will do it next time. Please advise if Andrew should need to be notified in the separate cover letter to remove the prev. merged to -mm tree patch and use this v4: https://lore.kernel.org/all/20241008020150.4795AC4CEC6@smtp.kernel.org/
On Wed, 9 Oct 2024 00:42:25 +0500 Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > > v4: > > > - replaced Suggested-By with Reviewed-By: Marco Elver > > > > For future reference: No need to send v+1 just for this tag. Usually > > maintainers pick up tags from the last round without the original > > author having to send out a v+1 with the tags. Of course, if you make > > other corrections and need to send a v+1, then it is appropriate to > > collect tags where those tags would remain valid (such as on unchanged > > patches part of the series, or for simpler corrections). > > Thanks! Will do it next time. > > Please advise if Andrew should need to be notified in the separate cover letter > to remove the prev. merged to -mm tree patch and use this v4: > https://lore.kernel.org/all/20241008020150.4795AC4CEC6@smtp.kernel.org/ I've updated v3's changelog, thanks. I kept Marco's Suggested-by:, as that's still relevant even with the Reviewed-by:.
© 2016 - 2024 Red Hat, Inc.