| Message ID | 20240919105750.901303-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mm: x86: instrument __get/__put_kernel_nofault | expand |
On Thu, Sep 19, 2024 at 12:57 PM Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > On Wed, Sep 18, 2024 at 8:15 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > You still have the same problem here. > > > > What I meant is: > > > > char *ptr; > > char buf[128 - KASAN_GRANULE_SIZE]; > > size_t size = sizeof(buf); > > > > ptr = kmalloc(size, GFP_KERNEL); > > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); > > > > KUNIT_EXPECT_KASAN_FAIL(...); > > ... > > > > kfree(ptr); > > Thanks for catching this! I've turned kunit test into OOB instead of UAF. > --- > v3: changed kunit test from UAF to OOB case and git commit message. > --- > 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 for each copy_from/to_kernel_nofault call. "as expected for each" => "as expected, one for each" > > 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> > --- > 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) > > #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), > {} > }; The test looks good to me now. But you need to send the patch as a standalone email, without combining it with the response to my comment. 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), {} };