| Message ID | 20240917201817.657490-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: x86: instrument __get/__put_kernel_nofault | expand |
On Tue, Sep 17, 2024 at 10:18 PM 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. > > Regular instrument_read() and instrument_write() handles KASAN, KCSAN > checks for the access address, though instrument_memcpy_before() might > be considered as well for both src and dst address validation. > > __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. > > 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> Hi Sabyrzhan, Thanks for working on this! > diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c > index 7b32be2a3cf0..f5086c86e0bd 100644 > --- a/mm/kasan/kasan_test.c > +++ b/mm/kasan/kasan_test.c > @@ -1899,6 +1899,22 @@ static void match_all_mem_tag(struct kunit *test) > kfree(ptr); > } > > +static void copy_from_to_kernel_nofault(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)); > + KUNIT_EXPECT_KASAN_FAIL(test, > + copy_to_kernel_nofault(ptr, &buf[0], size)); I just realized that the test I wrote in the bug report is not good. This call will overwrite the object's contents and thus corrupt the freelist pointer. This might cause crashes in further tests. KASAN tests try to avoid harmfully corrupting memory to avoid crashes. I think the easiest fix would be to allocate e.g. 128 - KASAN_GRANULE_SIZE bytes and do an out-of-bounds up to 128 bytes via copy_to/from_kernel_nofault. This will only corrupt the in-object kmalloc redzone, which is not harmful. Also, I think we need to test all 4 calls that I had in the bug report to check both arguments of both functions. Not only the 2 you included. > +} Thank you!
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h index 3a7755c1a441..bed84d3f7245 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_read(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_read(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..f5086c86e0bd 100644 --- a/mm/kasan/kasan_test.c +++ b/mm/kasan/kasan_test.c @@ -1899,6 +1899,22 @@ static void match_all_mem_tag(struct kunit *test) kfree(ptr); } +static void copy_from_to_kernel_nofault(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)); + KUNIT_EXPECT_KASAN_FAIL(test, + copy_to_kernel_nofault(ptr, &buf[0], size)); +} + static struct kunit_case kasan_kunit_test_cases[] = { KUNIT_CASE(kmalloc_oob_right), KUNIT_CASE(kmalloc_oob_left), @@ -1971,6 +1987,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), {} };
Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(), strncpy_from_kernel_nofault() where __put_kernel_nofault, __get_kernel_nofault macros are used. Regular instrument_read() and instrument_write() handles KASAN, KCSAN checks for the access address, though instrument_memcpy_before() might be considered as well for both src and dst address validation. __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. 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 | 17 +++++++++++++++++ 2 files changed, 21 insertions(+)