[PATCH v5 0/2] allow KASAN to detect UAF in SLAB_TYPESAFE_BY_RCU slabs

[PATCH v5 0/2] allow KASAN to detect UAF in SLAB_TYPESAFE_BY_RCU slabs

[Jann Horn]

Hi!

The purpose of the series is to allow KASAN to detect use-after-free
access in SLAB_TYPESAFE_BY_RCU slab caches, by essentially making them
behave as if the cache was not SLAB_TYPESAFE_BY_RCU but instead every
kfree() in the cache was a kfree_rcu().
This is gated behind a config flag that is supposed to only be enabled
in fuzzing/testing builds where the performance impact doesn't matter.

Output of the new kunit testcase I added to the KASAN test suite:
==================================================================
BUG: KASAN: slab-use-after-free in kmem_cache_rcu_uaf+0x3ae/0x4d0
Read of size 1 at addr ffff888106224000 by task kunit_try_catch/224

CPU: 7 PID: 224 Comm: kunit_try_catch Tainted: G    B            N 6.10.0-00003-g065427d4b87f #430
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x53/0x70
 print_report+0xce/0x670
[...]
 kasan_report+0xa5/0xe0
[...]
 kmem_cache_rcu_uaf+0x3ae/0x4d0
[...]
 kunit_try_run_case+0x1b3/0x490
[...]
 kunit_generic_run_threadfn_adapter+0x80/0xe0
 kthread+0x2a5/0x370
[...]
 ret_from_fork+0x34/0x70
[...]
 ret_from_fork_asm+0x1a/0x30
 </TASK>

Allocated by task 224:
 kasan_save_stack+0x33/0x60
 kasan_save_track+0x14/0x30
 __kasan_slab_alloc+0x6e/0x70
 kmem_cache_alloc_noprof+0xef/0x2b0
 kmem_cache_rcu_uaf+0x10d/0x4d0
 kunit_try_run_case+0x1b3/0x490
 kunit_generic_run_threadfn_adapter+0x80/0xe0
 kthread+0x2a5/0x370
 ret_from_fork+0x34/0x70
 ret_from_fork_asm+0x1a/0x30

Freed by task 0:
 kasan_save_stack+0x33/0x60
 kasan_save_track+0x14/0x30
 kasan_save_free_info+0x3b/0x60
 __kasan_slab_free+0x57/0x80
 slab_free_after_rcu_debug+0xe3/0x220
 rcu_core+0x676/0x15b0
 handle_softirqs+0x22f/0x690
 irq_exit_rcu+0x84/0xb0
 sysvec_apic_timer_interrupt+0x6a/0x80
 asm_sysvec_apic_timer_interrupt+0x1a/0x20

Last potentially related work creation:
 kasan_save_stack+0x33/0x60
 __kasan_record_aux_stack+0x8e/0xa0
 kmem_cache_free+0x10c/0x420
 kmem_cache_rcu_uaf+0x16e/0x4d0
 kunit_try_run_case+0x1b3/0x490
 kunit_generic_run_threadfn_adapter+0x80/0xe0
 kthread+0x2a5/0x370
 ret_from_fork+0x34/0x70
 ret_from_fork_asm+0x1a/0x30

The buggy address belongs to the object at ffff888106224000
 which belongs to the cache test_cache of size 200
The buggy address is located 0 bytes inside of
 freed 200-byte region [ffff888106224000, ffff8881062240c8)

The buggy address belongs to the physical page:
page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x106224
head: order:1 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
flags: 0x200000000000040(head|node=0|zone=2)
page_type: 0xffffefff(slab)
raw: 0200000000000040 ffff88810621c140 dead000000000122 0000000000000000
raw: 0000000000000000 00000000801f001f 00000001ffffefff 0000000000000000
head: 0200000000000040 ffff88810621c140 dead000000000122 0000000000000000
head: 0000000000000000 00000000801f001f 00000001ffffefff 0000000000000000
head: 0200000000000001 ffffea0004188901 ffffffffffffffff 0000000000000000
head: 0000000000000002 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff888106223f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff888106223f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888106224000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                   ^
 ffff888106224080: fb fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc
 ffff888106224100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================
    ok 38 kmem_cache_rcu_uaf

Signed-off-by: Jann Horn <jannh@google.com>
---
Changes in v5:
- rebase to latest origin/master (akpm), no other changes from v4
- Link to v4: https://lore.kernel.org/r/20240729-kasan-tsbrcu-v4-0-57ec85ef80c6@google.com

Changes in v4:
- note I kept vbabka's ack for the SLUB changes in patch 1/2 since the
  SLUB part didn't change, even though I refactored a bunch of the
  KASAN parts
- in patch 1/2 (major rework):
  - fix commit message (Andrey)
  - add doc comments in header (Andrey)
  - remove "ip" argument from __kasan_slab_free()
  - rework the whole check_slab_free() thing and move code around (Andrey)
- in patch 2/2:
  - kconfig description and dependency changes (Andrey)
  - remove useless linebreak (Andrey)
  - fix comment style (Andrey)
  - fix do_slab_free() invocation (kernel test robot)
- Link to v3: https://lore.kernel.org/r/20240725-kasan-tsbrcu-v3-0-51c92f8f1101@google.com

Changes in v3:
- in patch 1/2, integrate akpm's fix for !CONFIG_KASAN build failure
- in patch 2/2, as suggested by vbabka, use dynamically allocated
  rcu_head to avoid having to add slab metadata
- in patch 2/2, add a warning in the kconfig help text that objects can
  be recycled immediately under memory pressure
- Link to v2: https://lore.kernel.org/r/20240724-kasan-tsbrcu-v2-0-45f898064468@google.com

Changes in v2:
Patch 1/2 is new; it's some necessary prep work for the main patch to
work, though the KASAN integration maybe is a bit ugly.
Patch 2/2 is a rebased version of the old patch, with some changes to
how the config is wired up, with poison/unpoison logic added as
suggested by dvyukov@ back then, with cache destruction fixed using
rcu_barrier() as pointed out by dvyukov@ and the test robot, and a test
added as suggested by elver@.

---
Jann Horn (2):
      kasan: catch invalid free before SLUB reinitializes the object
      slub: Introduce CONFIG_SLUB_RCU_DEBUG

 include/linux/kasan.h | 50 +++++++++++++++++++++++++++----
 mm/Kconfig.debug      | 30 +++++++++++++++++++
 mm/kasan/common.c     | 60 +++++++++++++++++++++++--------------
 mm/kasan/kasan_test.c | 46 ++++++++++++++++++++++++++++
 mm/slab_common.c      | 12 ++++++++
 mm/slub.c             | 83 ++++++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 245 insertions(+), 36 deletions(-)
---
base-commit: 94ede2a3e9135764736221c080ac7c0ad993dc2d
change-id: 20240723-kasan-tsbrcu-b715a901f776
-- 
Jann Horn <jannh@google.com>

[PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Jann Horn]

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 RCU-delay the actual
SLUB freeing of an object, and we'd like KASAN to still validate
synchronously that freeing the object is permitted. (Otherwise this
change will make the existing testcase kmem_cache_invalid_free fail.)

So add a new KASAN hook that allows KASAN to pre-validate a
kmem_cache_free() operation before SLUB actually starts modifying the
object or its metadata.

Inside KASAN, this:

 - moves checks from poison_slab_object() into check_slab_free()
 - moves kasan_arch_is_ready() up into callers of poison_slab_object()
 - removes "ip" argument of poison_slab_object() and __kasan_slab_free()
   (since those functions no longer do any reporting)
 - renames check_slab_free() to check_slab_allocation()

Acked-by: Vlastimil Babka <vbabka@suse.cz> #slub
Signed-off-by: Jann Horn <jannh@google.com>
---
 include/linux/kasan.h | 43 ++++++++++++++++++++++++++++++++++---
 mm/kasan/common.c     | 59 +++++++++++++++++++++++++++++++--------------------
 mm/slub.c             |  7 ++++++
 3 files changed, 83 insertions(+), 26 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 70d6a8f6e25d..34cb7a25aacb 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -172,19 +172,50 @@ static __always_inline void * __must_check kasan_init_slab_obj(
 {
 	if (kasan_enabled())
 		return __kasan_init_slab_obj(cache, object);
 	return (void *)object;
 }
 
-bool __kasan_slab_free(struct kmem_cache *s, void *object,
-			unsigned long ip, bool init);
+bool __kasan_slab_pre_free(struct kmem_cache *s, void *object,
+			unsigned long ip);
+/**
+ * kasan_slab_pre_free - Validate a slab object freeing request.
+ * @object: Object to free.
+ *
+ * This function checks whether freeing the given object might be permitted; it
+ * checks things like whether the given object is properly aligned and not
+ * already freed.
+ *
+ * This function is only intended for use by the slab allocator.
+ *
+ * @Return true if freeing the object is known to be invalid; false otherwise.
+ */
+static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s,
+						void *object)
+{
+	if (kasan_enabled())
+		return __kasan_slab_pre_free(s, object, _RET_IP_);
+	return false;
+}
+
+bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init);
+/**
+ * kasan_slab_free - Possibly handle slab object freeing.
+ * @object: Object to free.
+ *
+ * This hook is called from the slab allocator to give KASAN a chance to take
+ * ownership of the object and handle its freeing.
+ * kasan_slab_pre_free() must have already been called on the same object.
+ *
+ * @Return true if KASAN took ownership of the object; false otherwise.
+ */
 static __always_inline bool kasan_slab_free(struct kmem_cache *s,
 						void *object, bool init)
 {
 	if (kasan_enabled())
-		return __kasan_slab_free(s, object, _RET_IP_, init);
+		return __kasan_slab_free(s, object, init);
 	return false;
 }
 
 void __kasan_kfree_large(void *ptr, unsigned long ip);
 static __always_inline void kasan_kfree_large(void *ptr)
 {
@@ -368,12 +399,18 @@ static inline void kasan_poison_new_object(struct kmem_cache *cache,
 					void *object) {}
 static inline void *kasan_init_slab_obj(struct kmem_cache *cache,
 				const void *object)
 {
 	return (void *)object;
 }
+
+static inline bool kasan_slab_pre_free(struct kmem_cache *s, void *object)
+{
+	return false;
+}
+
 static inline bool kasan_slab_free(struct kmem_cache *s, void *object, bool init)
 {
 	return false;
 }
 static inline void kasan_kfree_large(void *ptr) {}
 static inline void *kasan_slab_alloc(struct kmem_cache *s, void *object,
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 85e7c6b4575c..8cede1ce00e1 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -205,59 +205,65 @@ void * __must_check __kasan_init_slab_obj(struct kmem_cache *cache,
 	/* Tag is ignored in set_tag() without CONFIG_KASAN_SW/HW_TAGS */
 	object = set_tag(object, assign_tag(cache, object, true));
 
 	return (void *)object;
 }
 
-static inline bool poison_slab_object(struct kmem_cache *cache, void *object,
-				      unsigned long ip, bool init)
+/* returns true for invalid request */
+static bool check_slab_allocation(struct kmem_cache *cache, void *object,
+				  unsigned long ip)
 {
-	void *tagged_object;
-
-	if (!kasan_arch_is_ready())
-		return false;
+	void *tagged_object = object;
 
-	tagged_object = object;
 	object = kasan_reset_tag(object);
 
 	if (unlikely(nearest_obj(cache, virt_to_slab(object), object) != object)) {
 		kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT_INVALID_FREE);
 		return true;
 	}
 
-	/* RCU slabs could be legally used after free within the RCU period. */
-	if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
-		return false;
-
 	if (!kasan_byte_accessible(tagged_object)) {
 		kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT_DOUBLE_FREE);
 		return true;
 	}
 
+	return false;
+}
+
+static inline void poison_slab_object(struct kmem_cache *cache, void *object,
+				      bool init)
+{
+	void *tagged_object = object;
+
+	object = kasan_reset_tag(object);
+
+	/* RCU slabs could be legally used after free within the RCU period. */
+	if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
+		return;
+
 	kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_SIZE),
 			KASAN_SLAB_FREE, init);
 
 	if (kasan_stack_collection_enabled())
 		kasan_save_free_info(cache, tagged_object);
+}
 
-	return false;
+bool __kasan_slab_pre_free(struct kmem_cache *cache, void *object,
+				unsigned long ip)
+{
+	if (!kasan_arch_is_ready() || is_kfence_address(object))
+		return false;
+	return check_slab_allocation(cache, object, ip);
 }
 
-bool __kasan_slab_free(struct kmem_cache *cache, void *object,
-				unsigned long ip, bool init)
+bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init)
 {
-	if (is_kfence_address(object))
+	if (!kasan_arch_is_ready() || is_kfence_address(object))
 		return false;
 
-	/*
-	 * If the object is buggy, do not let slab put the object onto the
-	 * freelist. The object will thus never be allocated again and its
-	 * metadata will never get released.
-	 */
-	if (poison_slab_object(cache, object, ip, init))
-		return true;
+	poison_slab_object(cache, object, init);
 
 	/*
 	 * If the object is put into quarantine, do not let slab put the object
 	 * onto the freelist for now. The object's metadata is kept until the
 	 * object gets evicted from quarantine.
 	 */
@@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip)
 		kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false);
 		return true;
 	}
 
 	if (is_kfence_address(ptr))
 		return false;
+	if (!kasan_arch_is_ready())
+		return true;
 
 	slab = folio_slab(folio);
-	return !poison_slab_object(slab->slab_cache, ptr, ip, false);
+
+	if (check_slab_allocation(slab->slab_cache, ptr, ip))
+		return false;
+
+	poison_slab_object(slab->slab_cache, ptr, false);
+	return true;
 }
 
 void __kasan_mempool_unpoison_object(void *ptr, size_t size, unsigned long ip)
 {
 	struct slab *slab;
 	gfp_t flags = 0; /* Might be executing under a lock. */
diff --git a/mm/slub.c b/mm/slub.c
index 3520acaf9afa..0c98b6a2124f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2223,12 +2223,19 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init)
 		__kcsan_check_access(x, s->object_size,
 				     KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT);
 
 	if (kfence_free(x))
 		return false;
 
+	/*
+	 * Give KASAN a chance to notice an invalid free operation before we
+	 * modify the object.
+	 */
+	if (kasan_slab_pre_free(s, x))
+		return false;
+
 	/*
 	 * As memory initialization might be integrated into KASAN,
 	 * kasan_slab_free and initialization memset's must be
 	 * kept together to avoid discrepancies in behavior.
 	 *
 	 * The initialization memset's clear the object and the metadata,

-- 
2.46.0.rc1.232.g9752f9e123-goog

[PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Jann Horn]

Currently, KASAN is unable to catch use-after-free in SLAB_TYPESAFE_BY_RCU
slabs because use-after-free is allowed within the RCU grace period by
design.

Add a SLUB debugging feature which RCU-delays every individual
kmem_cache_free() before either actually freeing the object or handing it
off to KASAN, and change KASAN to poison freed objects as normal when this
option is enabled.

For now I've configured Kconfig.debug to default-enable this feature in the
KASAN GENERIC and SW_TAGS modes; I'm not enabling it by default in HW_TAGS
mode because I'm not sure if it might have unwanted performance degradation
effects there.

Note that this is mostly useful with KASAN in the quarantine-based GENERIC
mode; SLAB_TYPESAFE_BY_RCU slabs are basically always also slabs with a
->ctor, and KASAN's assign_tag() currently has to assign fixed tags for
those, reducing the effectiveness of SW_TAGS/HW_TAGS mode.
(A possible future extension of this work would be to also let SLUB call
the ->ctor() on every allocation instead of only when the slab page is
allocated; then tag-based modes would be able to assign new tags on every
reallocation.)

Signed-off-by: Jann Horn <jannh@google.com>
---
 include/linux/kasan.h | 11 +++++---
 mm/Kconfig.debug      | 30 ++++++++++++++++++++
 mm/kasan/common.c     | 11 ++++----
 mm/kasan/kasan_test.c | 46 +++++++++++++++++++++++++++++++
 mm/slab_common.c      | 12 ++++++++
 mm/slub.c             | 76 +++++++++++++++++++++++++++++++++++++++++++++------
 6 files changed, 169 insertions(+), 17 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 34cb7a25aacb..0b952e11c7a0 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -194,28 +194,30 @@ static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s,
 {
 	if (kasan_enabled())
 		return __kasan_slab_pre_free(s, object, _RET_IP_);
 	return false;
 }
 
-bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init);
+bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init,
+		       bool after_rcu_delay);
 /**
  * kasan_slab_free - Possibly handle slab object freeing.
  * @object: Object to free.
  *
  * This hook is called from the slab allocator to give KASAN a chance to take
  * ownership of the object and handle its freeing.
  * kasan_slab_pre_free() must have already been called on the same object.
  *
  * @Return true if KASAN took ownership of the object; false otherwise.
  */
 static __always_inline bool kasan_slab_free(struct kmem_cache *s,
-						void *object, bool init)
+						void *object, bool init,
+						bool after_rcu_delay)
 {
 	if (kasan_enabled())
-		return __kasan_slab_free(s, object, init);
+		return __kasan_slab_free(s, object, init, after_rcu_delay);
 	return false;
 }
 
 void __kasan_kfree_large(void *ptr, unsigned long ip);
 static __always_inline void kasan_kfree_large(void *ptr)
 {
@@ -405,13 +407,14 @@ static inline void *kasan_init_slab_obj(struct kmem_cache *cache,
 
 static inline bool kasan_slab_pre_free(struct kmem_cache *s, void *object)
 {
 	return false;
 }
 
-static inline bool kasan_slab_free(struct kmem_cache *s, void *object, bool init)
+static inline bool kasan_slab_free(struct kmem_cache *s, void *object,
+				   bool init, bool after_rcu_delay)
 {
 	return false;
 }
 static inline void kasan_kfree_large(void *ptr) {}
 static inline void *kasan_slab_alloc(struct kmem_cache *s, void *object,
 				   gfp_t flags, bool init)
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index afc72fde0f03..8e440214aac8 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -67,12 +67,42 @@ config SLUB_DEBUG_ON
 	  equivalent to specifying the "slab_debug" parameter on boot.
 	  There is no support for more fine grained debug control like
 	  possible with slab_debug=xxx. SLUB debugging may be switched
 	  off in a kernel built with CONFIG_SLUB_DEBUG_ON by specifying
 	  "slab_debug=-".
 
+config SLUB_RCU_DEBUG
+	bool "Enable UAF detection in TYPESAFE_BY_RCU caches (for KASAN)"
+	depends on SLUB_DEBUG
+	depends on KASAN # not a real dependency; currently useless without KASAN
+	default KASAN_GENERIC || KASAN_SW_TAGS
+	help
+	  Make SLAB_TYPESAFE_BY_RCU caches behave approximately as if the cache
+	  was not marked as SLAB_TYPESAFE_BY_RCU and every caller used
+	  kfree_rcu() instead.
+
+	  This is intended for use in combination with KASAN, to enable KASAN to
+	  detect use-after-free accesses in such caches.
+	  (KFENCE is able to do that independent of this flag.)
+
+	  This might degrade performance.
+	  Unfortunately this also prevents a very specific bug pattern from
+	  triggering (insufficient checks against an object being recycled
+	  within the RCU grace period); so this option can be turned off even on
+	  KASAN builds, in case you want to test for such a bug.
+
+	  If you're using this for testing bugs / fuzzing and care about
+	  catching all the bugs WAY more than performance, you might want to
+	  also turn on CONFIG_RCU_STRICT_GRACE_PERIOD.
+
+	  WARNING:
+	  This is designed as a debugging feature, not a security feature.
+	  Objects are sometimes recycled without RCU delay under memory pressure.
+
+	  If unsure, say N.
+
 config PAGE_OWNER
 	bool "Track page owner"
 	depends on DEBUG_KERNEL && STACKTRACE_SUPPORT
 	select DEBUG_FS
 	select STACKTRACE
 	select STACKDEPOT
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 8cede1ce00e1..0769b23a9d5f 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -227,43 +227,44 @@ static bool check_slab_allocation(struct kmem_cache *cache, void *object,
 	}
 
 	return false;
 }
 
 static inline void poison_slab_object(struct kmem_cache *cache, void *object,
-				      bool init)
+				      bool init, bool after_rcu_delay)
 {
 	void *tagged_object = object;
 
 	object = kasan_reset_tag(object);
 
 	/* RCU slabs could be legally used after free within the RCU period. */
-	if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
+	if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay)
 		return;
 
 	kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_SIZE),
 			KASAN_SLAB_FREE, init);
 
 	if (kasan_stack_collection_enabled())
 		kasan_save_free_info(cache, tagged_object);
 }
 
 bool __kasan_slab_pre_free(struct kmem_cache *cache, void *object,
 				unsigned long ip)
 {
 	if (!kasan_arch_is_ready() || is_kfence_address(object))
 		return false;
 	return check_slab_allocation(cache, object, ip);
 }
 
-bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init)
+bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init,
+		       bool after_rcu_delay)
 {
 	if (!kasan_arch_is_ready() || is_kfence_address(object))
 		return false;
 
-	poison_slab_object(cache, object, init);
+	poison_slab_object(cache, object, init, after_rcu_delay);
 
 	/*
 	 * If the object is put into quarantine, do not let slab put the object
 	 * onto the freelist for now. The object's metadata is kept until the
 	 * object gets evicted from quarantine.
 	 */
@@ -517,13 +518,13 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip)
 
 	slab = folio_slab(folio);
 
 	if (check_slab_allocation(slab->slab_cache, ptr, ip))
 		return false;
 
-	poison_slab_object(slab->slab_cache, ptr, false);
+	poison_slab_object(slab->slab_cache, ptr, false, false);
 	return true;
 }
 
 void __kasan_mempool_unpoison_object(void *ptr, size_t size, unsigned long ip)
 {
 	struct slab *slab;
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index 7b32be2a3cf0..567d33b493e2 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -993,12 +993,57 @@ static void kmem_cache_invalid_free(struct kunit *test)
 	 */
 	kmem_cache_free(cache, p);
 
 	kmem_cache_destroy(cache);
 }
 
+static void kmem_cache_rcu_uaf(struct kunit *test)
+{
+	char *p;
+	size_t size = 200;
+	struct kmem_cache *cache;
+
+	KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_SLUB_RCU_DEBUG);
+
+	cache = kmem_cache_create("test_cache", size, 0, SLAB_TYPESAFE_BY_RCU,
+				  NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cache);
+
+	p = kmem_cache_alloc(cache, GFP_KERNEL);
+	if (!p) {
+		kunit_err(test, "Allocation failed: %s\n", __func__);
+		kmem_cache_destroy(cache);
+		return;
+	}
+	*p = 1;
+
+	rcu_read_lock();
+
+	/* Free the object - this will internally schedule an RCU callback. */
+	kmem_cache_free(cache, p);
+
+	/*
+	 * We should still be allowed to access the object at this point because
+	 * the cache is SLAB_TYPESAFE_BY_RCU and we've been in an RCU read-side
+	 * critical section since before the kmem_cache_free().
+	 */
+	READ_ONCE(*p);
+
+	rcu_read_unlock();
+
+	/*
+	 * Wait for the RCU callback to execute; after this, the object should
+	 * have actually been freed from KASAN's perspective.
+	 */
+	rcu_barrier();
+
+	KUNIT_EXPECT_KASAN_FAIL(test, READ_ONCE(*p));
+
+	kmem_cache_destroy(cache);
+}
+
 static void empty_cache_ctor(void *object) { }
 
 static void kmem_cache_double_destroy(struct kunit *test)
 {
 	struct kmem_cache *cache;
 
@@ -1934,12 +1979,13 @@ static struct kunit_case kasan_kunit_test_cases[] = {
 	KUNIT_CASE(workqueue_uaf),
 	KUNIT_CASE(kfree_via_page),
 	KUNIT_CASE(kfree_via_phys),
 	KUNIT_CASE(kmem_cache_oob),
 	KUNIT_CASE(kmem_cache_double_free),
 	KUNIT_CASE(kmem_cache_invalid_free),
+	KUNIT_CASE(kmem_cache_rcu_uaf),
 	KUNIT_CASE(kmem_cache_double_destroy),
 	KUNIT_CASE(kmem_cache_accounted),
 	KUNIT_CASE(kmem_cache_bulk),
 	KUNIT_CASE(mempool_kmalloc_oob_right),
 	KUNIT_CASE(mempool_kmalloc_large_oob_right),
 	KUNIT_CASE(mempool_slab_oob_right),
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 40b582a014b8..df09066d56fe 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -539,12 +539,24 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work)
 		kmem_cache_release(s);
 	}
 }
 
 static int shutdown_cache(struct kmem_cache *s)
 {
+	if (IS_ENABLED(CONFIG_SLUB_RCU_DEBUG) &&
+	    (s->flags & SLAB_TYPESAFE_BY_RCU)) {
+		/*
+		 * Under CONFIG_SLUB_RCU_DEBUG, when objects in a
+		 * SLAB_TYPESAFE_BY_RCU slab are freed, SLUB will internally
+		 * defer their freeing with call_rcu().
+		 * Wait for such call_rcu() invocations here before actually
+		 * destroying the cache.
+		 */
+		rcu_barrier();
+	}
+
 	/* free asan quarantined objects */
 	kasan_cache_shutdown(s);
 
 	if (__kmem_cache_shutdown(s) != 0)
 		return -EBUSY;
 
diff --git a/mm/slub.c b/mm/slub.c
index 0c98b6a2124f..f0d0e3c30837 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2197,45 +2197,78 @@ static inline bool memcg_slab_post_alloc_hook(struct kmem_cache *s,
 static inline void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
 					void **p, int objects)
 {
 }
 #endif /* CONFIG_MEMCG */
 
+#ifdef CONFIG_SLUB_RCU_DEBUG
+static void slab_free_after_rcu_debug(struct rcu_head *rcu_head);
+
+struct rcu_delayed_free {
+	struct rcu_head head;
+	void *object;
+};
+#endif
+
 /*
  * Hooks for other subsystems that check memory allocations. In a typical
  * production configuration these hooks all should produce no code at all.
  *
  * Returns true if freeing of the object can proceed, false if its reuse
- * was delayed by KASAN quarantine, or it was returned to KFENCE.
+ * was delayed by CONFIG_SLUB_RCU_DEBUG or KASAN quarantine, or it was returned
+ * to KFENCE.
  */
 static __always_inline
-bool slab_free_hook(struct kmem_cache *s, void *x, bool init)
+bool slab_free_hook(struct kmem_cache *s, void *x, bool init,
+		    bool after_rcu_delay)
 {
 	kmemleak_free_recursive(x, s->flags);
 	kmsan_slab_free(s, x);
 
 	debug_check_no_locks_freed(x, s->object_size);
 
 	if (!(s->flags & SLAB_DEBUG_OBJECTS))
 		debug_check_no_obj_freed(x, s->object_size);
 
 	/* Use KCSAN to help debug racy use-after-free. */
-	if (!(s->flags & SLAB_TYPESAFE_BY_RCU))
+	if (!(s->flags & SLAB_TYPESAFE_BY_RCU) || after_rcu_delay)
 		__kcsan_check_access(x, s->object_size,
 				     KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT);
 
 	if (kfence_free(x))
 		return false;
 
 	/*
 	 * Give KASAN a chance to notice an invalid free operation before we
 	 * modify the object.
 	 */
 	if (kasan_slab_pre_free(s, x))
 		return false;
 
+#ifdef CONFIG_SLUB_RCU_DEBUG
+	if ((s->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay) {
+		struct rcu_delayed_free *delayed_free;
+
+		delayed_free = kmalloc(sizeof(*delayed_free), GFP_NOWAIT);
+		if (delayed_free) {
+			/*
+			 * Let KASAN track our call stack as a "related work
+			 * creation", just like if the object had been freed
+			 * normally via kfree_rcu().
+			 * We have to do this manually because the rcu_head is
+			 * not located inside the object.
+			 */
+			kasan_record_aux_stack_noalloc(x);
+
+			delayed_free->object = x;
+			call_rcu(&delayed_free->head, slab_free_after_rcu_debug);
+			return false;
+		}
+	}
+#endif /* CONFIG_SLUB_RCU_DEBUG */
+
 	/*
 	 * As memory initialization might be integrated into KASAN,
 	 * kasan_slab_free and initialization memset's must be
 	 * kept together to avoid discrepancies in behavior.
 	 *
 	 * The initialization memset's clear the object and the metadata,
@@ -2253,42 +2286,42 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init)
 			memset(kasan_reset_tag(x), 0, s->object_size);
 		rsize = (s->flags & SLAB_RED_ZONE) ? s->red_left_pad : 0;
 		memset((char *)kasan_reset_tag(x) + inuse, 0,
 		       s->size - inuse - rsize);
 	}
 	/* KASAN might put x into memory quarantine, delaying its reuse. */
-	return !kasan_slab_free(s, x, init);
+	return !kasan_slab_free(s, x, init, after_rcu_delay);
 }
 
 static __fastpath_inline
 bool slab_free_freelist_hook(struct kmem_cache *s, void **head, void **tail,
 			     int *cnt)
 {
 
 	void *object;
 	void *next = *head;
 	void *old_tail = *tail;
 	bool init;
 
 	if (is_kfence_address(next)) {
-		slab_free_hook(s, next, false);
+		slab_free_hook(s, next, false, false);
 		return false;
 	}
 
 	/* Head and tail of the reconstructed freelist */
 	*head = NULL;
 	*tail = NULL;
 
 	init = slab_want_init_on_free(s);
 
 	do {
 		object = next;
 		next = get_freepointer(s, object);
 
 		/* If object's reuse doesn't have to be delayed */
-		if (likely(slab_free_hook(s, object, init))) {
+		if (likely(slab_free_hook(s, object, init, false))) {
 			/* Move object to the new freelist */
 			set_freepointer(s, object, *head);
 			*head = object;
 			if (!*tail)
 				*tail = object;
 		} else {
@@ -4474,40 +4507,67 @@ static __fastpath_inline
 void slab_free(struct kmem_cache *s, struct slab *slab, void *object,
 	       unsigned long addr)
 {
 	memcg_slab_free_hook(s, slab, &object, 1);
 	alloc_tagging_slab_free_hook(s, slab, &object, 1);
 
-	if (likely(slab_free_hook(s, object, slab_want_init_on_free(s))))
+	if (likely(slab_free_hook(s, object, slab_want_init_on_free(s), false)))
 		do_slab_free(s, slab, object, object, 1, addr);
 }
 
 #ifdef CONFIG_MEMCG
 /* Do not inline the rare memcg charging failed path into the allocation path */
 static noinline
 void memcg_alloc_abort_single(struct kmem_cache *s, void *object)
 {
-	if (likely(slab_free_hook(s, object, slab_want_init_on_free(s))))
+	if (likely(slab_free_hook(s, object, slab_want_init_on_free(s), false)))
 		do_slab_free(s, virt_to_slab(object), object, object, 1, _RET_IP_);
 }
 #endif
 
 static __fastpath_inline
 void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head,
 		    void *tail, void **p, int cnt, unsigned long addr)
 {
 	memcg_slab_free_hook(s, slab, p, cnt);
 	alloc_tagging_slab_free_hook(s, slab, p, cnt);
 	/*
 	 * With KASAN enabled slab_free_freelist_hook modifies the freelist
 	 * to remove objects, whose reuse must be delayed.
 	 */
 	if (likely(slab_free_freelist_hook(s, &head, &tail, &cnt)))
 		do_slab_free(s, slab, head, tail, cnt, addr);
 }
 
+#ifdef CONFIG_SLUB_RCU_DEBUG
+static void slab_free_after_rcu_debug(struct rcu_head *rcu_head)
+{
+	struct rcu_delayed_free *delayed_free =
+			container_of(rcu_head, struct rcu_delayed_free, head);
+	void *object = delayed_free->object;
+	struct slab *slab = virt_to_slab(object);
+	struct kmem_cache *s;
+
+	if (WARN_ON(is_kfence_address(rcu_head)))
+		return;
+
+	/* find the object and the cache again */
+	if (WARN_ON(!slab))
+		return;
+	s = slab->slab_cache;
+	if (WARN_ON(!(s->flags & SLAB_TYPESAFE_BY_RCU)))
+		return;
+
+	/* resume freeing */
+	if (!slab_free_hook(s, object, slab_want_init_on_free(s), true))
+		return;
+	do_slab_free(s, slab, object, object, 1, _THIS_IP_);
+	kfree(delayed_free);
+}
+#endif /* CONFIG_SLUB_RCU_DEBUG */
+
 #ifdef CONFIG_KASAN_GENERIC
 void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
 {
 	do_slab_free(cache, virt_to_slab(x), x, x, 1, addr);
 }
 #endif

-- 
2.46.0.rc1.232.g9752f9e123-goog

Re: [PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Vlastimil Babka]

On 7/30/24 1:06 PM, Jann Horn wrote:
> Currently, KASAN is unable to catch use-after-free in SLAB_TYPESAFE_BY_RCU
> slabs because use-after-free is allowed within the RCU grace period by
> design.
> 
> Add a SLUB debugging feature which RCU-delays every individual
> kmem_cache_free() before either actually freeing the object or handing it
> off to KASAN, and change KASAN to poison freed objects as normal when this
> option is enabled.
> 
> For now I've configured Kconfig.debug to default-enable this feature in the
> KASAN GENERIC and SW_TAGS modes; I'm not enabling it by default in HW_TAGS
> mode because I'm not sure if it might have unwanted performance degradation
> effects there.
> 
> Note that this is mostly useful with KASAN in the quarantine-based GENERIC
> mode; SLAB_TYPESAFE_BY_RCU slabs are basically always also slabs with a
> ->ctor, and KASAN's assign_tag() currently has to assign fixed tags for
> those, reducing the effectiveness of SW_TAGS/HW_TAGS mode.
> (A possible future extension of this work would be to also let SLUB call
> the ->ctor() on every allocation instead of only when the slab page is
> allocated; then tag-based modes would be able to assign new tags on every
> reallocation.)
> 
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz> #slab

Re: [PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Andrey Konovalov]

On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.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 RCU-delay the actual
> SLUB freeing of an object, and we'd like KASAN to still validate
> synchronously that freeing the object is permitted. (Otherwise this
> change will make the existing testcase kmem_cache_invalid_free fail.)
>
> So add a new KASAN hook that allows KASAN to pre-validate a
> kmem_cache_free() operation before SLUB actually starts modifying the
> object or its metadata.

A few more minor comments below. With that:

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>

Thank you!

> Inside KASAN, this:
>
>  - moves checks from poison_slab_object() into check_slab_free()
>  - moves kasan_arch_is_ready() up into callers of poison_slab_object()
>  - removes "ip" argument of poison_slab_object() and __kasan_slab_free()
>    (since those functions no longer do any reporting)

>  - renames check_slab_free() to check_slab_allocation()

check_slab_allocation() is introduced in this patch, so technically
you don't rename anything.

> Acked-by: Vlastimil Babka <vbabka@suse.cz> #slub
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  include/linux/kasan.h | 43 ++++++++++++++++++++++++++++++++++---
>  mm/kasan/common.c     | 59 +++++++++++++++++++++++++++++++--------------------
>  mm/slub.c             |  7 ++++++
>  3 files changed, 83 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 70d6a8f6e25d..34cb7a25aacb 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -172,19 +172,50 @@ static __always_inline void * __must_check kasan_init_slab_obj(
>  {
>         if (kasan_enabled())
>                 return __kasan_init_slab_obj(cache, object);
>         return (void *)object;
>  }
>
> -bool __kasan_slab_free(struct kmem_cache *s, void *object,
> -                       unsigned long ip, bool init);
> +bool __kasan_slab_pre_free(struct kmem_cache *s, void *object,
> +                       unsigned long ip);
> +/**
> + * kasan_slab_pre_free - Validate a slab object freeing request.
> + * @object: Object to free.
> + *
> + * This function checks whether freeing the given object might be permitted; it
> + * checks things like whether the given object is properly aligned and not
> + * already freed.
> + *
> + * This function is only intended for use by the slab allocator.
> + *
> + * @Return true if freeing the object is known to be invalid; false otherwise.
> + */

Let's reword this to:

kasan_slab_pre_free - Check whether freeing a slab object is safe.
@object: Object to be freed.

This function checks whether freeing the given object is safe. It
performs checks to detect double-free and invalid-free bugs and
reports them.

This function is intended only for use by the slab allocator.

@Return true if freeing the object is not safe; false otherwise.

> +static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s,
> +                                               void *object)
> +{
> +       if (kasan_enabled())
> +               return __kasan_slab_pre_free(s, object, _RET_IP_);
> +       return false;
> +}
> +
> +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init);
> +/**
> + * kasan_slab_free - Possibly handle slab object freeing.
> + * @object: Object to free.
> + *
> + * This hook is called from the slab allocator to give KASAN a chance to take
> + * ownership of the object and handle its freeing.
> + * kasan_slab_pre_free() must have already been called on the same object.
> + *
> + * @Return true if KASAN took ownership of the object; false otherwise.
> + */

kasan_slab_free - Poison, initialize, and quarantine a slab object.
@object: Object to be freed.
@init: Whether to initialize the object.

This function poisons a slab object and saves a free stack trace for
it, except for SLAB_TYPESAFE_BY_RCU caches.

For KASAN modes that have integrated memory initialization
(kasan_has_integrated_init() == true), this function also initializes
the object's memory. For other modes, the @init argument is ignored.

For the Generic mode, this function might also quarantine the object.
When this happens, KASAN will defer freeing the object to a later
stage and handle it internally then. The return value indicates
whether the object was quarantined.

This function is intended only for use by the slab allocator.

@Return true if KASAN quarantined the object; false otherwise.

>  static __always_inline bool kasan_slab_free(struct kmem_cache *s,
>                                                 void *object, bool init)
>  {
>         if (kasan_enabled())
> -               return __kasan_slab_free(s, object, _RET_IP_, init);
> +               return __kasan_slab_free(s, object, init);
>         return false;
>  }
>
>  void __kasan_kfree_large(void *ptr, unsigned long ip);
>  static __always_inline void kasan_kfree_large(void *ptr)
>  {
> @@ -368,12 +399,18 @@ static inline void kasan_poison_new_object(struct kmem_cache *cache,
>                                         void *object) {}
>  static inline void *kasan_init_slab_obj(struct kmem_cache *cache,
>                                 const void *object)
>  {
>         return (void *)object;
>  }
> +
> +static inline bool kasan_slab_pre_free(struct kmem_cache *s, void *object)
> +{
> +       return false;
> +}
> +
>  static inline bool kasan_slab_free(struct kmem_cache *s, void *object, bool init)
>  {
>         return false;
>  }
>  static inline void kasan_kfree_large(void *ptr) {}
>  static inline void *kasan_slab_alloc(struct kmem_cache *s, void *object,
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 85e7c6b4575c..8cede1ce00e1 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -205,59 +205,65 @@ void * __must_check __kasan_init_slab_obj(struct kmem_cache *cache,
>         /* Tag is ignored in set_tag() without CONFIG_KASAN_SW/HW_TAGS */
>         object = set_tag(object, assign_tag(cache, object, true));
>
>         return (void *)object;
>  }
>
> -static inline bool poison_slab_object(struct kmem_cache *cache, void *object,
> -                                     unsigned long ip, bool init)
> +/* returns true for invalid request */

"Returns true when freeing the object is not safe."

> +static bool check_slab_allocation(struct kmem_cache *cache, void *object,
> +                                 unsigned long ip)
>  {
> -       void *tagged_object;
> -
> -       if (!kasan_arch_is_ready())
> -               return false;
> +       void *tagged_object = object;
>
> -       tagged_object = object;
>         object = kasan_reset_tag(object);
>
>         if (unlikely(nearest_obj(cache, virt_to_slab(object), object) != object)) {
>                 kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT_INVALID_FREE);
>                 return true;
>         }
>
> -       /* RCU slabs could be legally used after free within the RCU period. */
> -       if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
> -               return false;
> -
>         if (!kasan_byte_accessible(tagged_object)) {
>                 kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT_DOUBLE_FREE);
>                 return true;
>         }
>
> +       return false;
> +}
> +
> +static inline void poison_slab_object(struct kmem_cache *cache, void *object,
> +                                     bool init)
> +{
> +       void *tagged_object = object;
> +
> +       object = kasan_reset_tag(object);
> +
> +       /* RCU slabs could be legally used after free within the RCU period. */
> +       if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
> +               return;
> +
>         kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_SIZE),
>                         KASAN_SLAB_FREE, init);
>
>         if (kasan_stack_collection_enabled())
>                 kasan_save_free_info(cache, tagged_object);
> +}
>
> -       return false;
> +bool __kasan_slab_pre_free(struct kmem_cache *cache, void *object,
> +                               unsigned long ip)
> +{
> +       if (!kasan_arch_is_ready() || is_kfence_address(object))
> +               return false;
> +       return check_slab_allocation(cache, object, ip);
>  }
>
> -bool __kasan_slab_free(struct kmem_cache *cache, void *object,
> -                               unsigned long ip, bool init)
> +bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init)
>  {
> -       if (is_kfence_address(object))
> +       if (!kasan_arch_is_ready() || is_kfence_address(object))
>                 return false;
>
> -       /*
> -        * If the object is buggy, do not let slab put the object onto the
> -        * freelist. The object will thus never be allocated again and its
> -        * metadata will never get released.
> -        */
> -       if (poison_slab_object(cache, object, ip, init))
> -               return true;
> +       poison_slab_object(cache, object, init);
>
>         /*
>          * If the object is put into quarantine, do not let slab put the object
>          * onto the freelist for now. The object's metadata is kept until the
>          * object gets evicted from quarantine.
>          */
> @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip)
>                 kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false);
>                 return true;
>         }
>
>         if (is_kfence_address(ptr))
>                 return false;
> +       if (!kasan_arch_is_ready())
> +               return true;

Hm, I think we had a bug here: the function should return true in both
cases. This seems reasonable: if KASAN is not checking the object, the
caller can do whatever they want with it.





>         slab = folio_slab(folio);
> -       return !poison_slab_object(slab->slab_cache, ptr, ip, false);
> +
> +       if (check_slab_allocation(slab->slab_cache, ptr, ip))
> +               return false;
> +
> +       poison_slab_object(slab->slab_cache, ptr, false);
> +       return true;
>  }
>
>  void __kasan_mempool_unpoison_object(void *ptr, size_t size, unsigned long ip)
>  {
>         struct slab *slab;
>         gfp_t flags = 0; /* Might be executing under a lock. */
> diff --git a/mm/slub.c b/mm/slub.c
> index 3520acaf9afa..0c98b6a2124f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2223,12 +2223,19 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init)
>                 __kcsan_check_access(x, s->object_size,
>                                      KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT);
>
>         if (kfence_free(x))
>                 return false;
>
> +       /*
> +        * Give KASAN a chance to notice an invalid free operation before we
> +        * modify the object.
> +        */
> +       if (kasan_slab_pre_free(s, x))
> +               return false;
> +
>         /*
>          * As memory initialization might be integrated into KASAN,
>          * kasan_slab_free and initialization memset's must be
>          * kept together to avoid discrepancies in behavior.
>          *
>          * The initialization memset's clear the object and the metadata,
>
> --
> 2.46.0.rc1.232.g9752f9e123-goog
>

Re: [PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Andrey Konovalov]

On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.com> wrote:
>
> Currently, KASAN is unable to catch use-after-free in SLAB_TYPESAFE_BY_RCU
> slabs because use-after-free is allowed within the RCU grace period by
> design.
>
> Add a SLUB debugging feature which RCU-delays every individual
> kmem_cache_free() before either actually freeing the object or handing it
> off to KASAN, and change KASAN to poison freed objects as normal when this
> option is enabled.
>
> For now I've configured Kconfig.debug to default-enable this feature in the
> KASAN GENERIC and SW_TAGS modes; I'm not enabling it by default in HW_TAGS
> mode because I'm not sure if it might have unwanted performance degradation
> effects there.
>
> Note that this is mostly useful with KASAN in the quarantine-based GENERIC
> mode; SLAB_TYPESAFE_BY_RCU slabs are basically always also slabs with a
> ->ctor, and KASAN's assign_tag() currently has to assign fixed tags for
> those, reducing the effectiveness of SW_TAGS/HW_TAGS mode.
> (A possible future extension of this work would be to also let SLUB call
> the ->ctor() on every allocation instead of only when the slab page is
> allocated; then tag-based modes would be able to assign new tags on every
> reallocation.)
>
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Andrey Konovalov <andreyknvl@gmail.com>

But see a comment below.

> ---
>  include/linux/kasan.h | 11 +++++---
>  mm/Kconfig.debug      | 30 ++++++++++++++++++++
>  mm/kasan/common.c     | 11 ++++----
>  mm/kasan/kasan_test.c | 46 +++++++++++++++++++++++++++++++
>  mm/slab_common.c      | 12 ++++++++
>  mm/slub.c             | 76 +++++++++++++++++++++++++++++++++++++++++++++------
>  6 files changed, 169 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 34cb7a25aacb..0b952e11c7a0 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -194,28 +194,30 @@ static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s,
>  {
>         if (kasan_enabled())
>                 return __kasan_slab_pre_free(s, object, _RET_IP_);
>         return false;
>  }
>
> -bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init);
> +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init,
> +                      bool after_rcu_delay);

What do you think about renaming this argument to poison_rcu? I think
it makes the intention more clear from the KASAN's point of view.

>  /**
>   * kasan_slab_free - Possibly handle slab object freeing.
>   * @object: Object to free.

@poison_rcu - Whether to skip poisoning for SLAB_TYPESAFE_BY_RCU caches.

And also update the reworded comment from the previous patch:

This function poisons a slab object and saves a free stack trace for
it, except for SLAB_TYPESAFE_BY_RCU caches when @poison_rcu is false.



>   *
>   * This hook is called from the slab allocator to give KASAN a chance to take
>   * ownership of the object and handle its freeing.
>   * kasan_slab_pre_free() must have already been called on the same object.
>   *
>   * @Return true if KASAN took ownership of the object; false otherwise.
>   */
>  static __always_inline bool kasan_slab_free(struct kmem_cache *s,
> -                                               void *object, bool init)
> +                                               void *object, bool init,
> +                                               bool after_rcu_delay)
>  {
>         if (kasan_enabled())
> -               return __kasan_slab_free(s, object, init);
> +               return __kasan_slab_free(s, object, init, after_rcu_delay);
>         return false;
>  }
>
>  void __kasan_kfree_large(void *ptr, unsigned long ip);
>  static __always_inline void kasan_kfree_large(void *ptr)
>  {
> @@ -405,13 +407,14 @@ static inline void *kasan_init_slab_obj(struct kmem_cache *cache,
>
>  static inline bool kasan_slab_pre_free(struct kmem_cache *s, void *object)
>  {
>         return false;
>  }
>
> -static inline bool kasan_slab_free(struct kmem_cache *s, void *object, bool init)
> +static inline bool kasan_slab_free(struct kmem_cache *s, void *object,
> +                                  bool init, bool after_rcu_delay)
>  {
>         return false;
>  }
>  static inline void kasan_kfree_large(void *ptr) {}
>  static inline void *kasan_slab_alloc(struct kmem_cache *s, void *object,
>                                    gfp_t flags, bool init)
> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> index afc72fde0f03..8e440214aac8 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -67,12 +67,42 @@ config SLUB_DEBUG_ON
>           equivalent to specifying the "slab_debug" parameter on boot.
>           There is no support for more fine grained debug control like
>           possible with slab_debug=xxx. SLUB debugging may be switched
>           off in a kernel built with CONFIG_SLUB_DEBUG_ON by specifying
>           "slab_debug=-".
>
> +config SLUB_RCU_DEBUG
> +       bool "Enable UAF detection in TYPESAFE_BY_RCU caches (for KASAN)"
> +       depends on SLUB_DEBUG
> +       depends on KASAN # not a real dependency; currently useless without KASAN
> +       default KASAN_GENERIC || KASAN_SW_TAGS
> +       help
> +         Make SLAB_TYPESAFE_BY_RCU caches behave approximately as if the cache
> +         was not marked as SLAB_TYPESAFE_BY_RCU and every caller used
> +         kfree_rcu() instead.
> +
> +         This is intended for use in combination with KASAN, to enable KASAN to
> +         detect use-after-free accesses in such caches.
> +         (KFENCE is able to do that independent of this flag.)
> +
> +         This might degrade performance.
> +         Unfortunately this also prevents a very specific bug pattern from
> +         triggering (insufficient checks against an object being recycled
> +         within the RCU grace period); so this option can be turned off even on
> +         KASAN builds, in case you want to test for such a bug.
> +
> +         If you're using this for testing bugs / fuzzing and care about
> +         catching all the bugs WAY more than performance, you might want to
> +         also turn on CONFIG_RCU_STRICT_GRACE_PERIOD.
> +
> +         WARNING:
> +         This is designed as a debugging feature, not a security feature.
> +         Objects are sometimes recycled without RCU delay under memory pressure.
> +
> +         If unsure, say N.
> +
>  config PAGE_OWNER
>         bool "Track page owner"
>         depends on DEBUG_KERNEL && STACKTRACE_SUPPORT
>         select DEBUG_FS
>         select STACKTRACE
>         select STACKDEPOT
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 8cede1ce00e1..0769b23a9d5f 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -227,43 +227,44 @@ static bool check_slab_allocation(struct kmem_cache *cache, void *object,
>         }
>
>         return false;
>  }
>
>  static inline void poison_slab_object(struct kmem_cache *cache, void *object,
> -                                     bool init)
> +                                     bool init, bool after_rcu_delay)
>  {
>         void *tagged_object = object;
>
>         object = kasan_reset_tag(object);
>
>         /* RCU slabs could be legally used after free within the RCU period. */
> -       if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
> +       if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay)
>                 return;
>
>         kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_SIZE),
>                         KASAN_SLAB_FREE, init);
>
>         if (kasan_stack_collection_enabled())
>                 kasan_save_free_info(cache, tagged_object);
>  }
>
>  bool __kasan_slab_pre_free(struct kmem_cache *cache, void *object,
>                                 unsigned long ip)
>  {
>         if (!kasan_arch_is_ready() || is_kfence_address(object))
>                 return false;
>         return check_slab_allocation(cache, object, ip);
>  }
>
> -bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init)
> +bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init,
> +                      bool after_rcu_delay)
>  {
>         if (!kasan_arch_is_ready() || is_kfence_address(object))
>                 return false;
>
> -       poison_slab_object(cache, object, init);
> +       poison_slab_object(cache, object, init, after_rcu_delay);
>
>         /*
>          * If the object is put into quarantine, do not let slab put the object
>          * onto the freelist for now. The object's metadata is kept until the
>          * object gets evicted from quarantine.
>          */
> @@ -517,13 +518,13 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip)
>
>         slab = folio_slab(folio);
>
>         if (check_slab_allocation(slab->slab_cache, ptr, ip))
>                 return false;
>
> -       poison_slab_object(slab->slab_cache, ptr, false);
> +       poison_slab_object(slab->slab_cache, ptr, false, false);
>         return true;
>  }
>
>  void __kasan_mempool_unpoison_object(void *ptr, size_t size, unsigned long ip)
>  {
>         struct slab *slab;
> diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
> index 7b32be2a3cf0..567d33b493e2 100644
> --- a/mm/kasan/kasan_test.c
> +++ b/mm/kasan/kasan_test.c
> @@ -993,12 +993,57 @@ static void kmem_cache_invalid_free(struct kunit *test)
>          */
>         kmem_cache_free(cache, p);
>
>         kmem_cache_destroy(cache);
>  }
>
> +static void kmem_cache_rcu_uaf(struct kunit *test)
> +{
> +       char *p;
> +       size_t size = 200;
> +       struct kmem_cache *cache;
> +
> +       KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_SLUB_RCU_DEBUG);
> +
> +       cache = kmem_cache_create("test_cache", size, 0, SLAB_TYPESAFE_BY_RCU,
> +                                 NULL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cache);
> +
> +       p = kmem_cache_alloc(cache, GFP_KERNEL);
> +       if (!p) {
> +               kunit_err(test, "Allocation failed: %s\n", __func__);
> +               kmem_cache_destroy(cache);
> +               return;
> +       }
> +       *p = 1;
> +
> +       rcu_read_lock();
> +
> +       /* Free the object - this will internally schedule an RCU callback. */
> +       kmem_cache_free(cache, p);
> +
> +       /*
> +        * We should still be allowed to access the object at this point because
> +        * the cache is SLAB_TYPESAFE_BY_RCU and we've been in an RCU read-side
> +        * critical section since before the kmem_cache_free().
> +        */
> +       READ_ONCE(*p);
> +
> +       rcu_read_unlock();
> +
> +       /*
> +        * Wait for the RCU callback to execute; after this, the object should
> +        * have actually been freed from KASAN's perspective.
> +        */
> +       rcu_barrier();
> +
> +       KUNIT_EXPECT_KASAN_FAIL(test, READ_ONCE(*p));
> +
> +       kmem_cache_destroy(cache);
> +}
> +
>  static void empty_cache_ctor(void *object) { }
>
>  static void kmem_cache_double_destroy(struct kunit *test)
>  {
>         struct kmem_cache *cache;
>
> @@ -1934,12 +1979,13 @@ static struct kunit_case kasan_kunit_test_cases[] = {
>         KUNIT_CASE(workqueue_uaf),
>         KUNIT_CASE(kfree_via_page),
>         KUNIT_CASE(kfree_via_phys),
>         KUNIT_CASE(kmem_cache_oob),
>         KUNIT_CASE(kmem_cache_double_free),
>         KUNIT_CASE(kmem_cache_invalid_free),
> +       KUNIT_CASE(kmem_cache_rcu_uaf),
>         KUNIT_CASE(kmem_cache_double_destroy),
>         KUNIT_CASE(kmem_cache_accounted),
>         KUNIT_CASE(kmem_cache_bulk),
>         KUNIT_CASE(mempool_kmalloc_oob_right),
>         KUNIT_CASE(mempool_kmalloc_large_oob_right),
>         KUNIT_CASE(mempool_slab_oob_right),
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 40b582a014b8..df09066d56fe 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -539,12 +539,24 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work)
>                 kmem_cache_release(s);
>         }
>  }
>
>  static int shutdown_cache(struct kmem_cache *s)
>  {
> +       if (IS_ENABLED(CONFIG_SLUB_RCU_DEBUG) &&
> +           (s->flags & SLAB_TYPESAFE_BY_RCU)) {
> +               /*
> +                * Under CONFIG_SLUB_RCU_DEBUG, when objects in a
> +                * SLAB_TYPESAFE_BY_RCU slab are freed, SLUB will internally
> +                * defer their freeing with call_rcu().
> +                * Wait for such call_rcu() invocations here before actually
> +                * destroying the cache.
> +                */
> +               rcu_barrier();
> +       }
> +
>         /* free asan quarantined objects */
>         kasan_cache_shutdown(s);
>
>         if (__kmem_cache_shutdown(s) != 0)
>                 return -EBUSY;
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 0c98b6a2124f..f0d0e3c30837 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2197,45 +2197,78 @@ static inline bool memcg_slab_post_alloc_hook(struct kmem_cache *s,
>  static inline void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
>                                         void **p, int objects)
>  {
>  }
>  #endif /* CONFIG_MEMCG */
>
> +#ifdef CONFIG_SLUB_RCU_DEBUG
> +static void slab_free_after_rcu_debug(struct rcu_head *rcu_head);
> +
> +struct rcu_delayed_free {
> +       struct rcu_head head;
> +       void *object;
> +};
> +#endif
> +
>  /*
>   * Hooks for other subsystems that check memory allocations. In a typical
>   * production configuration these hooks all should produce no code at all.
>   *
>   * Returns true if freeing of the object can proceed, false if its reuse
> - * was delayed by KASAN quarantine, or it was returned to KFENCE.
> + * was delayed by CONFIG_SLUB_RCU_DEBUG or KASAN quarantine, or it was returned
> + * to KFENCE.
>   */
>  static __always_inline
> -bool slab_free_hook(struct kmem_cache *s, void *x, bool init)
> +bool slab_free_hook(struct kmem_cache *s, void *x, bool init,
> +                   bool after_rcu_delay)
>  {
>         kmemleak_free_recursive(x, s->flags);
>         kmsan_slab_free(s, x);
>
>         debug_check_no_locks_freed(x, s->object_size);
>
>         if (!(s->flags & SLAB_DEBUG_OBJECTS))
>                 debug_check_no_obj_freed(x, s->object_size);
>
>         /* Use KCSAN to help debug racy use-after-free. */
> -       if (!(s->flags & SLAB_TYPESAFE_BY_RCU))
> +       if (!(s->flags & SLAB_TYPESAFE_BY_RCU) || after_rcu_delay)
>                 __kcsan_check_access(x, s->object_size,
>                                      KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT);
>
>         if (kfence_free(x))
>                 return false;
>
>         /*
>          * Give KASAN a chance to notice an invalid free operation before we
>          * modify the object.
>          */
>         if (kasan_slab_pre_free(s, x))
>                 return false;
>
> +#ifdef CONFIG_SLUB_RCU_DEBUG
> +       if ((s->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay) {
> +               struct rcu_delayed_free *delayed_free;
> +
> +               delayed_free = kmalloc(sizeof(*delayed_free), GFP_NOWAIT);
> +               if (delayed_free) {
> +                       /*
> +                        * Let KASAN track our call stack as a "related work
> +                        * creation", just like if the object had been freed
> +                        * normally via kfree_rcu().
> +                        * We have to do this manually because the rcu_head is
> +                        * not located inside the object.
> +                        */
> +                       kasan_record_aux_stack_noalloc(x);
> +
> +                       delayed_free->object = x;
> +                       call_rcu(&delayed_free->head, slab_free_after_rcu_debug);
> +                       return false;
> +               }
> +       }
> +#endif /* CONFIG_SLUB_RCU_DEBUG */
> +
>         /*
>          * As memory initialization might be integrated into KASAN,
>          * kasan_slab_free and initialization memset's must be
>          * kept together to avoid discrepancies in behavior.
>          *
>          * The initialization memset's clear the object and the metadata,
> @@ -2253,42 +2286,42 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init)
>                         memset(kasan_reset_tag(x), 0, s->object_size);
>                 rsize = (s->flags & SLAB_RED_ZONE) ? s->red_left_pad : 0;
>                 memset((char *)kasan_reset_tag(x) + inuse, 0,
>                        s->size - inuse - rsize);
>         }
>         /* KASAN might put x into memory quarantine, delaying its reuse. */
> -       return !kasan_slab_free(s, x, init);
> +       return !kasan_slab_free(s, x, init, after_rcu_delay);
>  }
>
>  static __fastpath_inline
>  bool slab_free_freelist_hook(struct kmem_cache *s, void **head, void **tail,
>                              int *cnt)
>  {
>
>         void *object;
>         void *next = *head;
>         void *old_tail = *tail;
>         bool init;
>
>         if (is_kfence_address(next)) {
> -               slab_free_hook(s, next, false);
> +               slab_free_hook(s, next, false, false);
>                 return false;
>         }
>
>         /* Head and tail of the reconstructed freelist */
>         *head = NULL;
>         *tail = NULL;
>
>         init = slab_want_init_on_free(s);
>
>         do {
>                 object = next;
>                 next = get_freepointer(s, object);
>
>                 /* If object's reuse doesn't have to be delayed */
> -               if (likely(slab_free_hook(s, object, init))) {
> +               if (likely(slab_free_hook(s, object, init, false))) {
>                         /* Move object to the new freelist */
>                         set_freepointer(s, object, *head);
>                         *head = object;
>                         if (!*tail)
>                                 *tail = object;
>                 } else {
> @@ -4474,40 +4507,67 @@ static __fastpath_inline
>  void slab_free(struct kmem_cache *s, struct slab *slab, void *object,
>                unsigned long addr)
>  {
>         memcg_slab_free_hook(s, slab, &object, 1);
>         alloc_tagging_slab_free_hook(s, slab, &object, 1);
>
> -       if (likely(slab_free_hook(s, object, slab_want_init_on_free(s))))
> +       if (likely(slab_free_hook(s, object, slab_want_init_on_free(s), false)))
>                 do_slab_free(s, slab, object, object, 1, addr);
>  }
>
>  #ifdef CONFIG_MEMCG
>  /* Do not inline the rare memcg charging failed path into the allocation path */
>  static noinline
>  void memcg_alloc_abort_single(struct kmem_cache *s, void *object)
>  {
> -       if (likely(slab_free_hook(s, object, slab_want_init_on_free(s))))
> +       if (likely(slab_free_hook(s, object, slab_want_init_on_free(s), false)))
>                 do_slab_free(s, virt_to_slab(object), object, object, 1, _RET_IP_);
>  }
>  #endif
>
>  static __fastpath_inline
>  void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head,
>                     void *tail, void **p, int cnt, unsigned long addr)
>  {
>         memcg_slab_free_hook(s, slab, p, cnt);
>         alloc_tagging_slab_free_hook(s, slab, p, cnt);
>         /*
>          * With KASAN enabled slab_free_freelist_hook modifies the freelist
>          * to remove objects, whose reuse must be delayed.
>          */
>         if (likely(slab_free_freelist_hook(s, &head, &tail, &cnt)))
>                 do_slab_free(s, slab, head, tail, cnt, addr);
>  }
>
> +#ifdef CONFIG_SLUB_RCU_DEBUG
> +static void slab_free_after_rcu_debug(struct rcu_head *rcu_head)
> +{
> +       struct rcu_delayed_free *delayed_free =
> +                       container_of(rcu_head, struct rcu_delayed_free, head);
> +       void *object = delayed_free->object;
> +       struct slab *slab = virt_to_slab(object);
> +       struct kmem_cache *s;
> +
> +       if (WARN_ON(is_kfence_address(rcu_head)))
> +               return;
> +
> +       /* find the object and the cache again */
> +       if (WARN_ON(!slab))
> +               return;
> +       s = slab->slab_cache;
> +       if (WARN_ON(!(s->flags & SLAB_TYPESAFE_BY_RCU)))
> +               return;
> +
> +       /* resume freeing */
> +       if (!slab_free_hook(s, object, slab_want_init_on_free(s), true))
> +               return;
> +       do_slab_free(s, slab, object, object, 1, _THIS_IP_);
> +       kfree(delayed_free);
> +}
> +#endif /* CONFIG_SLUB_RCU_DEBUG */
> +
>  #ifdef CONFIG_KASAN_GENERIC
>  void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
>  {
>         do_slab_free(cache, virt_to_slab(x), x, x, 1, addr);
>  }
>  #endif
>
> --
> 2.46.0.rc1.232.g9752f9e123-goog
>

Re: [PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Jann Horn]

(I'll address the other feedback later)

On Thu, Aug 1, 2024 at 2:23 AM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.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 RCU-delay the actual
> > SLUB freeing of an object, and we'd like KASAN to still validate
> > synchronously that freeing the object is permitted. (Otherwise this
> > change will make the existing testcase kmem_cache_invalid_free fail.)
> >
> > So add a new KASAN hook that allows KASAN to pre-validate a
> > kmem_cache_free() operation before SLUB actually starts modifying the
> > object or its metadata.
[...]
> > @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip)
> >                 kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false);
> >                 return true;
> >         }
> >
> >         if (is_kfence_address(ptr))
> >                 return false;
> > +       if (!kasan_arch_is_ready())
> > +               return true;
>
> Hm, I think we had a bug here: the function should return true in both
> cases. This seems reasonable: if KASAN is not checking the object, the
> caller can do whatever they want with it.

But if the object is a kfence allocation, we maybe do want the caller
to free it quickly so that kfence can catch potential UAF access? So
"return false" in that case seems appropriate. Or maybe we don't
because that makes the probability of catching an OOB access much
lower if the mempool is going to always return non-kfence allocations
as long as the pool isn't empty? Also I guess whether kfence vetoes
reuse of kfence objects probably shouldn't depend on whether the
kernel is built with KASAN... so I guess it would be more consistent
to either put "return true" there or change the !KASAN stub of this to
check for kfence objects or something like that? Honestly I think the
latter would be most appropriate, though then maybe the hook shouldn't
have "kasan" in its name...

Either way, I agree that the current situation wrt mempools and kfence
is inconsistent, but I think I should probably leave that as-is in my
series for now, and the kfence mempool issue can be addressed
separately afterwards? I also would like to avoid changing kfence
behavior as part of this patch.

If you want, I can add a comment above the "if (is_kfence_address())"
that notes the inconsistency?

Re: [PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Andrey Konovalov]

On Thu, Aug 1, 2024 at 6:01 AM Jann Horn <jannh@google.com> wrote:
>
> > > @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip)
> > >                 kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false);
> > >                 return true;
> > >         }
> > >
> > >         if (is_kfence_address(ptr))
> > >                 return false;
> > > +       if (!kasan_arch_is_ready())
> > > +               return true;
> >
> > Hm, I think we had a bug here: the function should return true in both
> > cases. This seems reasonable: if KASAN is not checking the object, the
> > caller can do whatever they want with it.
>
> But if the object is a kfence allocation, we maybe do want the caller
> to free it quickly so that kfence can catch potential UAF access? So
> "return false" in that case seems appropriate.

Return false would mean: allocation is buggy, do not use it and do not
free it (note that the return value meaning here is inverse compared
to the newly added check_slab_allocation()). And this doesn't seem
like something we want for KFENCE-managed objects. But regardless of
the return value here, the callers tend not to free these allocations
to the slab allocator, that's the point of mempools. So KFENCE won't
catch a UAF either way.

> Or maybe we don't
> because that makes the probability of catching an OOB access much
> lower if the mempool is going to always return non-kfence allocations
> as long as the pool isn't empty? Also I guess whether kfence vetoes
> reuse of kfence objects probably shouldn't depend on whether the
> kernel is built with KASAN... so I guess it would be more consistent
> to either put "return true" there or change the !KASAN stub of this to
> check for kfence objects or something like that? Honestly I think the
> latter would be most appropriate, though then maybe the hook shouldn't
> have "kasan" in its name...

Yeah, we could add some custom handling of mempool to KFENCE as well.
But that would be a separate effort.

> Either way, I agree that the current situation wrt mempools and kfence
> is inconsistent, but I think I should probably leave that as-is in my
> series for now, and the kfence mempool issue can be addressed
> separately afterwards? I also would like to avoid changing kfence
> behavior as part of this patch.

Sure, sounds good to me.

> If you want, I can add a comment above the "if (is_kfence_address())"
> that notes the inconsistency?

Up to you, I'll likely add a note to the bug tracker to fix this once
the patch lands anyway.

Thanks!

Re: [PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Marco Elver]

On Tue, Jul 30, 2024 at 01:06PM +0200, Jann Horn wrote:
[...]
> +#ifdef CONFIG_SLUB_RCU_DEBUG
> +	if ((s->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay) {
> +		struct rcu_delayed_free *delayed_free;
> +
> +		delayed_free = kmalloc(sizeof(*delayed_free), GFP_NOWAIT);

This may well be allocated by KFENCE.

[...]
> +#ifdef CONFIG_SLUB_RCU_DEBUG
> +static void slab_free_after_rcu_debug(struct rcu_head *rcu_head)
> +{
> +	struct rcu_delayed_free *delayed_free =
> +			container_of(rcu_head, struct rcu_delayed_free, head);
> +	void *object = delayed_free->object;
> +	struct slab *slab = virt_to_slab(object);
> +	struct kmem_cache *s;
> +
> +	if (WARN_ON(is_kfence_address(rcu_head)))
> +		return;

syzbot found this warning to trigger (because see above comment):
https://lore.kernel.org/all/00000000000052aa15061eaeb1fd@google.com/

Should this have been `is_kfence_address(object)`?

Re: [PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Jann Horn]

On Fri, Aug 2, 2024 at 10:06 AM Marco Elver <elver@google.com> wrote:
>
> On Tue, Jul 30, 2024 at 01:06PM +0200, Jann Horn wrote:
> [...]
> > +#ifdef CONFIG_SLUB_RCU_DEBUG
> > +     if ((s->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay) {
> > +             struct rcu_delayed_free *delayed_free;
> > +
> > +             delayed_free = kmalloc(sizeof(*delayed_free), GFP_NOWAIT);
>
> This may well be allocated by KFENCE.
>
> [...]
> > +#ifdef CONFIG_SLUB_RCU_DEBUG
> > +static void slab_free_after_rcu_debug(struct rcu_head *rcu_head)
> > +{
> > +     struct rcu_delayed_free *delayed_free =
> > +                     container_of(rcu_head, struct rcu_delayed_free, head);
> > +     void *object = delayed_free->object;
> > +     struct slab *slab = virt_to_slab(object);
> > +     struct kmem_cache *s;
> > +
> > +     if (WARN_ON(is_kfence_address(rcu_head)))
> > +             return;
>
> syzbot found this warning to trigger (because see above comment):
> https://lore.kernel.org/all/00000000000052aa15061eaeb1fd@google.com/
>
> Should this have been `is_kfence_address(object)`?

Whoops, indeed... thanks, will fix in v6.

Re: [PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Jann Horn]

On Thu, Aug 1, 2024 at 2:23 AM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.com> wrote:
> > Currently, KASAN is unable to catch use-after-free in SLAB_TYPESAFE_BY_RCU
> > slabs because use-after-free is allowed within the RCU grace period by
> > design.
> >
> > Add a SLUB debugging feature which RCU-delays every individual
> > kmem_cache_free() before either actually freeing the object or handing it
> > off to KASAN, and change KASAN to poison freed objects as normal when this
> > option is enabled.
> >
> > For now I've configured Kconfig.debug to default-enable this feature in the
> > KASAN GENERIC and SW_TAGS modes; I'm not enabling it by default in HW_TAGS
> > mode because I'm not sure if it might have unwanted performance degradation
> > effects there.
> >
> > Note that this is mostly useful with KASAN in the quarantine-based GENERIC
> > mode; SLAB_TYPESAFE_BY_RCU slabs are basically always also slabs with a
> > ->ctor, and KASAN's assign_tag() currently has to assign fixed tags for
> > those, reducing the effectiveness of SW_TAGS/HW_TAGS mode.
> > (A possible future extension of this work would be to also let SLUB call
> > the ->ctor() on every allocation instead of only when the slab page is
> > allocated; then tag-based modes would be able to assign new tags on every
> > reallocation.)
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> Acked-by: Andrey Konovalov <andreyknvl@gmail.com>
>
> But see a comment below.
>
> > ---
> >  include/linux/kasan.h | 11 +++++---
> >  mm/Kconfig.debug      | 30 ++++++++++++++++++++
> >  mm/kasan/common.c     | 11 ++++----
> >  mm/kasan/kasan_test.c | 46 +++++++++++++++++++++++++++++++
> >  mm/slab_common.c      | 12 ++++++++
> >  mm/slub.c             | 76 +++++++++++++++++++++++++++++++++++++++++++++------
> >  6 files changed, 169 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index 34cb7a25aacb..0b952e11c7a0 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -194,28 +194,30 @@ static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s,
> >  {
> >         if (kasan_enabled())
> >                 return __kasan_slab_pre_free(s, object, _RET_IP_);
> >         return false;
> >  }
> >
> > -bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init);
> > +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init,
> > +                      bool after_rcu_delay);
>
> What do you think about renaming this argument to poison_rcu? I think
> it makes the intention more clear from the KASAN's point of view.

Hm - my thinking here was that the hook is an API between SLUB and
KASAN, and so the hook definition should reflect the API contract that
both SLUB and KASAN have to know - and in my head, this contract is
that the parameter says whether SLUB guarantees that an RCU delay has
happened after kmem_cache_free() was called.
In my mind, SLUB tells KASAN what is going on and gives KASAN a chance
to take ownership of the object, but doesn't instruct KASAN to do
anything specific.

And "poison" is ambiguous - in SLUB, "poison" normally refers to
overwriting object contents with a poison value, which currently
wouldn't be allowed here due to constructor slabs.

I guess another way to describe the meaning of the argument with its
current value would be something like "even though the object is an
object with RCU lifetime, the object is guaranteed to no longer be in
use". But I think the simplest way to describe the argument as
currently defined is "an RCU grace period has passed since
kmem_cache_free() was called" (which I guess I'll add to the
kasan_slab_free doc comment if we keep the current naming).

I guess I could also change the API to pass something different - like
a flag meaning "the object is guaranteed to no longer be in use".
There is already code in slab_free_hook() that computes this
expression, so we could easily pass that to KASAN and then avoid doing
the same logic in KASAN again... I think that would be the most
elegant approach?

> >  /**
> >   * kasan_slab_free - Possibly handle slab object freeing.
> >   * @object: Object to free.
>
> @poison_rcu - Whether to skip poisoning for SLAB_TYPESAFE_BY_RCU caches.
>
> And also update the reworded comment from the previous patch:
>
> This function poisons a slab object and saves a free stack trace for
> it, except for SLAB_TYPESAFE_BY_RCU caches when @poison_rcu is false.

I think that's a KASAN implementation detail, so I would prefer not
putting that in this header.

Re: [PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Jann Horn]

On Thu, Aug 1, 2024 at 2:23 AM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.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 RCU-delay the actual
> > SLUB freeing of an object, and we'd like KASAN to still validate
> > synchronously that freeing the object is permitted. (Otherwise this
> > change will make the existing testcase kmem_cache_invalid_free fail.)
> >
> > So add a new KASAN hook that allows KASAN to pre-validate a
> > kmem_cache_free() operation before SLUB actually starts modifying the
> > object or its metadata.
>
> A few more minor comments below. With that:
>
> Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
>
> Thank you!
>
> > Inside KASAN, this:
> >
> >  - moves checks from poison_slab_object() into check_slab_free()
> >  - moves kasan_arch_is_ready() up into callers of poison_slab_object()
> >  - removes "ip" argument of poison_slab_object() and __kasan_slab_free()
> >    (since those functions no longer do any reporting)
>
> >  - renames check_slab_free() to check_slab_allocation()
>
> check_slab_allocation() is introduced in this patch, so technically
> you don't rename anything.

Right, I'll fix the commit message.

> > Acked-by: Vlastimil Babka <vbabka@suse.cz> #slub
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> >  include/linux/kasan.h | 43 ++++++++++++++++++++++++++++++++++---
> >  mm/kasan/common.c     | 59 +++++++++++++++++++++++++++++++--------------------
> >  mm/slub.c             |  7 ++++++
> >  3 files changed, 83 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index 70d6a8f6e25d..34cb7a25aacb 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -172,19 +172,50 @@ static __always_inline void * __must_check kasan_init_slab_obj(
> >  {
> >         if (kasan_enabled())
> >                 return __kasan_init_slab_obj(cache, object);
> >         return (void *)object;
> >  }
> >
> > -bool __kasan_slab_free(struct kmem_cache *s, void *object,
> > -                       unsigned long ip, bool init);
> > +bool __kasan_slab_pre_free(struct kmem_cache *s, void *object,
> > +                       unsigned long ip);
> > +/**
> > + * kasan_slab_pre_free - Validate a slab object freeing request.
> > + * @object: Object to free.
> > + *
> > + * This function checks whether freeing the given object might be permitted; it
> > + * checks things like whether the given object is properly aligned and not
> > + * already freed.
> > + *
> > + * This function is only intended for use by the slab allocator.
> > + *
> > + * @Return true if freeing the object is known to be invalid; false otherwise.
> > + */
>
> Let's reword this to:
>
> kasan_slab_pre_free - Check whether freeing a slab object is safe.
> @object: Object to be freed.
>
> This function checks whether freeing the given object is safe. It
> performs checks to detect double-free and invalid-free bugs and
> reports them.
>
> This function is intended only for use by the slab allocator.
>
> @Return true if freeing the object is not safe; false otherwise.

Ack, will apply this for v6. But I'll replace "not safe" with
"unsafe", and change "It performs checks to detect double-free and
invalid-free bugs and reports them" to "It may check for double-free
and invalid-free bugs and report them.", since KASAN only sometimes
performs such checks (depending on CONFIG_KASAN, kasan_enabled(),
kasan_arch_is_ready(), and so on).

> > +static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s,
> > +                                               void *object)
> > +{
> > +       if (kasan_enabled())
> > +               return __kasan_slab_pre_free(s, object, _RET_IP_);
> > +       return false;
> > +}
> > +
> > +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init);
> > +/**
> > + * kasan_slab_free - Possibly handle slab object freeing.
> > + * @object: Object to free.
> > + *
> > + * This hook is called from the slab allocator to give KASAN a chance to take
> > + * ownership of the object and handle its freeing.
> > + * kasan_slab_pre_free() must have already been called on the same object.
> > + *
> > + * @Return true if KASAN took ownership of the object; false otherwise.
> > + */
>
> kasan_slab_free - Poison, initialize, and quarantine a slab object.
> @object: Object to be freed.
> @init: Whether to initialize the object.
>
> This function poisons a slab object and saves a free stack trace for
> it, except for SLAB_TYPESAFE_BY_RCU caches.
>
> For KASAN modes that have integrated memory initialization
> (kasan_has_integrated_init() == true), this function also initializes
> the object's memory. For other modes, the @init argument is ignored.

As an aside: Is this actually reliably true? It would be false for
kfence objects, but luckily we can't actually get kfence objects
passed to this function (which I guess maybe we should maybe document
here as part of the API). It would also be wrong if
__kasan_slab_free() can be reached while kasan_arch_is_ready() is
false, which I guess would happen if you ran a CONFIG_KASAN=y kernel
on a powerpc machine without radix or something like that?

(And similarly I wonder if the check of kasan_has_integrated_init() in
slab_post_alloc_hook() is racy, but I haven't checked in which phase
of boot KASAN is enabled for HWASAN.)

But I guess that's out of scope for this series.

> For the Generic mode, this function might also quarantine the object.
> When this happens, KASAN will defer freeing the object to a later
> stage and handle it internally then. The return value indicates
> whether the object was quarantined.
>
> This function is intended only for use by the slab allocator.
>
> @Return true if KASAN quarantined the object; false otherwise.

Same thing as I wrote on patch 2/2: To me this seems like too much
implementation detail for the documentation of an API between
components of the kernel? I agree that the meaning of the "init"
argument is important to document here, and it should be documented
that the hook can take ownership of the object (and I guess it's fine
to mention that this is for quarantine purposes), but I would leave
out details about differences in behavior between KASAN modes.
Basically my heuristic here is that in my opinion, this header comment
should mostly describe as much of the function as SLUB has to know to
properly use it.

So I'd do something like:

<<<
kasan_slab_free - Poison, initialize, and quarantine a slab object.
@object: Object to be freed.
@init: Whether to initialize the object.

This function informs that a slab object has been freed and is not
supposed to be accessed anymore, except for objects in
SLAB_TYPESAFE_BY_RCU caches.

For KASAN modes that have integrated memory initialization
(kasan_has_integrated_init() == true), this function also initializes
the object's memory. For other modes, the @init argument is ignored.

This function might also take ownership of the object to quarantine it.
When this happens, KASAN will defer freeing the object to a later
stage and handle it internally until then. The return value indicates
whether KASAN took ownership of the object.

This function is intended only for use by the slab allocator.

@Return true if KASAN took ownership of the object; false otherwise.
>>>

But if you disagree, I'll add your full comment as you suggested.

[...]
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 85e7c6b4575c..8cede1ce00e1 100644
> > --- a/mm/kasan/common.c
> > +++ b/mm/kasan/common.c
> > @@ -205,59 +205,65 @@ void * __must_check __kasan_init_slab_obj(struct kmem_cache *cache,
> >         /* Tag is ignored in set_tag() without CONFIG_KASAN_SW/HW_TAGS */
> >         object = set_tag(object, assign_tag(cache, object, true));
> >
> >         return (void *)object;
> >  }
> >
> > -static inline bool poison_slab_object(struct kmem_cache *cache, void *object,
> > -                                     unsigned long ip, bool init)
> > +/* returns true for invalid request */
>
> "Returns true when freeing the object is not safe."

ack, applied

Re: [PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Jann Horn]

On Thu, Aug 1, 2024 at 2:54 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> On Thu, Aug 1, 2024 at 6:01 AM Jann Horn <jannh@google.com> wrote:
> >
> > > > @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip)
> > > >                 kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false);
> > > >                 return true;
> > > >         }
> > > >
> > > >         if (is_kfence_address(ptr))
> > > >                 return false;
> > > > +       if (!kasan_arch_is_ready())
> > > > +               return true;
> > >
> > > Hm, I think we had a bug here: the function should return true in both
> > > cases. This seems reasonable: if KASAN is not checking the object, the
> > > caller can do whatever they want with it.
> >
> > But if the object is a kfence allocation, we maybe do want the caller
> > to free it quickly so that kfence can catch potential UAF access? So
> > "return false" in that case seems appropriate.
>
> Return false would mean: allocation is buggy, do not use it and do not
> free it (note that the return value meaning here is inverse compared
> to the newly added check_slab_allocation()). And this doesn't seem
> like something we want for KFENCE-managed objects. But regardless of
> the return value here, the callers tend not to free these allocations
> to the slab allocator, that's the point of mempools. So KFENCE won't
> catch a UAF either way.

Oooh, right, I misunderstood the semantics of the function. I'll
change it in v6.

Re: [PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Jann Horn]

On Fri, Aug 2, 2024 at 11:09 AM Jann Horn <jannh@google.com> wrote:
> I guess I could also change the API to pass something different - like
> a flag meaning "the object is guaranteed to no longer be in use".
> There is already code in slab_free_hook() that computes this
> expression, so we could easily pass that to KASAN and then avoid doing
> the same logic in KASAN again... I think that would be the most
> elegant approach?

Regarding this, I think I'll add something like this on top of this patch in v6:

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b63f5351c5f3..50bad011352e 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -201,16 +201,17 @@ bool __kasan_slab_free(struct kmem_cache *s,
void *object, bool init,
 /**
  * kasan_slab_free - Possibly handle slab object freeing.
  * @object: Object to free.
+ * @still_accessible: Whether the object contents are still accessible.
  *
  * This hook is called from the slab allocator to give KASAN a chance to take
  * ownership of the object and handle its freeing.
  * kasan_slab_pre_free() must have already been called on the same object.
  *
  * @Return true if KASAN took ownership of the object; false otherwise.
  */
 static __always_inline bool kasan_slab_free(struct kmem_cache *s,
                                                void *object, bool init,
-                                               bool after_rcu_delay)
+                                               bool still_accessible)
 {
        if (kasan_enabled())
                return __kasan_slab_free(s, object, init, after_rcu_delay);
@@ -410,7 +411,7 @@ static inline bool kasan_slab_pre_free(struct
kmem_cache *s, void *object)
 }

 static inline bool kasan_slab_free(struct kmem_cache *s, void *object,
-                                  bool init, bool after_rcu_delay)
+                                  bool init, bool still_accessible)
 {
        return false;
 }
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 71a20818b122..ed4873e18c75 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -230,14 +230,14 @@ static bool check_slab_allocation(struct
kmem_cache *cache, void *object,
 }

 static inline void poison_slab_object(struct kmem_cache *cache, void *object,
-                                     bool init, bool after_rcu_delay)
+                                     bool init, bool still_accessible)
 {
        void *tagged_object = object;

        object = kasan_reset_tag(object);

        /* RCU slabs could be legally used after free within the RCU period. */
-       if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay)
+       if (unlikely(still_accessible))
                return;

        kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_SIZE),
@@ -256,12 +256,12 @@ bool __kasan_slab_pre_free(struct kmem_cache
*cache, void *object,
 }

 bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init,
-                      bool after_rcu_delay)
+                      bool still_accessible)
 {
        if (!kasan_arch_is_ready() || is_kfence_address(object))
                return false;

-       poison_slab_object(cache, object, init, after_rcu_delay);
+       poison_slab_object(cache, object, init, still_accessible);

        /*
         * If the object is put into quarantine, do not let slab put the object
diff --git a/mm/slub.c b/mm/slub.c
index 49571d5ded75..a89f2006d46e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2221,31 +2221,34 @@ static __always_inline
 bool slab_free_hook(struct kmem_cache *s, void *x, bool init,
                    bool after_rcu_delay)
 {
+       /* Are the object contents still accessible? */
+       bool still_accessible = (s->flags & SLAB_TYPESAFE_BY_RCU) &&
!after_rcu_delay;
+
        kmemleak_free_recursive(x, s->flags);
        kmsan_slab_free(s, x);

        debug_check_no_locks_freed(x, s->object_size);

        if (!(s->flags & SLAB_DEBUG_OBJECTS))
                debug_check_no_obj_freed(x, s->object_size);

        /* Use KCSAN to help debug racy use-after-free. */
-       if (!(s->flags & SLAB_TYPESAFE_BY_RCU) || after_rcu_delay)
+       if (!still_accessible)
                __kcsan_check_access(x, s->object_size,
                                     KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT);

        if (kfence_free(x))
                return false;

        /*
         * Give KASAN a chance to notice an invalid free operation before we
         * modify the object.
         */
        if (kasan_slab_pre_free(s, x))
                return false;

 #ifdef CONFIG_SLUB_RCU_DEBUG
-       if ((s->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay) {
+       if (still_accessible) {
                struct rcu_delayed_free *delayed_free;

                delayed_free = kmalloc(sizeof(*delayed_free), GFP_NOWAIT);
@@ -2289,7 +2292,7 @@ bool slab_free_hook(struct kmem_cache *s, void
*x, bool init,
                       s->size - inuse - rsize);
        }
        /* KASAN might put x into memory quarantine, delaying its reuse. */
-       return !kasan_slab_free(s, x, init, after_rcu_delay);
+       return !kasan_slab_free(s, x, init, still_accessible);
 }

 static __fastpath_inline

Re: [PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Andrey Konovalov]

On Fri, Aug 2, 2024 at 11:57 AM Jann Horn <jannh@google.com> wrote:
>
> >
> > Let's reword this to:
> >
> > kasan_slab_pre_free - Check whether freeing a slab object is safe.
> > @object: Object to be freed.
> >
> > This function checks whether freeing the given object is safe. It
> > performs checks to detect double-free and invalid-free bugs and
> > reports them.
> >
> > This function is intended only for use by the slab allocator.
> >
> > @Return true if freeing the object is not safe; false otherwise.
>
> Ack, will apply this for v6. But I'll replace "not safe" with
> "unsafe", and change "It performs checks to detect double-free and
> invalid-free bugs and reports them" to "It may check for double-free
> and invalid-free bugs and report them.", since KASAN only sometimes
> performs such checks (depending on CONFIG_KASAN, kasan_enabled(),
> kasan_arch_is_ready(), and so on).

Ok!

> > kasan_slab_free - Poison, initialize, and quarantine a slab object.
> > @object: Object to be freed.
> > @init: Whether to initialize the object.
> >
> > This function poisons a slab object and saves a free stack trace for
> > it, except for SLAB_TYPESAFE_BY_RCU caches.
> >
> > For KASAN modes that have integrated memory initialization
> > (kasan_has_integrated_init() == true), this function also initializes
> > the object's memory. For other modes, the @init argument is ignored.
>
> As an aside: Is this actually reliably true? It would be false for
> kfence objects, but luckily we can't actually get kfence objects
> passed to this function (which I guess maybe we should maybe document
> here as part of the API). It would also be wrong if
> __kasan_slab_free() can be reached while kasan_arch_is_ready() is
> false, which I guess would happen if you ran a CONFIG_KASAN=y kernel
> on a powerpc machine without radix or something like that?
>
> (And similarly I wonder if the check of kasan_has_integrated_init() in
> slab_post_alloc_hook() is racy, but I haven't checked in which phase
> of boot KASAN is enabled for HWASAN.)
>
> But I guess that's out of scope for this series.

Yeah, valid concerns. Documenting all of them is definitely too much, though.

> > For the Generic mode, this function might also quarantine the object.
> > When this happens, KASAN will defer freeing the object to a later
> > stage and handle it internally then. The return value indicates
> > whether the object was quarantined.
> >
> > This function is intended only for use by the slab allocator.
> >
> > @Return true if KASAN quarantined the object; false otherwise.
>
> Same thing as I wrote on patch 2/2: To me this seems like too much
> implementation detail for the documentation of an API between
> components of the kernel? I agree that the meaning of the "init"
> argument is important to document here, and it should be documented
> that the hook can take ownership of the object (and I guess it's fine
> to mention that this is for quarantine purposes), but I would leave
> out details about differences in behavior between KASAN modes.
> Basically my heuristic here is that in my opinion, this header comment
> should mostly describe as much of the function as SLUB has to know to
> properly use it.
>
> So I'd do something like:
>
> <<<
> kasan_slab_free - Poison, initialize, and quarantine a slab object.
> @object: Object to be freed.
> @init: Whether to initialize the object.
>
> This function informs that a slab object has been freed and is not
> supposed to be accessed anymore, except for objects in
> SLAB_TYPESAFE_BY_RCU caches.
>
> For KASAN modes that have integrated memory initialization
> (kasan_has_integrated_init() == true), this function also initializes
> the object's memory. For other modes, the @init argument is ignored.
>
> This function might also take ownership of the object to quarantine it.
> When this happens, KASAN will defer freeing the object to a later
> stage and handle it internally until then. The return value indicates
> whether KASAN took ownership of the object.
>
> This function is intended only for use by the slab allocator.
>
> @Return true if KASAN took ownership of the object; false otherwise.
> >>>

Looks good to me.

Re: [PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Andrey Konovalov]

On Fri, Aug 2, 2024 at 1:23 PM Jann Horn <jannh@google.com> wrote:
>
> On Fri, Aug 2, 2024 at 11:09 AM Jann Horn <jannh@google.com> wrote:
> > I guess I could also change the API to pass something different - like
> > a flag meaning "the object is guaranteed to no longer be in use".
> > There is already code in slab_free_hook() that computes this
> > expression, so we could easily pass that to KASAN and then avoid doing
> > the same logic in KASAN again... I think that would be the most
> > elegant approach?
>
> Regarding this, I think I'll add something like this on top of this patch in v6:
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index b63f5351c5f3..50bad011352e 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -201,16 +201,17 @@ bool __kasan_slab_free(struct kmem_cache *s,
> void *object, bool init,
>  /**
>   * kasan_slab_free - Possibly handle slab object freeing.
>   * @object: Object to free.
> + * @still_accessible: Whether the object contents are still accessible.
>   *
>   * This hook is called from the slab allocator to give KASAN a chance to take
>   * ownership of the object and handle its freeing.
>   * kasan_slab_pre_free() must have already been called on the same object.
>   *
>   * @Return true if KASAN took ownership of the object; false otherwise.
>   */
>  static __always_inline bool kasan_slab_free(struct kmem_cache *s,
>                                                 void *object, bool init,
> -                                               bool after_rcu_delay)
> +                                               bool still_accessible)
>  {
>         if (kasan_enabled())
>                 return __kasan_slab_free(s, object, init, after_rcu_delay);
> @@ -410,7 +411,7 @@ static inline bool kasan_slab_pre_free(struct
> kmem_cache *s, void *object)
>  }
>
>  static inline bool kasan_slab_free(struct kmem_cache *s, void *object,
> -                                  bool init, bool after_rcu_delay)
> +                                  bool init, bool still_accessible)
>  {
>         return false;
>  }
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 71a20818b122..ed4873e18c75 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -230,14 +230,14 @@ static bool check_slab_allocation(struct
> kmem_cache *cache, void *object,
>  }
>
>  static inline void poison_slab_object(struct kmem_cache *cache, void *object,
> -                                     bool init, bool after_rcu_delay)
> +                                     bool init, bool still_accessible)
>  {
>         void *tagged_object = object;
>
>         object = kasan_reset_tag(object);
>
>         /* RCU slabs could be legally used after free within the RCU period. */
> -       if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay)
> +       if (unlikely(still_accessible))
>                 return;
>
>         kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_SIZE),
> @@ -256,12 +256,12 @@ bool __kasan_slab_pre_free(struct kmem_cache
> *cache, void *object,
>  }
>
>  bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init,
> -                      bool after_rcu_delay)
> +                      bool still_accessible)
>  {
>         if (!kasan_arch_is_ready() || is_kfence_address(object))
>                 return false;
>
> -       poison_slab_object(cache, object, init, after_rcu_delay);
> +       poison_slab_object(cache, object, init, still_accessible);
>
>         /*
>          * If the object is put into quarantine, do not let slab put the object
> diff --git a/mm/slub.c b/mm/slub.c
> index 49571d5ded75..a89f2006d46e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2221,31 +2221,34 @@ static __always_inline
>  bool slab_free_hook(struct kmem_cache *s, void *x, bool init,
>                     bool after_rcu_delay)
>  {
> +       /* Are the object contents still accessible? */
> +       bool still_accessible = (s->flags & SLAB_TYPESAFE_BY_RCU) &&
> !after_rcu_delay;
> +
>         kmemleak_free_recursive(x, s->flags);
>         kmsan_slab_free(s, x);
>
>         debug_check_no_locks_freed(x, s->object_size);
>
>         if (!(s->flags & SLAB_DEBUG_OBJECTS))
>                 debug_check_no_obj_freed(x, s->object_size);
>
>         /* Use KCSAN to help debug racy use-after-free. */
> -       if (!(s->flags & SLAB_TYPESAFE_BY_RCU) || after_rcu_delay)
> +       if (!still_accessible)
>                 __kcsan_check_access(x, s->object_size,
>                                      KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT);
>
>         if (kfence_free(x))
>                 return false;
>
>         /*
>          * Give KASAN a chance to notice an invalid free operation before we
>          * modify the object.
>          */
>         if (kasan_slab_pre_free(s, x))
>                 return false;
>
>  #ifdef CONFIG_SLUB_RCU_DEBUG
> -       if ((s->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay) {
> +       if (still_accessible) {
>                 struct rcu_delayed_free *delayed_free;
>
>                 delayed_free = kmalloc(sizeof(*delayed_free), GFP_NOWAIT);
> @@ -2289,7 +2292,7 @@ bool slab_free_hook(struct kmem_cache *s, void
> *x, bool init,
>                        s->size - inuse - rsize);
>         }
>         /* KASAN might put x into memory quarantine, delaying its reuse. */
> -       return !kasan_slab_free(s, x, init, after_rcu_delay);
> +       return !kasan_slab_free(s, x, init, still_accessible);
>  }
>
>  static __fastpath_inline

Ok, let's do it like this.

Thank you!