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

[Dmitry Vyukov]

On Fri, Nov 8, 2019 at 11:32 PM Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> 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.

The idea was to use one of the existing bug titles so that syzbot does
not produce 2 versions for OOBs where size is user-controlled. We
don't know if it's overflow from heap, global or stack, but heap is
the most common bug, so saying heap overflow will reduce chances of
producing duplicates the most.
But for all of this to work we do need to use one of the existing bug titles.

> > +     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;
> >