Re: [PATCH] kasan: test: don't copy more than size bytes in memcpy test

[Andrey Konovalov <andreyknvl@gmail.com>]

On Fri, Sep 10, 2021 at 10:32 PM Peter Collingbourne <pcc@google.com> wrote:
>
> With HW tag-based KASAN, error checks are performed implicitly by the load
> and store instructions in the memcpy implementation.  A failed check results
> in tag checks being disabled and execution will keep going. As a result,
> under HW tag-based KASAN, this memcpy would end up corrupting memory until
> it hits an inaccessible page and causes a kernel panic.
>
> This is a pre-existing issue that was revealed by commit 285133040e6c ("arm64:
> Import latest memcpy()/memmove() implementation") which changed the memcpy
> implementation from using signed comparisons (incorrectly, resulting in
> the memcpy being terminated early for negative sizes) to using unsigned
> comparisons.
>
> It is unclear how this could be handled by memcpy itself in a reasonable
> way. One possibility would be to add an exception handler that would force
> memcpy to return if a tag check fault is detected -- this would make the
> behavior roughly similar to generic and SW tag-based KASAN. However, this
> wouldn't solve the problem for asynchronous mode and also makes memcpy
> behavior inconsistent with manually copying data.
>
> It may be more accurate to consider this a bug in the test: what we really
> want to test here is that a memcpy overflow, however small, is caught, and any
> further copying after the initial overflow is unnecessary and may affect system
> stability. Therefore, adjust the test to pass the allocation size as the memcpy
> size, ensuring that the memcpy will not result in an out-of-bounds write.
>
> Commit 1b0668be62cf ("kasan: test: disable kmalloc_memmove_invalid_size for
> HW_TAGS") disabled this test in HW tags mode, but there is some value in
> testing small memcpy overflows, so let's re-enable it with this fix.
>
> Link: https://linux-review.googlesource.com/id/I048d1e6a9aff766c4a53f989fb0c83de68923882
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> ---
>  lib/test_kasan.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 8835e0784578..9af51e1f692d 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -497,14 +497,7 @@ static void kmalloc_memmove_invalid_size(struct kunit *test)
>  {
>         char *ptr;
>         size_t size = 64;
> -       volatile size_t invalid_size = -2;
> -
> -       /*
> -        * Hardware tag-based mode doesn't check memmove for negative size.
> -        * As a result, this test introduces a side-effect memory corruption,
> -        * which can result in a crash.
> -        */
> -       KASAN_TEST_NEEDS_CONFIG_OFF(test, CONFIG_KASAN_HW_TAGS);
> +       volatile size_t invalid_size = size;
>
>         ptr = kmalloc(size, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> --
> 2.33.0.309.g3052b89438-goog
>

Hi Peter,

This test was added as a part of series that taught KASAN to detect
negative sizes in memory operations, see 8cceeff48f23 ("kasan: detect
negative size in memory operation function"). So we need to keep it
using negative sizes.

I think we should rename kmalloc_memmove_invalid_size to
kmalloc_memmove_negative_size, and keep it disabled with HW_TAGS. And
add another test named kmalloc_memmove_invalid_size, which does what
you did in this patch.

Thanks!