RE: [PATCH v5 1/2] mm, kasan: improve double-free detection

["Luruo, Kuthonuzo" <kuthonuzo.luruo@hpe.com>]

> > Next time, when/if you send patch series, send patches in one thread, i.e.
> > patches should be replies to the cover letter.
> > Your patches are not linked together, which makes them harder to track.

Thanks for the tip; but doesn't this conflict with the advice in
https://www.kernel.org/doc/Documentation/SubmittingPatches, specifically the
use of  the "summary phrase"... 

> >
> >
> >> Currently, KASAN may fail to detect concurrent deallocations of the same
> >> object due to a race in kasan_slab_free(). This patch makes double-free
> >> detection more reliable by serializing access to KASAN object metadata.
> >> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> >> lock/unlock per-object metadata. Double-free errors are now reported via
> >> kasan_report().
> >>
> >> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
> >>
> >
> >
> > So, I still don't like this, this too way hacky and complex.

I don't think patch is particularly complex; but respect your judgment.

> > I have some thoughts about how to make this lockless and robust enough.
> > I'll try to sort this out tomorrow.
> >
> 
> 
> So, I something like this should work.
> Tested very briefly.
> 
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index ac4b3c4..8691142 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -75,6 +75,8 @@ struct kasan_cache {
>  int kasan_module_alloc(void *addr, size_t size);
>  void kasan_free_shadow(const struct vm_struct *vm);
> 
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object);
> +
>  size_t ksize(const void *);
>  static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
> 
> @@ -102,6 +104,9 @@ static inline void kasan_unpoison_object_data(struct
> kmem_cache *cache,
>  static inline void kasan_poison_object_data(struct kmem_cache *cache,
>  					void *object) {}
> 
> +static inline void kasan_init_slab_obj(struct kmem_cache *cache,
> +				const void *object) { }
> +
>  static inline void kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags) {}
>  static inline void kasan_kfree_large(const void *ptr) {}
>  static inline void kasan_poison_kfree(void *ptr) {}
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 6845f92..ab0fded 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -388,11 +388,9 @@ void kasan_cache_create(struct kmem_cache *cache,
> size_t *size,
>  	*size += sizeof(struct kasan_alloc_meta);
> 
>  	/* Add free meta. */
> -	if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
> -	    cache->object_size < sizeof(struct kasan_free_meta)) {
> -		cache->kasan_info.free_meta_offset = *size;
> -		*size += sizeof(struct kasan_free_meta);
> -	}
> +	cache->kasan_info.free_meta_offset = *size;
> +	*size += sizeof(struct kasan_free_meta);
> +
>  	redzone_adjust = optimal_redzone(cache->object_size) -
>  		(*size - cache->object_size);
>  	if (redzone_adjust > 0)
> @@ -431,13 +429,6 @@ void kasan_poison_object_data(struct kmem_cache
> *cache, void *object)
>  	kasan_poison_shadow(object,
>  			round_up(cache->object_size,
> KASAN_SHADOW_SCALE_SIZE),
>  			KASAN_KMALLOC_REDZONE);
> -#ifdef CONFIG_SLAB
> -	if (cache->flags & SLAB_KASAN) {
> -		struct kasan_alloc_meta *alloc_info =
> -			get_alloc_info(cache, object);
> -		alloc_info->state = KASAN_STATE_INIT;
> -	}
> -#endif
>  }
> 
>  #ifdef CONFIG_SLAB
> @@ -501,6 +492,20 @@ struct kasan_free_meta *get_free_info(struct
> kmem_cache *cache,
>  	BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
>  	return (void *)object + cache->kasan_info.free_meta_offset;
>  }
> +
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object)
> +{
> +	struct kasan_alloc_meta *alloc_info;
> +	struct kasan_free_meta *free_info;
> +
> +	if (!(cache->flags & SLAB_KASAN))
> +		return;
> +
> +	alloc_info = get_alloc_info(cache, object);
> +	free_info = get_free_info(cache, object);
> +	__memset(alloc_info, 0, sizeof(*alloc_info));
> +	__memset(free_info, 0, sizeof(*free_info));
> +}
>  #endif
> 
>  void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
> @@ -523,37 +528,47 @@ static void kasan_poison_slab_free(struct
> kmem_cache *cache, void *object)
>  bool kasan_slab_free(struct kmem_cache *cache, void *object)
>  {
>  #ifdef CONFIG_SLAB
> +	struct kasan_free_meta *free_info = get_free_info(cache, object);
> +	struct kasan_track new_free_stack, old_free_stack;
> +	s8 old_shadow;
> +
>  	/* RCU slabs could be legally used after free within the RCU period */
>  	if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
>  		return false;
> 
> -	if (likely(cache->flags & SLAB_KASAN)) {
> -		struct kasan_alloc_meta *alloc_info =
> -			get_alloc_info(cache, object);
> -		struct kasan_free_meta *free_info =
> -			get_free_info(cache, object);
> -
> -		switch (alloc_info->state) {
> -		case KASAN_STATE_ALLOC:
> -			alloc_info->state = KASAN_STATE_QUARANTINE;
> -			quarantine_put(free_info, cache);
> -			set_track(&free_info->track, GFP_NOWAIT);
> -			kasan_poison_slab_free(cache, object);
> -			return true;
> -		case KASAN_STATE_QUARANTINE:
> -		case KASAN_STATE_FREE:
> -			pr_err("Double free");
> -			dump_stack();
> -			break;
> -		default:
> -			break;
> -		}
> +	if (unlikely(!(cache->flags & SLAB_KASAN)))
> +		return false;
> +
> +	set_track(&new_free_stack, GFP_NOWAIT);
> +	old_free_stack = xchg(&free_info->track, new_free_stack);
> +	old_shadow = xchg((s8 *)kasan_mem_to_shadow(object),
> +			KASAN_KMALLOC_FREE);
> +
> +	if (old_shadow < 0 || old_shadow >= KASAN_SHADOW_SCALE_SIZE) {
> +		struct kasan_track free_stack;
> +
> +		/* Paired with xchg() above */
> +		free_stack = smp_load_acquire(&free_info->track);
> +
> +		/*
> +		 * We didn't raced with another instance of kasan_slab_free()
> +		 * so the previous free stack supposed to be in old_free_stack.
> +		 * Otherwise, free_stack will contain stack trace of another
> +		 * kfree() call.
> +		 */
> +		if (free_stack.id == new_free_stack.id)
> +			free_stack = old_free_stack;
> +
> +		kasan_report_double_free(cache, object,
> +					free_stack, old_shadow);
> +		return false;
>  	}
> -	return false;
> -#else
>  	kasan_poison_slab_free(cache, object);
> -	return false;
> +	return true;
> +
>  #endif
> +	kasan_poison_slab_free(cache, object);
> +	return false;
>  }
> 
>  void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> @@ -581,7 +596,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const
> void *object, size_t size,
>  		struct kasan_alloc_meta *alloc_info =
>  			get_alloc_info(cache, object);
> 
> -		alloc_info->state = KASAN_STATE_ALLOC;
>  		alloc_info->alloc_size = size;
>  		set_track(&alloc_info->track, flags);
>  	}
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index fb87923..9b46d2e 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -59,24 +59,21 @@ struct kasan_global {
>   * Structures to keep alloc and free tracks *
>   */
> 
> -enum kasan_state {
> -	KASAN_STATE_INIT,
> -	KASAN_STATE_ALLOC,
> -	KASAN_STATE_QUARANTINE,
> -	KASAN_STATE_FREE
> -};
> -
>  #define KASAN_STACK_DEPTH 64
> 
>  struct kasan_track {
> -	u32 pid;
> -	depot_stack_handle_t stack;
> +union {
> +	struct {
> +		u32 pid;
> +		depot_stack_handle_t stack;
> +	};
> +	u64 id;
> +};
>  };
> 
>  struct kasan_alloc_meta {
>  	struct kasan_track track;
> -	u32 state : 2;	/* enum kasan_state */
> -	u32 alloc_size : 30;
> +	u32 alloc_size;
>  };
> 
>  struct qlist_node {
> @@ -109,6 +106,9 @@ static inline bool kasan_report_enabled(void)
> 
>  void kasan_report(unsigned long addr, size_t size,
>  		bool is_write, unsigned long ip);
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> +			struct kasan_track free_stack, s8 shadow);
> +
> 
>  #ifdef CONFIG_SLAB
>  void quarantine_put(struct kasan_free_meta *info, struct kmem_cache
> *cache);
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 4973505..3ec039c 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -144,11 +144,9 @@ static void *qlink_to_object(struct qlist_node *qlink,
> struct kmem_cache *cache)
>  static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
>  {
>  	void *object = qlink_to_object(qlink, cache);
> -	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
>  	unsigned long flags;
> 
>  	local_irq_save(flags);
> -	alloc_info->state = KASAN_STATE_FREE;
>  	___cache_free(cache, object, _THIS_IP_);
>  	local_irq_restore(flags);
>  }
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index b3c122d..a0f4519 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,28 +140,13 @@ static void object_err(struct kmem_cache *cache,
> struct page *page,
>  	pr_err("Object at %p, in cache %s\n", object, cache->name);
>  	if (!(cache->flags & SLAB_KASAN))
>  		return;
> -	switch (alloc_info->state) {
> -	case KASAN_STATE_INIT:
> -		pr_err("Object not allocated yet\n");
> -		break;
> -	case KASAN_STATE_ALLOC:
> -		pr_err("Object allocated with size %u bytes.\n",
> -		       alloc_info->alloc_size);
> -		pr_err("Allocation:\n");
> -		print_track(&alloc_info->track);
> -		break;
> -	case KASAN_STATE_FREE:
> -	case KASAN_STATE_QUARANTINE:
> -		pr_err("Object freed, allocated with size %u bytes\n",
> -		       alloc_info->alloc_size);
> -		free_info = get_free_info(cache, object);
> -		pr_err("Allocation:\n");
> -		print_track(&alloc_info->track);
> -		pr_err("Deallocation:\n");
> -		print_track(&free_info->track);
> -		break;
> -	}
> +	free_info = get_free_info(cache, object);
> +	pr_err("Allocation:\n");
> +	print_track(&alloc_info->track);
> +	pr_err("Deallocation:\n");
> +	print_track(&free_info->track);
>  }
> +
>  #endif
> 
>  static void print_address_description(struct kasan_access_info *info)
> @@ -245,17 +230,31 @@ static void print_shadow_for_address(const void
> *addr)
> 
>  static DEFINE_SPINLOCK(report_lock);
> 
> -static void kasan_report_error(struct kasan_access_info *info)
> +static void kasan_start_report(unsigned long *flags)
>  {
> -	unsigned long flags;
> -	const char *bug_type;
> -
>  	/*
>  	 * Make sure we don't end up in loop.
>  	 */
>  	kasan_disable_current();
> -	spin_lock_irqsave(&report_lock, flags);
> +	spin_lock_irqsave(&report_lock, *flags);
> 
> 	pr_err("====================================================
> ==============\n");
> +}
> +
> +static void kasan_end_report(unsigned long *flags)
> +{
> +
> 	pr_err("====================================================
> ==============\n");
> +	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> +	spin_unlock_irqrestore(&report_lock, *flags);
> +	kasan_enable_current();
> +}
> +
> +static void kasan_report_error(struct kasan_access_info *info)
> +{
> +	unsigned long flags;
> +	const char *bug_type;
> +
> +	kasan_start_report(&flags);
> +
>  	if (info->access_addr <
>  			kasan_shadow_to_mem((void
> *)KASAN_SHADOW_START)) {
>  		if ((unsigned long)info->access_addr < PAGE_SIZE)
> @@ -276,10 +275,29 @@ static void kasan_report_error(struct
> kasan_access_info *info)
>  		print_address_description(info);
>  		print_shadow_for_address(info->first_bad_addr);
>  	}
> -
> 	pr_err("====================================================
> ==============\n");
> -	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> -	spin_unlock_irqrestore(&report_lock, flags);
> -	kasan_enable_current();
> +
> +	kasan_end_report(&flags);
> +}
> +
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> +			struct kasan_track free_stack, s8 shadow)
> +{
> +	unsigned long flags;
> +
> +	kasan_start_report(&flags);
> +
> +	pr_err("BUG: Double free or corrupt pointer\n");
> +	pr_err("Unexpected shadow byte: 0x%hhX\n", shadow);
> +
> +	dump_stack();
> +	pr_err("Object at %p, in cache %s\n", object, cache->name);
> +	get_alloc_info(cache, object);
> +	pr_err("Allocation:\n");
> +	print_track(&get_alloc_info(cache, object)->track);
> +	pr_err("Deallocation:\n");
> +	print_track(&free_stack);
> +
> +	kasan_end_report(&flags);
>  }
> 
>  void kasan_report(unsigned long addr, size_t size,
> diff --git a/mm/slab.c b/mm/slab.c
> index 763096a..65c942b 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2604,9 +2604,11 @@ static void cache_init_objs(struct kmem_cache
> *cachep,
>  	}
> 
>  	for (i = 0; i < cachep->num; i++) {
> +		objp = index_to_obj(cachep, page, i);
> +		kasan_init_slab_obj(cachep, objp);
> +
>  		/* constructor could break poison info */
>  		if (DEBUG == 0 && cachep->ctor) {
> -			objp = index_to_obj(cachep, page, i);
>  			kasan_unpoison_object_data(cachep, objp);
>  			cachep->ctor(objp);
>  			kasan_poison_object_data(cachep, objp);

Nice hack & novel approach. It does have the flaw that subsequent error reports
for object will have a bogus deallocation track.

Kuthonuzo