| Message ID | 20220923202822.2667581-15-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | slab: Introduce kmalloc_size_roundup() | expand |
On Fri, 23 Sept 2022 at 22:28, Kees Cook <keescook@chromium.org> wrote: > > In preparation for no longer unpoisoning in ksize(), remove the behavioral > self-tests for ksize(). > > Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com> > Cc: Alexander Potapenko <glider@google.com> > Cc: Andrey Konovalov <andreyknvl@gmail.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: kasan-dev@googlegroups.com > Cc: linux-mm@kvack.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > lib/test_kasan.c | 42 ------------------------------------------ > mm/kasan/shadow.c | 4 +--- > 2 files changed, 1 insertion(+), 45 deletions(-) > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c > index 58c1b01ccfe2..bdd0ced8f8d7 100644 > --- a/lib/test_kasan.c > +++ b/lib/test_kasan.c > @@ -753,46 +753,6 @@ static void kasan_global_oob_left(struct kunit *test) > KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)p); > } > > -/* Check that ksize() makes the whole object accessible. */ > -static void ksize_unpoisons_memory(struct kunit *test) > -{ > - char *ptr; > - size_t size = 123, real_size; > - > - ptr = kmalloc(size, GFP_KERNEL); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); > - real_size = ksize(ptr); > - > - OPTIMIZER_HIDE_VAR(ptr); > - > - /* This access shouldn't trigger a KASAN report. */ > - ptr[size] = 'x'; I would rather keep the tests and update to the new behavior. We had bugs in ksize, we need test coverage. I assume ptr[size] access must now produce an error even after ksize. > - /* This one must. */ > - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]); > - > - kfree(ptr); > -} > - > -/* > - * Check that a use-after-free is detected by ksize() and via normal accesses > - * after it. > - */ > -static void ksize_uaf(struct kunit *test) > -{ > - char *ptr; > - int size = 128 - KASAN_GRANULE_SIZE; > - > - ptr = kmalloc(size, GFP_KERNEL); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); > - kfree(ptr); > - > - OPTIMIZER_HIDE_VAR(ptr); > - KUNIT_EXPECT_KASAN_FAIL(test, ksize(ptr)); This is still a bug that should be detected, right? Calling ksize on a freed pointer is a bug. > - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]); > - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[size]); > -} > - > static void kasan_stack_oob(struct kunit *test) > { > char stack_array[10]; > @@ -1392,8 +1352,6 @@ static struct kunit_case kasan_kunit_test_cases[] = { > KUNIT_CASE(kasan_stack_oob), > KUNIT_CASE(kasan_alloca_oob_left), > KUNIT_CASE(kasan_alloca_oob_right), > - KUNIT_CASE(ksize_unpoisons_memory), > - KUNIT_CASE(ksize_uaf), > KUNIT_CASE(kmem_cache_double_free), > KUNIT_CASE(kmem_cache_invalid_free), > KUNIT_CASE(kmem_cache_double_destroy), > diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c > index 0e3648b603a6..0895c73e9b69 100644 > --- a/mm/kasan/shadow.c > +++ b/mm/kasan/shadow.c > @@ -124,9 +124,7 @@ void kasan_unpoison(const void *addr, size_t size, bool init) > addr = kasan_reset_tag(addr); > > /* > - * Skip KFENCE memory if called explicitly outside of sl*b. Also note > - * that calls to ksize(), where size is not a multiple of machine-word > - * size, would otherwise poison the invalid portion of the word. > + * Skip KFENCE memory if called explicitly outside of sl*b. > */ > if (is_kfence_address(addr)) > return; > -- > 2.34.1
On Sat, Sep 24, 2022 at 10:15:18AM +0200, Dmitry Vyukov wrote: > On Fri, 23 Sept 2022 at 22:28, Kees Cook <keescook@chromium.org> wrote: > > > > In preparation for no longer unpoisoning in ksize(), remove the behavioral > > self-tests for ksize(). > > > > [...] > > -/* Check that ksize() makes the whole object accessible. */ > > -static void ksize_unpoisons_memory(struct kunit *test) > > -{ > > - char *ptr; > > - size_t size = 123, real_size; > > - > > - ptr = kmalloc(size, GFP_KERNEL); > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); > > - real_size = ksize(ptr); > > - > > - OPTIMIZER_HIDE_VAR(ptr); > > - > > - /* This access shouldn't trigger a KASAN report. */ > > - ptr[size] = 'x'; > > I would rather keep the tests and update to the new behavior. We had > bugs in ksize, we need test coverage. > I assume ptr[size] access must now produce an error even after ksize. Good point on all these! I'll respin.
diff --git a/lib/test_kasan.c b/lib/test_kasan.c index 58c1b01ccfe2..bdd0ced8f8d7 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -753,46 +753,6 @@ static void kasan_global_oob_left(struct kunit *test) KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)p); } -/* Check that ksize() makes the whole object accessible. */ -static void ksize_unpoisons_memory(struct kunit *test) -{ - char *ptr; - size_t size = 123, real_size; - - ptr = kmalloc(size, GFP_KERNEL); - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); - real_size = ksize(ptr); - - OPTIMIZER_HIDE_VAR(ptr); - - /* This access shouldn't trigger a KASAN report. */ - ptr[size] = 'x'; - - /* This one must. */ - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]); - - kfree(ptr); -} - -/* - * Check that a use-after-free is detected by ksize() and via normal accesses - * after it. - */ -static void ksize_uaf(struct kunit *test) -{ - char *ptr; - int size = 128 - KASAN_GRANULE_SIZE; - - ptr = kmalloc(size, GFP_KERNEL); - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); - kfree(ptr); - - OPTIMIZER_HIDE_VAR(ptr); - KUNIT_EXPECT_KASAN_FAIL(test, ksize(ptr)); - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]); - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[size]); -} - static void kasan_stack_oob(struct kunit *test) { char stack_array[10]; @@ -1392,8 +1352,6 @@ static struct kunit_case kasan_kunit_test_cases[] = { KUNIT_CASE(kasan_stack_oob), KUNIT_CASE(kasan_alloca_oob_left), KUNIT_CASE(kasan_alloca_oob_right), - KUNIT_CASE(ksize_unpoisons_memory), - KUNIT_CASE(ksize_uaf), KUNIT_CASE(kmem_cache_double_free), KUNIT_CASE(kmem_cache_invalid_free), KUNIT_CASE(kmem_cache_double_destroy), diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c index 0e3648b603a6..0895c73e9b69 100644 --- a/mm/kasan/shadow.c +++ b/mm/kasan/shadow.c @@ -124,9 +124,7 @@ void kasan_unpoison(const void *addr, size_t size, bool init) addr = kasan_reset_tag(addr); /* - * Skip KFENCE memory if called explicitly outside of sl*b. Also note - * that calls to ksize(), where size is not a multiple of machine-word - * size, would otherwise poison the invalid portion of the word. + * Skip KFENCE memory if called explicitly outside of sl*b. */ if (is_kfence_address(addr)) return;
In preparation for no longer unpoisoning in ksize(), remove the behavioral self-tests for ksize(). Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com> Cc: Alexander Potapenko <glider@google.com> Cc: Andrey Konovalov <andreyknvl@gmail.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: kasan-dev@googlegroups.com Cc: linux-mm@kvack.org Signed-off-by: Kees Cook <keescook@chromium.org> --- lib/test_kasan.c | 42 ------------------------------------------ mm/kasan/shadow.c | 4 +--- 2 files changed, 1 insertion(+), 45 deletions(-)