| Message ID | 20241014025701.3096253-3-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kasan: migrate the last module test to kunit | expand |
On Mon, 14 Oct 2024 07:57:00 +0500 Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > Migrate the copy_user_test to the KUnit framework to verify out-of-bound > detection via KASAN reports in copy_from_user(), copy_to_user() and > their static functions. > > This is the last migrated test in kasan_test_module.c, therefore delete > the file. > x86_64 allmodconfig produces: vmlinux.o: warning: objtool: strncpy_from_user+0x8a: call to __check_object_size() with UACCESS enabled
On Tue, Oct 15, 2024 at 1:10 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 14 Oct 2024 07:57:00 +0500 Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > > Migrate the copy_user_test to the KUnit framework to verify out-of-bound > > detection via KASAN reports in copy_from_user(), copy_to_user() and > > their static functions. > > > > This is the last migrated test in kasan_test_module.c, therefore delete > > the file. > > > > x86_64 allmodconfig produces: > > vmlinux.o: warning: objtool: strncpy_from_user+0x8a: call to __check_object_size() with UACCESS enabled Too bad. I guess we have to duplicate both kasan_check_write and check_object_size before both do_strncpy_from_user calls in strncpy_from_user.
On Tue, Oct 15, 2024 at 6:18 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Tue, Oct 15, 2024 at 1:10 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Mon, 14 Oct 2024 07:57:00 +0500 Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > > > > Migrate the copy_user_test to the KUnit framework to verify out-of-bound > > > detection via KASAN reports in copy_from_user(), copy_to_user() and > > > their static functions. > > > > > > This is the last migrated test in kasan_test_module.c, therefore delete > > > the file. > > > > > > > x86_64 allmodconfig produces: > > > > vmlinux.o: warning: objtool: strncpy_from_user+0x8a: call to __check_object_size() with UACCESS enabled I've missed this warning during x86_64 build, sorry. > > Too bad. I guess we have to duplicate both kasan_check_write and > check_object_size before both do_strncpy_from_user calls in > strncpy_from_user. Shall we do it once in strncpy_from_user() as I did in v1? Please let me know as I've tested in x86_64 and arm64 - there is no warning during kernel build with the diff below. These checks are for kernel pointer *dst only and size: kasan_check_write(dst, count); check_object_size(dst, count, false); And there are 2 calls of do_strncpy_from_user, which are implemented in x86 atm per commit 2865baf54077, and they are relevant to __user *src address, AFAIU. long strncpy_from_user() if (can_do_masked_user_access()) { src = masked_user_access_begin(src); retval = do_strncpy_from_user(dst, src, count, count); user_read_access_end(); } if (likely(src_addr < max_addr)) { if (user_read_access_begin(src, max)) { retval = do_strncpy_from_user(dst, src, count, max); user_read_access_end(); --- diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c index 989a12a6787..6dc234913dd 100644 --- a/lib/strncpy_from_user.c +++ b/lib/strncpy_from_user.c @@ -120,6 +120,9 @@ long strncpy_from_user(char *dst, const char __user *src, long count) if (unlikely(count <= 0)) return 0; + kasan_check_write(dst, count); + check_object_size(dst, count, false); + if (can_do_masked_user_access()) { long retval; @@ -142,8 +145,6 @@ long strncpy_from_user(char *dst, const char __user *src, long count) if (max > count) max = count; - kasan_check_write(dst, count); - check_object_size(dst, count, false); if (user_read_access_begin(src, max)) { retval = do_strncpy_from_user(dst, src, count, max); user_read_access_end();
On Tue, Oct 15, 2024 at 12:52 PM Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > > Too bad. I guess we have to duplicate both kasan_check_write and > > check_object_size before both do_strncpy_from_user calls in > > strncpy_from_user. > > Shall we do it once in strncpy_from_user() as I did in v1? > Please let me know as I've tested in x86_64 and arm64 - > there is no warning during kernel build with the diff below. > > These checks are for kernel pointer *dst only and size: > kasan_check_write(dst, count); > check_object_size(dst, count, false); > > And there are 2 calls of do_strncpy_from_user, > which are implemented in x86 atm per commit 2865baf54077, > and they are relevant to __user *src address, AFAIU. > > long strncpy_from_user() > if (can_do_masked_user_access()) { > src = masked_user_access_begin(src); > retval = do_strncpy_from_user(dst, src, count, count); > user_read_access_end(); > } > > if (likely(src_addr < max_addr)) { > if (user_read_access_begin(src, max)) { > retval = do_strncpy_from_user(dst, src, count, max); > user_read_access_end(); > > --- > diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c > index 989a12a6787..6dc234913dd 100644 > --- a/lib/strncpy_from_user.c > +++ b/lib/strncpy_from_user.c > @@ -120,6 +120,9 @@ long strncpy_from_user(char *dst, const char > __user *src, long count) > if (unlikely(count <= 0)) > return 0; > > + kasan_check_write(dst, count); > + check_object_size(dst, count, false); > + > if (can_do_masked_user_access()) { > long retval; > > @@ -142,8 +145,6 @@ long strncpy_from_user(char *dst, const char > __user *src, long count) > if (max > count) > max = count; > > - kasan_check_write(dst, count); > - check_object_size(dst, count, false); > if (user_read_access_begin(src, max)) { > retval = do_strncpy_from_user(dst, src, count, max); > user_read_access_end(); Ok, let's do this. (What looked concerning to me with this approach was doing the KASAN/userscopy checks outside of the src_addr < max_addr, but I suppose that should be fine.) Thank you!
diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile index b88543e5c0c..1a958e7c8a4 100644 --- a/mm/kasan/Makefile +++ b/mm/kasan/Makefile @@ -46,7 +46,6 @@ endif CFLAGS_kasan_test_c.o := $(CFLAGS_KASAN_TEST) RUSTFLAGS_kasan_test_rust.o := $(RUSTFLAGS_KASAN) -CFLAGS_kasan_test_module.o := $(CFLAGS_KASAN_TEST) obj-y := common.o report.o obj-$(CONFIG_KASAN_GENERIC) += init.o generic.o report_generic.o shadow.o quarantine.o @@ -59,4 +58,3 @@ ifdef CONFIG_RUST endif obj-$(CONFIG_KASAN_KUNIT_TEST) += kasan_test.o -obj-$(CONFIG_KASAN_MODULE_TEST) += kasan_test_module.o diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c index a181e4780d9..382bc64e42d 100644 --- a/mm/kasan/kasan_test_c.c +++ b/mm/kasan/kasan_test_c.c @@ -1954,6 +1954,52 @@ static void rust_uaf(struct kunit *test) KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf()); } +static void copy_user_test_oob(struct kunit *test) +{ + char *kmem; + char __user *usermem; + unsigned long useraddr; + size_t size = 128 - KASAN_GRANULE_SIZE; + int __maybe_unused unused; + + kmem = kunit_kmalloc(test, size, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kmem); + + useraddr = kunit_vm_mmap(test, NULL, 0, PAGE_SIZE, + PROT_READ | PROT_WRITE | PROT_EXEC, + MAP_ANONYMOUS | MAP_PRIVATE, 0); + KUNIT_ASSERT_NE_MSG(test, useraddr, 0, + "Could not create userspace mm"); + KUNIT_ASSERT_LT_MSG(test, useraddr, (unsigned long)TASK_SIZE, + "Failed to allocate user memory"); + + OPTIMIZER_HIDE_VAR(size); + usermem = (char __user *)useraddr; + + KUNIT_EXPECT_KASAN_FAIL(test, + unused = copy_from_user(kmem, usermem, size + 1)); + KUNIT_EXPECT_KASAN_FAIL(test, + unused = copy_to_user(usermem, kmem, size + 1)); + KUNIT_EXPECT_KASAN_FAIL(test, + unused = __copy_from_user(kmem, usermem, size + 1)); + KUNIT_EXPECT_KASAN_FAIL(test, + unused = __copy_to_user(usermem, kmem, size + 1)); + KUNIT_EXPECT_KASAN_FAIL(test, + unused = __copy_from_user_inatomic(kmem, usermem, size + 1)); + KUNIT_EXPECT_KASAN_FAIL(test, + unused = __copy_to_user_inatomic(usermem, kmem, size + 1)); + + /* + * Prepare a long string in usermem to avoid the strncpy_from_user test + * bailing out on '\0' before it reaches out-of-bounds. + */ + memset(kmem, 'a', size); + KUNIT_EXPECT_EQ(test, copy_to_user(usermem, kmem, size), 0); + + KUNIT_EXPECT_KASAN_FAIL(test, + unused = strncpy_from_user(kmem, usermem, size + 1)); +} + static struct kunit_case kasan_kunit_test_cases[] = { KUNIT_CASE(kmalloc_oob_right), KUNIT_CASE(kmalloc_oob_left), @@ -2028,6 +2074,7 @@ static struct kunit_case kasan_kunit_test_cases[] = { KUNIT_CASE(match_all_ptr_tag), KUNIT_CASE(match_all_mem_tag), KUNIT_CASE(rust_uaf), + KUNIT_CASE(copy_user_test_oob), {} }; diff --git a/mm/kasan/kasan_test_module.c b/mm/kasan/kasan_test_module.c deleted file mode 100644 index 27ec22767e4..00000000000 --- a/mm/kasan/kasan_test_module.c +++ /dev/null @@ -1,81 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * - * Copyright (c) 2014 Samsung Electronics Co., Ltd. - * Author: Andrey Ryabinin <a.ryabinin@samsung.com> - */ - -#define pr_fmt(fmt) "kasan: test: " fmt - -#include <linux/mman.h> -#include <linux/module.h> -#include <linux/printk.h> -#include <linux/slab.h> -#include <linux/uaccess.h> - -#include "kasan.h" - -static noinline void __init copy_user_test(void) -{ - char *kmem; - char __user *usermem; - size_t size = 128 - KASAN_GRANULE_SIZE; - int __maybe_unused unused; - - kmem = kmalloc(size, GFP_KERNEL); - if (!kmem) - return; - - usermem = (char __user *)vm_mmap(NULL, 0, PAGE_SIZE, - PROT_READ | PROT_WRITE | PROT_EXEC, - MAP_ANONYMOUS | MAP_PRIVATE, 0); - if (IS_ERR(usermem)) { - pr_err("Failed to allocate user memory\n"); - kfree(kmem); - return; - } - - OPTIMIZER_HIDE_VAR(size); - - pr_info("out-of-bounds in copy_from_user()\n"); - unused = copy_from_user(kmem, usermem, size + 1); - - pr_info("out-of-bounds in copy_to_user()\n"); - unused = copy_to_user(usermem, kmem, size + 1); - - pr_info("out-of-bounds in __copy_from_user()\n"); - unused = __copy_from_user(kmem, usermem, size + 1); - - pr_info("out-of-bounds in __copy_to_user()\n"); - unused = __copy_to_user(usermem, kmem, size + 1); - - pr_info("out-of-bounds in __copy_from_user_inatomic()\n"); - unused = __copy_from_user_inatomic(kmem, usermem, size + 1); - - pr_info("out-of-bounds in __copy_to_user_inatomic()\n"); - unused = __copy_to_user_inatomic(usermem, kmem, size + 1); - - pr_info("out-of-bounds in strncpy_from_user()\n"); - unused = strncpy_from_user(kmem, usermem, size + 1); - - vm_munmap((unsigned long)usermem, PAGE_SIZE); - kfree(kmem); -} - -static int __init kasan_test_module_init(void) -{ - /* - * Temporarily enable multi-shot mode. Otherwise, KASAN would only - * report the first detected bug and panic the kernel if panic_on_warn - * is enabled. - */ - bool multishot = kasan_save_enable_multi_shot(); - - copy_user_test(); - - kasan_restore_multi_shot(multishot); - return -EAGAIN; -} - -module_init(kasan_test_module_init); -MODULE_LICENSE("GPL");