Received-SPF: pass (google.com: domain of linux-kernel+bounces-40218-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 26 Jan 2024 15:44:33 +0100
From: Marco Elver <elver@google.com>
To: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Alexander Potapenko <glider@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Andrey Ryabinin <ryabinin.a.a@gmail.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com,
	linux-mm@kvack.org
Subject: Re: [PATCH 2/2] kasan: revert eviction of stack traces in generic
 mode
Message-ID: <ZbPFUXNeENyuwync@elver.google.com>
References: <20240125094815.2041933-1-elver@google.com>
 <20240125094815.2041933-2-elver@google.com>
 <CA+fCnZc6L3t3AdQS1rjFCT0s6RpT+q4Z4GmctOveeaDJW0tBow@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+fCnZc6L3t3AdQS1rjFCT0s6RpT+q4Z4GmctOveeaDJW0tBow@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)

On Thu, Jan 25, 2024 at 11:36PM +0100, Andrey Konovalov wrote:
[...]
> 
> Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
> 
> But I'm wondering if we should also stop resetting metadata when the
> object is fully freed (from quarantine or bypassing quarantine).
> 
> With stack_depot_put, I had to put the stack handles on free, as
> otherwise we would leak the stack depot references. And I also chose
> to memset meta at that point, as its gets invalid anyway. But without
> stack_depot_put, this is not required.
> 
> Before the stack depot-related changes, the code was inconsistent in
> this regard AFAICS: for quarantine, free meta was marked as invalid
> via KASAN_SLAB_FREE but alloc meta was kept; for no quarantine, both
> alloc and free meta were kept.
> 
> So perhaps we can just keep both metas on full free. I.e. drop both
> kasan_release_object_meta calls. This will go back to the old behavior
> + keeping free meta for the quarantine case (I think there's no harm
> in that). This will give better reporting for uaf-before-realloc bugs.
> 
> WDYT?

Yes, that makes sense.

You mean this on top?

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index ad32803e34e9..0577db1d2c62 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -264,12 +264,6 @@ bool __kasan_slab_free(struct kmem_cache *cache, void *object,
 	if (kasan_quarantine_put(cache, object))
 		return true;
 
-	/*
-	 * If the object is not put into quarantine, it will likely be quickly
-	 * reallocated. Thus, release its metadata now.
-	 */
-	kasan_release_object_meta(cache, object);
-
 	/* Let slab put the object onto the freelist. */
 	return false;
 }
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 8bfb52b28c22..fc9cf1860efb 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -510,20 +510,6 @@ static void release_free_meta(const void *object, struct kasan_free_meta *meta)
 	*(u8 *)kasan_mem_to_shadow(object) = KASAN_SLAB_FREE;
 }
 
-void kasan_release_object_meta(struct kmem_cache *cache, const void *object)
-{
-	struct kasan_alloc_meta *alloc_meta;
-	struct kasan_free_meta *free_meta;
-
-	alloc_meta = kasan_get_alloc_meta(cache, object);
-	if (alloc_meta)
-		release_alloc_meta(alloc_meta);
-
-	free_meta = kasan_get_free_meta(cache, object);
-	if (free_meta)
-		release_free_meta(object, free_meta);
-}
-
 size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object)
 {
 	struct kasan_cache *info = &cache->kasan_info;
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 216ae0ef1e4b..fb2b9ac0659a 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -390,10 +390,8 @@ struct kasan_alloc_meta *kasan_get_alloc_meta(struct kmem_cache *cache,
 struct kasan_free_meta *kasan_get_free_meta(struct kmem_cache *cache,
 						const void *object);
 void kasan_init_object_meta(struct kmem_cache *cache, const void *object);
-void kasan_release_object_meta(struct kmem_cache *cache, const void *object);
 #else
 static inline void kasan_init_object_meta(struct kmem_cache *cache, const void *object) { }
-static inline void kasan_release_object_meta(struct kmem_cache *cache, const void *object) { }
 #endif
 
 depot_stack_handle_t kasan_save_stack(gfp_t flags, depot_flags_t depot_flags);
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 3ba02efb952a..a758c2e10703 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -145,8 +145,6 @@ static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
 	void *object = qlink_to_object(qlink, cache);
 	struct kasan_free_meta *free_meta = kasan_get_free_meta(cache, object);
 
-	kasan_release_object_meta(cache, object);
-
 	/*
 	 * If init_on_free is enabled and KASAN's free metadata is stored in
 	 * the object, zero the metadata. Otherwise, the object's memory will