linux-kernel - Re: [PATCH v3 1/2] kasan: catch invalid free before SLUB reinitializes the object

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez0hAN-bJtQtbTiNa15qkHQ+67hy95Aybgw24LyNWbuU0g@mail.gmail.com>
Date: Fri, 26 Jul 2024 15:51:25 +0200
From: Jann Horn <jannh@...gle.com>
To: Andrey Konovalov <andreyknvl@...il.com>
Cc: Andrey Ryabinin <ryabinin.a.a@...il.com>, Alexander Potapenko <glider@...gle.com>, 
	Dmitry Vyukov <dvyukov@...gle.com>, Vincenzo Frascino <vincenzo.frascino@....com>, 
	Andrew Morton <akpm@...ux-foundation.org>, Christoph Lameter <cl@...ux.com>, 
	Pekka Enberg <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>, 
	Joonsoo Kim <iamjoonsoo.kim@....com>, Vlastimil Babka <vbabka@...e.cz>, 
	Roman Gushchin <roman.gushchin@...ux.dev>, Hyeonggon Yoo <42.hyeyoo@...il.com>, 
	Marco Elver <elver@...gle.com>, kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org
Subject: Re: [PATCH v3 1/2] kasan: catch invalid free before SLUB
 reinitializes the object

On Fri, Jul 26, 2024 at 2:43 AM Andrey Konovalov <andreyknvl@...il.com> wrote:
> On Thu, Jul 25, 2024 at 5:32 PM Jann Horn <jannh@...gle.com> wrote:
> >
> > Currently, when KASAN is combined with init-on-free behavior, the
> > initialization happens before KASAN's "invalid free" checks.
> >
> > More importantly, a subsequent commit will want to use the object metadata
> > region to store an rcu_head, and we should let KASAN check that the object
> > pointer is valid before that. (Otherwise that change will make the existing
> > testcase kmem_cache_invalid_free fail.)
>
> This is not the case since v3, right?

Oh, you're right, this text is now wrong.

> Do we still need this patch?

I just tried removing this patch from the series; without it, the
kmem_cache_invalid_free kunit test fails because the kmem_cache_free()
no longer synchronously notices that the pointer is misaligned. I
guess I could change the testcase like this to make the tests pass
without this patch, but I'd like to hear from you or another KASAN
person whether you think that's a reasonable change:

diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index cba782a4b072..f44b0dcb0e84 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -981,14 +981,21 @@ static void kmem_cache_invalid_free(struct kunit *test)
        if (!p) {
                kunit_err(test, "Allocation failed: %s\n", __func__);
                kmem_cache_destroy(cache);
                return;
        }

-       /* Trigger invalid free, the object doesn't get freed. */
-       KUNIT_EXPECT_KASAN_FAIL(test, kmem_cache_free(cache, p + 1));
+       /*
+        * Trigger invalid free, the object doesn't get freed.
+        * Note that the invalid free detection may happen asynchronously
+        * under CONFIG_SLUB_RCU_DEBUG.
+        */
+       KUNIT_EXPECT_KASAN_FAIL(test, ({
+               kmem_cache_free(cache, p + 1);
+               rcu_barrier();
+       }));

Being able to get rid of this patch would be a nice simplification, so
if you think asynchronous invalid-free detection for TYPESAFE_BY_RCU
slabs is fine, I'll happily throw it out.