| 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!
On Sun, Sep 22, 2024 at 1:49 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > 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. Thanks for testing it on arm64. I've checked other arch and found out that only s390, x86 are using <linux/instrumented.h> header with KASAN and friends in annotations. <linux/kasan-checks.h> is in arm64 and x86. While the current [PATCH v4] has x86 only instrumentations for __get/put_kernel_nofault, I think, we can take as an example copy_from_user solution here: https://elixir.bootlin.com/linux/v6.11-rc7/source/include/linux/uaccess.h#L162-L164 , which should be a generic instrumentation of __get/put_kernel_nofault for all arch. I can try to make a separate PATCH with this solution. > 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. Please suggest if the solution above to make a generic instrumentation of __get/put_kernel_nofault is suitable. Otherwise, for this patch as you've suggested, we can add KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_X86); to make sure that kunit test is for x86 only and I can add arm64 kasan-checks with SW, HW tags in separate "mm, arm64" PATCH.
On Sun, Sep 22, 2024 at 11:26 AM Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > On Sun, Sep 22, 2024 at 1:49 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > > > 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. > > Thanks for testing it on arm64. I've checked other arch and found out > that only s390, x86 are using <linux/instrumented.h> header with > KASAN and friends in annotations. <linux/kasan-checks.h> is in arm64 and x86. > > While the current [PATCH v4] has x86 only instrumentations for > __get/put_kernel_nofault, I think, we can take as an example copy_from_user > solution here: > > https://elixir.bootlin.com/linux/v6.11-rc7/source/include/linux/uaccess.h#L162-L164 > > , which should be a generic instrumentation of __get/put_kernel_nofault > for all arch. I can try to make a separate PATCH with this solution. _inline_copy_from_user appears to only be called for non-kernel variants of copy_from_user, so you would need something different. > > 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. > > Please suggest if the solution above to make a generic instrumentation of > __get/put_kernel_nofault is suitable. I think the approach you have taken with adding instrument_read/write into arch code is fine, we just need to do this for all arches. An alternative would be common wrapper macros that calls __get/put_kernel_nofault + instrument_read/write. > Otherwise, for this patch as you've suggested, we can add > KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_X86); > to make sure that kunit test is for x86 only and I can add arm64 kasan-checks > with SW, HW tags in separate "mm, arm64" PATCH. Sounds good too.
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> > --- > 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(+) > > 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) instrument_get_user is KMSAN-related, so I don't think this change belongs as a part of this patch. Perhaps Alexander can comment on whether we need to add instrument_get_user here for KMSAN.
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(+)