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

[Peter Collingbourne <pcc@google.com>]

On Fri, Sep 10, 2021 at 1:44 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> 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.

Makes sense, done in v2.

Peter