Re: [PATCH v3 1/2] kasan: detect negative size in memory operation function

[Andrey Ryabinin]

On 11/4/19 5:05 AM, Walter Wu wrote:

> 
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 6814d6d6a023..4ff67e2fd2db 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -99,10 +99,14 @@ bool __kasan_check_write(const volatile void *p, unsigned int size)
>  }
>  EXPORT_SYMBOL(__kasan_check_write);
>  
> +extern bool report_enabled(void);
> +
>  #undef memset
>  void *memset(void *addr, int c, size_t len)
>  {
> -	check_memory_region((unsigned long)addr, len, true, _RET_IP_);
> +	if (report_enabled() &&
> +	    !check_memory_region((unsigned long)addr, len, true, _RET_IP_))
> +		return NULL;
>  
>  	return __memset(addr, c, len);
>  }
> @@ -110,8 +114,10 @@ void *memset(void *addr, int c, size_t len)
>  #undef memmove
>  void *memmove(void *dest, const void *src, size_t len)
>  {
> -	check_memory_region((unsigned long)src, len, false, _RET_IP_);
> -	check_memory_region((unsigned long)dest, len, true, _RET_IP_);
> +	if (report_enabled() &&
> +	   (!check_memory_region((unsigned long)src, len, false, _RET_IP_) ||
> +	    !check_memory_region((unsigned long)dest, len, true, _RET_IP_)))
> +		return NULL;
>  
>  	return __memmove(dest, src, len);
>  }
> @@ -119,8 +125,10 @@ void *memmove(void *dest, const void *src, size_t len)
>  #undef memcpy
>  void *memcpy(void *dest, const void *src, size_t len)
>  {
> -	check_memory_region((unsigned long)src, len, false, _RET_IP_);
> -	check_memory_region((unsigned long)dest, len, true, _RET_IP_);
> +	if (report_enabled() &&

            report_enabled() checks seems to be useless.

> +	   (!check_memory_region((unsigned long)src, len, false, _RET_IP_) ||
> +	    !check_memory_region((unsigned long)dest, len, true, _RET_IP_)))
> +		return NULL;
>  
>  	return __memcpy(dest, src, len);
>  }
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 616f9dd82d12..02148a317d27 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -173,6 +173,11 @@ static __always_inline bool check_memory_region_inline(unsigned long addr,
>  	if (unlikely(size == 0))
>  		return true;
>  
> +	if (unlikely((long)size < 0)) {

        if (unlikely(addr + size < addr)) {

> +		kasan_report(addr, size, write, ret_ip);
> +		return false;
> +	}
> +
>  	if (unlikely((void *)addr <
>  		kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) {
>  		kasan_report(addr, size, write, ret_ip);
> diff --git a/mm/kasan/generic_report.c b/mm/kasan/generic_report.c
> index 36c645939bc9..52a92c7db697 100644
> --- a/mm/kasan/generic_report.c
> +++ b/mm/kasan/generic_report.c
> @@ -107,6 +107,24 @@ static const char *get_wild_bug_type(struct kasan_access_info *info)
>  
>  const char *get_bug_type(struct kasan_access_info *info)
>  {
> +	/*
> +	 * If access_size is negative numbers, then it has three reasons
> +	 * to be defined as heap-out-of-bounds bug type.
> +	 * 1) Casting negative numbers to size_t would indeed turn up as
> +	 *    a large size_t and its value will be larger than ULONG_MAX/2,
> +	 *    so that this can qualify as out-of-bounds.
> +	 * 2) If KASAN has new bug type and user-space passes negative size,
> +	 *    then there are duplicate reports. So don't produce new bug type
> +	 *    in order to prevent duplicate reports by some systems
> +	 *    (e.g. syzbot) to report the same bug twice.
> +	 * 3) When size is negative numbers, it may be passed from user-space.
> +	 *    So we always print heap-out-of-bounds in order to prevent that
> +	 *    kernel-space and user-space have the same bug but have duplicate
> +	 *    reports.
> +	 */
 
Completely fail to understand 2) and 3). 2) talks something about *NOT* producing new bug
type, but at the same time you code actually does that.
3) says something about user-space which have nothing to do with kasan.

> +	if ((long)info->access_size < 0)

        if (info->access_addr + info->access_size < info->access_addr)

> +		return "heap-out-of-bounds";
> +
>  	if (addr_has_shadow(info->access_addr))
>  		return get_shadow_bug_type(info);
>  	return get_wild_bug_type(info);
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 621782100eaa..c79e28814e8f 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -446,7 +446,7 @@ static void print_shadow_for_address(const void *addr)
>  	}
>  }
>  
> -static bool report_enabled(void)
> +bool report_enabled(void)
>  {
>  	if (current->kasan_depth)
>  		return false;
> diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> index 0e987c9ca052..b829535a3ad7 100644
> --- a/mm/kasan/tags.c
> +++ b/mm/kasan/tags.c
> @@ -86,6 +86,11 @@ bool check_memory_region(unsigned long addr, size_t size, bool write,
>  	if (unlikely(size == 0))
>  		return true;
>  
> +	if (unlikely((long)size < 0)) {

        if (unlikely(addr + size < addr)) {

> +		kasan_report(addr, size, write, ret_ip);
> +		return false;
> +	}
> +
>  	tag = get_tag((const void *)addr);
>  
>  	/*
> diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c
> index 969ae08f59d7..f7ae474aef3a 100644
> --- a/mm/kasan/tags_report.c
> +++ b/mm/kasan/tags_report.c
> @@ -36,6 +36,24 @@
>  
>  const char *get_bug_type(struct kasan_access_info *info)
>  {
> +	/*
> +	 * If access_size is negative numbers, then it has three reasons
> +	 * to be defined as heap-out-of-bounds bug type.
> +	 * 1) Casting negative numbers to size_t would indeed turn up as
> +	 *    a large size_t and its value will be larger than ULONG_MAX/2,
> +	 *    so that this can qualify as out-of-bounds.
> +	 * 2) If KASAN has new bug type and user-space passes negative size,
> +	 *    then there are duplicate reports. So don't produce new bug type
> +	 *    in order to prevent duplicate reports by some systems
> +	 *    (e.g. syzbot) to report the same bug twice.
> +	 * 3) When size is negative numbers, it may be passed from user-space.
> +	 *    So we always print heap-out-of-bounds in order to prevent that
> +	 *    kernel-space and user-space have the same bug but have duplicate
> +	 *    reports.
> +	 */
> +	if ((long)info->access_size < 0)

        if (info->access_addr + info->access_size < info->access_addr)

> +		return "heap-out-of-bounds";
> +
>  #ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
>  	struct kasan_alloc_meta *alloc_meta;
>  	struct kmem_cache *cache;
>