Re: [PATCH v5] mm: x86: instrument __get/__put_kernel_nofault

[Sabyrzhan Tasbolatov <snovitoll@gmail.com>]

On Sun, Sep 22, 2024 at 7:57 PM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
>  arch/x86/include/asm/uaccess.h |  3 +++
>  mm/kasan/kasan_test.c          | 23 +++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 3a7755c1a441..e8e5185dd65c 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -620,6 +620,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 +628,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 +637,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)
>

Instead of adding KASAN, KCSAN checks per arch macro,
here is the alternative, generic way with a wrapper.
I've tested it on x86_64 only, going to test on arm64
with KASAN_SW_TAGS, KASAN_HW_TAGS if I can do it in qemu,
and form a new patch for all arch
and this PATCH v5 for x86 only can be abandoned.

Please let me know if this wrapper is good enough,
I will see in kasan_test.c how I should use SW/HW_TAG, probably,
they should be a separate test with
KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_SW_TAGS);
---
 include/linux/uaccess.h |  8 ++++++++
 mm/kasan/kasan_test.c   | 21 +++++++++++++++++++++
 mm/maccess.c            |  4 ++--
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index d8e4105a2f21..1b5c23868f97 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -422,6 +422,14 @@ do { \
 } while (0)
 #endif

+#define __get_kernel_nofault_wrapper(dst, src, type, err_label) \
+ instrument_memcpy_before(dst, src, sizeof(type)); \
+ __get_kernel_nofault(dst, src, type, err_label); \
+
+#define __put_kernel_nofault_wrapper(dst, src, type, err_label) \
+ instrument_write(dst, sizeof(type)); \
+ __put_kernel_nofault(dst, src, type, err_label); \
+
 /**
  * get_kernel_nofault(): safely attempt to read from a location
  * @val: read into this variable
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index 567d33b493e2..ae05c8858c07 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -1944,6 +1944,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),
@@ -2017,6 +2037,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),
  {}
 };

diff --git a/mm/maccess.c b/mm/maccess.c
index 518a25667323..a3533a0d0677 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -15,7 +15,7 @@ bool __weak copy_from_kernel_nofault_allowed(const
void *unsafe_src,

 #define copy_from_kernel_nofault_loop(dst, src, len, type, err_label) \
  while (len >= sizeof(type)) { \
- __get_kernel_nofault(dst, src, type, err_label); \
+ __get_kernel_nofault_wrapper(dst, src, type, err_label);\
  dst += sizeof(type); \
  src += sizeof(type); \
  len -= sizeof(type); \
@@ -49,7 +49,7 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);

 #define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \
  while (len >= sizeof(type)) { \
- __put_kernel_nofault(dst, src, type, err_label); \
+ __put_kernel_nofault_wrapper(dst, src, type, err_label);\
  dst += sizeof(type); \
  src += sizeof(type); \
  len -= sizeof(type); \
--