| Message ID | 884e37ddff31b671725f4d83106111c7dcf8fb9b.1612208222.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: optimizations and fixes for HW_TAGS | expand |
On Mon, Feb 01, 2021 at 08:43PM +0100, Andrey Konovalov wrote: > Currently, if krealloc() is called on a freed object with KASAN enabled, > it allocates and returns a new object, but doesn't copy any memory from > the old one as ksize() returns 0. This makes a caller believe that > krealloc() succeeded (KASAN report is printed though). > > This patch adds an accessibility check into __do_krealloc(). If the check > fails, krealloc() returns NULL. This check duplicates the one in ksize(); > this is fixed in the following patch. I think "side-effect" is ambiguous, because either way behaviour of krealloc differs from a kernel with KASAN disabled. Something like "kasan, mm: fail krealloc on already freed object" perhaps? > This patch also adds a KASAN-KUnit test to check krealloc() behaviour > when it's called on a freed object. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> Reviewed-by: Marco Elver <elver@google.com> > --- > lib/test_kasan.c | 20 ++++++++++++++++++++ > mm/slab_common.c | 3 +++ > 2 files changed, 23 insertions(+) > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c > index 2bb52853f341..61bc894d9f7e 100644 > --- a/lib/test_kasan.c > +++ b/lib/test_kasan.c > @@ -359,6 +359,25 @@ static void krealloc_pagealloc_less_oob(struct kunit *test) > KMALLOC_MAX_CACHE_SIZE + 201); > } > > +/* > + * Check that krealloc() detects a use-after-free, returns NULL, > + * and doesn't unpoison the freed object. > + */ > +static void krealloc_uaf(struct kunit *test) > +{ > + char *ptr1, *ptr2; > + int size1 = 201; > + int size2 = 235; > + > + ptr1 = kmalloc(size1, GFP_KERNEL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr1); > + kfree(ptr1); > + > + KUNIT_EXPECT_KASAN_FAIL(test, ptr2 = krealloc(ptr1, size2, GFP_KERNEL)); > + KUNIT_ASSERT_PTR_EQ(test, (void *)ptr2, NULL); > + KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)ptr1); > +} > + > static void kmalloc_oob_16(struct kunit *test) > { > struct { > @@ -1056,6 +1075,7 @@ static struct kunit_case kasan_kunit_test_cases[] = { > KUNIT_CASE(krealloc_less_oob), > KUNIT_CASE(krealloc_pagealloc_more_oob), > KUNIT_CASE(krealloc_pagealloc_less_oob), > + KUNIT_CASE(krealloc_uaf), > KUNIT_CASE(kmalloc_oob_16), > KUNIT_CASE(kmalloc_uaf_16), > KUNIT_CASE(kmalloc_oob_in_memset), > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 39d1a8ff9bb8..dad70239b54c 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1140,6 +1140,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size, > void *ret; > size_t ks; > > + if (likely(!ZERO_OR_NULL_PTR(p)) && !kasan_check_byte(p)) > + return NULL; > + > ks = ksize(p); > > if (ks >= new_size) { > -- > 2.30.0.365.g02bc693789-goog >
diff --git a/lib/test_kasan.c b/lib/test_kasan.c index 2bb52853f341..61bc894d9f7e 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -359,6 +359,25 @@ static void krealloc_pagealloc_less_oob(struct kunit *test) KMALLOC_MAX_CACHE_SIZE + 201); } +/* + * Check that krealloc() detects a use-after-free, returns NULL, + * and doesn't unpoison the freed object. + */ +static void krealloc_uaf(struct kunit *test) +{ + char *ptr1, *ptr2; + int size1 = 201; + int size2 = 235; + + ptr1 = kmalloc(size1, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr1); + kfree(ptr1); + + KUNIT_EXPECT_KASAN_FAIL(test, ptr2 = krealloc(ptr1, size2, GFP_KERNEL)); + KUNIT_ASSERT_PTR_EQ(test, (void *)ptr2, NULL); + KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)ptr1); +} + static void kmalloc_oob_16(struct kunit *test) { struct { @@ -1056,6 +1075,7 @@ static struct kunit_case kasan_kunit_test_cases[] = { KUNIT_CASE(krealloc_less_oob), KUNIT_CASE(krealloc_pagealloc_more_oob), KUNIT_CASE(krealloc_pagealloc_less_oob), + KUNIT_CASE(krealloc_uaf), KUNIT_CASE(kmalloc_oob_16), KUNIT_CASE(kmalloc_uaf_16), KUNIT_CASE(kmalloc_oob_in_memset), diff --git a/mm/slab_common.c b/mm/slab_common.c index 39d1a8ff9bb8..dad70239b54c 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1140,6 +1140,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size, void *ret; size_t ks; + if (likely(!ZERO_OR_NULL_PTR(p)) && !kasan_check_byte(p)) + return NULL; + ks = ksize(p); if (ks >= new_size) {
Currently, if krealloc() is called on a freed object with KASAN enabled, it allocates and returns a new object, but doesn't copy any memory from the old one as ksize() returns 0. This makes a caller believe that krealloc() succeeded (KASAN report is printed though). This patch adds an accessibility check into __do_krealloc(). If the check fails, krealloc() returns NULL. This check duplicates the one in ksize(); this is fixed in the following patch. This patch also adds a KASAN-KUnit test to check krealloc() behaviour when it's called on a freed object. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- lib/test_kasan.c | 20 ++++++++++++++++++++ mm/slab_common.c | 3 +++ 2 files changed, 23 insertions(+)