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

[Andrey Konovalov <andreyknvl@gmail.com>]

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!