| Message ID | 20241005092316.2471810-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, kmsan: instrument copy_from_kernel_nofault | expand |
On 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(-)
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))
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(-)