Bug 210505
| Summary: | KASAN: handle copy_from/to_kernel_nofault | ||
|---|---|---|---|
| Product: | Memory Management | Reporter: | Andrey Konovalov (andreyknvl) |
| Component: | Sanitizers | Assignee: | MM/Sanitizers virtual assignee (mm_sanitizers) |
| Status: | RESOLVED CODE_FIX | ||
| Severity: | normal | CC: | dvyukov, kasan-dev, snovitoll |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | upstream | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
I've checked this kunit test on the latest 6.11.0-rc7 version,
and it does trigger KASAN on copy_to_kernel_nofault via
x86 macros call stack:
copy_to_kernel_nofault_loop
__put_kernel_nofault
__put_user_size
instrument_put_user
, though it does not trigger KASAN for copy_from_kernel_nofault,
which x86 macros call stack look:
copy_from_kernel_nofault_loop
__get_kernel_nofault
__get_user
__get_user_size
__get_user_size was appended with "instrument_get_user()" check in
commit 888f84a6("x86: asm: instrument usercopy in get_user() and put_user()")
but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
So I've tried to add "instrument_get_user" in __get_user_size
for non CONFIG_CC_HAS_ASM_GOTO_OUTPUT condition -- it didn't trigger
as well.
I think, __put_user_size and __get_user macros should not be
in __put_kernel_nofault and __get_kernel_nofault macros at all,
as they should be in user context according to the macros naming.
So for these {__get/__put}_kernel_nofault macros we should use
either existing kernel-context checks or introduce new ones.
Please advise.
Patch for this, has been merged in commit 88ad9dc30bbf("
mm, kasan: instrument copy_from/to_kernel_nofault")
in linux-next tree. Though currently, for arm64 HW_TAGS
the new copy_from_to_kernel_nofault_oob()
kunit test is disabled:
KASAN_TEST_NEEDS_CONFIG_OFF(test, CONFIG_KASAN_HW_TAGS);
Need further investigation on kunit test arm64 HW_TAGS issue.
Are you sure we need to check both arguments? I suspect it may lead to false positives. As far as I see, kernel code uses copy_from_kernel_nofault mostly to copy from "random" pointers (in the sense they are valid at some point before the operation starts, but may become invalid while it runs, up to the point that the memory may become unmapped, thus "nofailt", however, the result is later re-validated): https://elixir.bootlin.com/linux/v6.11/source/mm/slub.c#L544 https://elixir.bootlin.com/linux/v6.11/source/fs/d_path.c#L50 https://elixir.bootlin.com/linux/v6.11/source/arch/x86/kernel/ftrace.c#L92 If the source pointer is random, it may as well point to an unrelated KASAN-protected memory (which is actually likely b/c if it's heap memory and it was freed, it's likely to be in the quarantine). The patch will soon reach syzbot and it may start spewing lots of false positive reports. There is no config to disable this feature, so the reports will happen until a fixing patch reaches all tested trees. Thanks for the comment. I was trying with instrument_read/write() initially when the check was in x86 macros and it triggered 2/4 cases in kunit test. https://lore.kernel.org/linux-mm/20240918105641.704070-1-snovitoll@gmail.com/T/#u Let me make a PATCH with instrument_read()/write() in mm/maccess.c. Checking it now on the latest Linus tree on x86, arm64 SW_TAGS. Hopefully, it will be delivered today before false-positives. PATCH has been sent. Please review https://lore.kernel.org/linux-mm/20240930102405.2227124-1-snovitoll@gmail.com/T/#u PATCH with Marco's comments has been merged to -mm tree as the squashed commit dropping prev. patches. https://lore.kernel.org/all/20241005164813.2475778-2-snovitoll@gmail.com/T/#m314b7f19ded0915d925cff29361291cd5c479617 Resolved by Sabyrzhan in [1]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e4137f08816bbf91fe76d1b60fa16862a4827ac1 Filed [1] to track the issue with HW_TAGS. [1] https://bugzilla.kernel.org/show_bug.cgi?id=219523 |
KASAN should check both arguments of copy_from/to_kernel_nofault() for accessibility when both are fault-safe. KASAN also needs some reasonable checking scheme when accessing one of the arguments leads to a fault. ==== Test static void copy_from_to_kernel(struct kunit *test) { char *ptr; char buf[16]; size_t size = sizeof(buf); ptr = kmalloc(size, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); kfree(ptr); KUNIT_EXPECT_KASAN_FAIL(test, copy_from_kernel_nofault(&buf[0], ptr, size)); // fails 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)); // fails }