| Message ID | 20210910203152.3549236-1-pcc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kasan: test: don't copy more than size bytes in memcpy test | expand |
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!
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
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);
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(-)