| Message ID | 20240730-kasan-tsbrcu-v5-2-48d3cbdfccc5@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | allow KASAN to detect UAF in SLAB_TYPESAFE_BY_RCU slabs | expand |
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
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 >
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)`?
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.
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.
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
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!
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
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(-)