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

[Andrey Konovalov <andreyknvl@gmail.com>]

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.