| Message ID | 20240921071005.909660-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] mm: x86: instrument __get/__put_kernel_nofault | expand |
On Sat, Sep 21, 2024 at 9:09 AM Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(), > strncpy_from_kernel_nofault() where __put_kernel_nofault, > __get_kernel_nofault macros are used. > > __get_kernel_nofault needs instrument_memcpy_before() which handles > KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault > macro, instrument_write() check should be enough as it's validated via > kmsan_copy_to_user() in instrument_put_user(). > > __get_user_size was appended with instrument_get_user() for KMSAN check in > commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and > put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT. > > copy_from_to_kernel_nofault_oob() kunit test triggers 4 KASAN OOB > bug reports as expected, one for each copy_from/to_kernel_nofault call. > > 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> I tried running the tests with this patch applied, but unfortunately the added test fails on arm64, most likely due to missing annotations in arm64 asm code. We need to either mark the added test as x86-only via KASAN_TEST_NEEDS_CONFIG_ON or add annotations for arm64. With annotations for arm64, the test might still fail for other architectures, but I think that's fine: hopefully relevant people will add annotations in time. But I consider both x86 and arm64 important, so we should keep the tests working there. If you decide to add annotations for arm64, please also test both KASAN_SW_TAGS and KASAN_HW_TAGS modes. Thanks!
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h index 3a7755c1a441..87fb59071e8c 100644 --- a/arch/x86/include/asm/uaccess.h +++ b/arch/x86/include/asm/uaccess.h @@ -353,6 +353,7 @@ do { \ default: \ (x) = __get_user_bad(); \ } \ + instrument_get_user(x); \ } while (0) #define __get_user_asm(x, addr, err, itype) \ @@ -620,6 +621,7 @@ do { \ #ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT #define __get_kernel_nofault(dst, src, type, err_label) \ + instrument_memcpy_before(dst, src, sizeof(type)); \ __get_user_size(*((type *)(dst)), (__force type __user *)(src), \ sizeof(type), err_label) #else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT @@ -627,6 +629,7 @@ do { \ do { \ int __kr_err; \ \ + instrument_memcpy_before(dst, src, sizeof(type)); \ __get_user_size(*((type *)(dst)), (__force type __user *)(src), \ sizeof(type), __kr_err); \ if (unlikely(__kr_err)) \ @@ -635,6 +638,7 @@ do { \ #endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT #define __put_kernel_nofault(dst, src, type, err_label) \ + instrument_write(dst, sizeof(type)); \ __put_user_size(*((type *)(src)), (__force type __user *)(dst), \ sizeof(type), err_label) diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c index 7b32be2a3cf0..d13f1a514750 100644 --- a/mm/kasan/kasan_test.c +++ b/mm/kasan/kasan_test.c @@ -1899,6 +1899,26 @@ static void match_all_mem_tag(struct kunit *test) kfree(ptr); } +static void copy_from_to_kernel_nofault_oob(struct kunit *test) +{ + char *ptr; + char buf[128]; + size_t size = sizeof(buf); + + ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); + + 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, + 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), @@ -1971,6 +1991,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), {} };
Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(), strncpy_from_kernel_nofault() where __put_kernel_nofault, __get_kernel_nofault macros are used. __get_kernel_nofault needs instrument_memcpy_before() which handles KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault macro, instrument_write() check should be enough as it's validated via kmsan_copy_to_user() in instrument_put_user(). __get_user_size was appended with instrument_get_user() for KMSAN check in commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT. copy_from_to_kernel_nofault_oob() kunit test triggers 4 KASAN OOB bug reports as expected, one for each copy_from/to_kernel_nofault call. 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> --- v3: changed kunit test from UAF to OOB case and git commit message. v4: updated a grammar in git commit message. --- arch/x86/include/asm/uaccess.h | 4 ++++ mm/kasan/kasan_test.c | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+)