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

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

[Peter Collingbourne]

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

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

[Peter Collingbourne]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andrey Konovalov]

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!

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

[Andrey Konovalov]

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peter Collingbourne]

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

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

[Peter Collingbourne]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel