Re: [PATCH] kasan: make depot_fetch_stack more robust

[Joonsoo Kim]

On Fri, Jul 01, 2016 at 07:38:18PM +0200, Dmitry Vyukov wrote:
> I've hit a GPF in depot_fetch_stack when it was given
> bogus stack handle. I think it was caused by a distant
> out-of-bounds that hit a different object, as the result
> we treated uninit garbage as stack handle. Maybe there is
> something to fix in KASAN logic, but I think it makes
> sense to make depot_fetch_stack more robust as well.
> 
> Verify that the provided stack handle looks correct.
> 
> Signed-off-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> ---
> For your convenience uploaded to codereview:
> https://codereview.appspot.com/295680043
> 
> ---
>  include/linux/stackdepot.h |  2 +-
>  lib/stackdepot.c           | 21 +++++++++++++++++----
>  mm/kasan/report.c          | 10 ++++------
>  mm/page_owner.c            | 12 ++++++------
>  4 files changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index 7978b3e..b2dbe02 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -27,6 +27,6 @@ struct stack_trace;
>  
>  depot_stack_handle_t depot_save_stack(struct stack_trace *trace, gfp_t flags);
>  
> -void depot_fetch_stack(depot_stack_handle_t handle, struct stack_trace *trace);
> +bool depot_fetch_stack(depot_stack_handle_t handle, struct stack_trace *trace);
>  
>  #endif
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 53ad6c0..0982331 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -181,16 +181,29 @@ static inline struct stack_record *find_stack(struct stack_record *bucket,
>  	return NULL;
>  }
>  
> -void depot_fetch_stack(depot_stack_handle_t handle, struct stack_trace *trace)
> +bool depot_fetch_stack(depot_stack_handle_t handle, struct stack_trace *trace)
>  {
>  	union handle_parts parts = { .handle = handle };
> -	void *slab = stack_slabs[parts.slabindex];
> -	size_t offset = parts.offset << STACK_ALLOC_ALIGN;
> -	struct stack_record *stack = slab + offset;
> +	void *slab;
> +	struct stack_record *stack;
>  
> +	if (handle == 0)
> +		return false;
> +	if (parts.valid != 1 || parts.slabindex >= ARRAY_SIZE(stack_slabs))
> +		goto bad;
> +	slab = stack_slabs[parts.slabindex];
> +	if (slab == NULL)
> +		goto bad;
> +	stack = slab + (parts.offset << STACK_ALLOC_ALIGN);
> +	if (stack->handle.handle != handle)
> +		goto bad;
>  	trace->nr_entries = trace->max_entries = stack->size;
>  	trace->entries = stack->entries;
>  	trace->skip = 0;
> +	return true;
> +bad:
> +	pr_err("stackdepot: fetching bogus stack %x\n", handle);
> +	return false;
>  }
>  
>  /**
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 861b977..46e4b82 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -118,15 +118,13 @@ static inline bool init_task_stack_addr(const void *addr)
>  
>  static void print_track(struct kasan_track *track)
>  {
> -	pr_err("PID = %u\n", track->pid);
> -	if (track->stack) {
> -		struct stack_trace trace;
> +	struct stack_trace trace;
>  
> -		depot_fetch_stack(track->stack, &trace);
> +	pr_err("PID = %u\n", track->pid);
> +	if (depot_fetch_stack(track->stack, &trace))
>  		print_stack_trace(&trace, 0);
> -	} else {
> +	else
>  		pr_err("(stack is not available)\n");
> -	}
>  }
>  
>  static void kasan_object_err(struct kmem_cache *cache, struct page *page,
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 8fa5083..1862f05 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -252,10 +252,11 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>  	if (ret >= count)
>  		goto err;
>  
> -	depot_fetch_stack(handle, &trace);
> -	ret += snprint_stack_trace(kbuf + ret, count - ret, &trace, 0);
> -	if (ret >= count)
> -		goto err;
> +	if (depot_fetch_stack(handle, &trace)) {
> +		ret += snprint_stack_trace(kbuf + ret, count - ret, &trace, 0);
> +		if (ret >= count)
> +			goto err;
> +	}

Please do 'goto err' if depot_fetch_stack() return false here.

Others looks fine to me.

Thanks.