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

[Walter Wu]

On Mon, 2019-11-11 at 11:17 +0100, Dmitry Vyukov wrote:
> On Mon, Nov 11, 2019 at 11:12 AM Walter Wu <walter-zh.wu@xxxxxxxxxxxx> wrote:
> > > On 11/11/19 10:14 AM, Walter Wu wrote:
> > > > On Sat, 2019-11-09 at 01:31 +0300, Andrey Ryabinin wrote:
> > > >>
> > > >> 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.
> > > >>
> > > >
> > > > Hi Andrey,
> > > >
> > > > If it doesn't have report_enable(), then it will have below the error.
> > > > We think it should be x86 shadow memory is invalid value before KASAN
> > > > initialized, it will have some misjudgments to do directly return when
> > > > it detects invalid shadow value in memset()/memcpy()/memmove(). So we
> > > > add report_enable() to avoid this happening. but we should only use the
> > > > condition "current->kasan_depth == 0" to determine if KASAN is
> > > > initialized. And we try it is pass at x86.
> > > >
> > >
> > > Ok, I see. It just means that check_memory_region() return incorrect result in early stages of boot.
> > > So, the right way to deal with this would be making kasan_report() to return bool ("false" if no report and "true" if reported)
> > > and propagate this return value up to check_memory_region().
> > >
> > This changes in v4.
> >
> > >
> > > >>> 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.
> > > >>
> > > > about 2)
> > > > We originally think the heap-out-of-bounds is similar to
> > > > heap-buffer-overflow, maybe we should change the bug type to
> > > > heap-buffer-overflow.
> > >
> > > There is no "heap-buffer-overflow".
> > >
> > If I remember correctly, "heap-buffer-overflow" is one of existing bug
> > type in user-space? Or you want to expect to see an existing bug type in
> > kernel space?
> 
> Existing bug in KASAN.
> KASAN and ASAN bugs will never match regardless of what we do. They
> are simply in completely different code. So aligning titles between
> kernel and userspace will not lead to any better deduplication.

Ok, it seems like to print "out-of-bounds". Simple and easy to know it.
Thanks Dmitry.