| Message ID | 20191112065302.7015-1-walter-zh.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix the missing underflow in memory operation function | expand |
On Tue, 2019-11-12 at 14:53 +0800, Walter Wu wrote: > KASAN missed detecting size is a negative number in memset(), memcpy(), > and memmove(), it will cause out-of-bounds bug. So needs to be detected > by KASAN. > > If size is a negative number, then it has a reason to be defined as > out-of-bounds bug type. > 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. > > KASAN report is shown below: > > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0 > Read of size 18446744073709551608 at addr ffffff8069660904 by task cat/72 > > CPU: 2 PID: 72 Comm: cat Not tainted 5.4.0-rc1-next-20191004ajb-00001-gdb8af2f372b2-dirty #1 > Hardware name: linux,dummy-virt (DT) > Call trace: > dump_backtrace+0x0/0x288 > show_stack+0x14/0x20 > dump_stack+0x10c/0x164 > print_address_description.isra.9+0x68/0x378 > __kasan_report+0x164/0x1a0 > kasan_report+0xc/0x18 > check_memory_region+0x174/0x1d0 > memmove+0x34/0x88 > kmalloc_memmove_invalid_size+0x70/0xa0 > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341 > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Suggested-by: Dmitry Vyukov <dvyukov@google.com> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: Alexander Potapenko <glider@google.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > include/linux/kasan.h | 2 +- > mm/kasan/common.c | 25 ++++++++++++++++++------- > mm/kasan/generic.c | 9 +++++---- > mm/kasan/generic_report.c | 11 +++++++++++ > mm/kasan/kasan.h | 2 +- > mm/kasan/report.c | 5 +---- > mm/kasan/tags.c | 9 +++++---- > mm/kasan/tags_report.c | 11 +++++++++++ > 8 files changed, 53 insertions(+), 21 deletions(-) > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index cc8a03cc9674..2ef6b8fc63ef 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -180,7 +180,7 @@ void kasan_init_tags(void); > > void *kasan_reset_tag(const void *addr); > > -void kasan_report(unsigned long addr, size_t size, > +bool kasan_report(unsigned long addr, size_t size, > bool is_write, unsigned long ip); > > #else /* CONFIG_KASAN_SW_TAGS */ > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 6814d6d6a023..4bfce0af881f 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write); > #undef memset > void *memset(void *addr, int c, size_t len) > { > - check_memory_region((unsigned long)addr, len, true, _RET_IP_); > + if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_)) > + return NULL; > > return __memset(addr, c, len); > } > @@ -110,8 +111,9 @@ 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 (!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 +121,9 @@ 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 (!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); > } > @@ -627,12 +630,20 @@ void kasan_free_shadow(const struct vm_struct *vm) > } > > extern void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip); > +extern bool report_enabled(void); > > -void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) > +bool kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) > { > - unsigned long flags = user_access_save(); > + unsigned long flags; > + > + if (likely(!report_enabled())) > + return false; > + > + flags = user_access_save(); > __kasan_report(addr, size, is_write, ip); > user_access_restore(flags); > + > + return true; > } > > #ifdef CONFIG_MEMORY_HOTPLUG > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c > index 616f9dd82d12..56ff8885fe2e 100644 > --- a/mm/kasan/generic.c > +++ b/mm/kasan/generic.c > @@ -173,17 +173,18 @@ static __always_inline bool check_memory_region_inline(unsigned long addr, > if (unlikely(size == 0)) > return true; > > + if (unlikely(addr + size < addr)) > + return !kasan_report(addr, size, write, ret_ip); > + > if (unlikely((void *)addr < > kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) { > - kasan_report(addr, size, write, ret_ip); > - return false; > + return !kasan_report(addr, size, write, ret_ip); > } > > if (likely(!memory_is_poisoned(addr, size))) > return true; > > - kasan_report(addr, size, write, ret_ip); > - return false; > + return !kasan_report(addr, size, write, ret_ip); > } > > bool check_memory_region(unsigned long addr, size_t size, bool write, > diff --git a/mm/kasan/generic_report.c b/mm/kasan/generic_report.c > index 36c645939bc9..c82bc3f52c9a 100644 > --- a/mm/kasan/generic_report.c > +++ b/mm/kasan/generic_report.c > @@ -107,6 +107,17 @@ 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 a negative number, then it has reason to be > + * defined as out-of-bounds bug type. > + * > + * 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. > + */ > + if (info->access_addr + info->access_size < info->access_addr) > + return "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/kasan.h b/mm/kasan/kasan.h > index 35cff6bbb716..afada2ce14bf 100644 > --- a/mm/kasan/kasan.h > +++ b/mm/kasan/kasan.h > @@ -152,7 +152,7 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, > void *find_first_bad_addr(void *addr, size_t size); > const char *get_bug_type(struct kasan_access_info *info); > > -void kasan_report(unsigned long addr, size_t size, > +bool kasan_report(unsigned long addr, size_t size, > bool is_write, unsigned long ip); > void kasan_report_invalid_free(void *object, unsigned long ip); > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index 621782100eaa..c94f8e9c78d4 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; > @@ -478,9 +478,6 @@ void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned lon > void *untagged_addr; > unsigned long flags; > > - if (likely(!report_enabled())) > - return; > - > disable_trace_on_warning(); > > tagged_addr = (void *)addr; > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c > index 0e987c9ca052..25b7734e7013 100644 > --- a/mm/kasan/tags.c > +++ b/mm/kasan/tags.c > @@ -86,6 +86,9 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, > if (unlikely(size == 0)) > return true; > > + if (unlikely(addr + size < addr)) > + return !kasan_report(addr, size, write, ret_ip); > + > tag = get_tag((const void *)addr); > > /* > @@ -111,15 +114,13 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, > untagged_addr = reset_tag((const void *)addr); > if (unlikely(untagged_addr < > kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) { > - kasan_report(addr, size, write, ret_ip); > - return false; > + return !kasan_report(addr, size, write, ret_ip); > } > shadow_first = kasan_mem_to_shadow(untagged_addr); > shadow_last = kasan_mem_to_shadow(untagged_addr + size - 1); > for (shadow = shadow_first; shadow <= shadow_last; shadow++) { > if (*shadow != tag) { > - kasan_report(addr, size, write, ret_ip); > - return false; > + return !kasan_report(addr, size, write, ret_ip); > } > } > > diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c > index 969ae08f59d7..1d412760551a 100644 > --- a/mm/kasan/tags_report.c > +++ b/mm/kasan/tags_report.c > @@ -36,6 +36,17 @@ > > const char *get_bug_type(struct kasan_access_info *info) > { > + /* > + * If access_size is a negative number, then it has reason to be > + * defined as out-of-bounds bug type. > + * > + * 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. > + */ > + if (info->access_addr + info->access_size < info->access_addr) > + return "out-of-bounds"; > + > #ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY > struct kasan_alloc_meta *alloc_meta; > struct kmem_cache *cache; Hi Andrey, Would you have any concerns? Thanks. Walter
On 11/12/19 9:53 AM, Walter Wu wrote: > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 6814d6d6a023..4bfce0af881f 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write); > #undef memset > void *memset(void *addr, int c, size_t len) > { > - check_memory_region((unsigned long)addr, len, true, _RET_IP_); > + if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_)) > + return NULL; > > return __memset(addr, c, len); > } > @@ -110,8 +111,9 @@ 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 (!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 +121,9 @@ 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 (!check_memory_region((unsigned long)src, len, false, _RET_IP_) || > + !check_memory_region((unsigned long)dest, len, true, _RET_IP_)) > + return NULL; > I realized that we are going a wrong direction here. Entirely skipping mem*() operation on any poisoned shadow value might only make things worse. Some bugs just don't have any serious consequences, but skipping the mem*() ops entirely might introduce such consequences, which wouldn't happen otherwise. So let's keep this code as this, no need to check the result of check_memory_region().
On Thu, 2019-11-21 at 15:26 +0300, Andrey Ryabinin wrote: > > On 11/12/19 9:53 AM, Walter Wu wrote: > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > > index 6814d6d6a023..4bfce0af881f 100644 > > --- a/mm/kasan/common.c > > +++ b/mm/kasan/common.c > > @@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write); > > #undef memset > > void *memset(void *addr, int c, size_t len) > > { > > - check_memory_region((unsigned long)addr, len, true, _RET_IP_); > > + if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_)) > > + return NULL; > > > > return __memset(addr, c, len); > > } > > @@ -110,8 +111,9 @@ 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 (!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 +121,9 @@ 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 (!check_memory_region((unsigned long)src, len, false, _RET_IP_) || > > + !check_memory_region((unsigned long)dest, len, true, _RET_IP_)) > > + return NULL; > > > > I realized that we are going a wrong direction here. Entirely skipping mem*() operation on any > poisoned shadow value might only make things worse. Some bugs just don't have any serious consequences, > but skipping the mem*() ops entirely might introduce such consequences, which wouldn't happen otherwise. > > So let's keep this code as this, no need to check the result of check_memory_region(). > > Ok, we just need to determine whether size is negative number. If yes then KASAN produce report and continue to execute mem*(). right?
On 11/21/19 4:02 PM, Walter Wu wrote: > On Thu, 2019-11-21 at 15:26 +0300, Andrey Ryabinin wrote: >> >> On 11/12/19 9:53 AM, Walter Wu wrote: >> >>> diff --git a/mm/kasan/common.c b/mm/kasan/common.c >>> index 6814d6d6a023..4bfce0af881f 100644 >>> --- a/mm/kasan/common.c >>> +++ b/mm/kasan/common.c >>> @@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write); >>> #undef memset >>> void *memset(void *addr, int c, size_t len) >>> { >>> - check_memory_region((unsigned long)addr, len, true, _RET_IP_); >>> + if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_)) >>> + return NULL; >>> >>> return __memset(addr, c, len); >>> } >>> @@ -110,8 +111,9 @@ 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 (!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 +121,9 @@ 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 (!check_memory_region((unsigned long)src, len, false, _RET_IP_) || >>> + !check_memory_region((unsigned long)dest, len, true, _RET_IP_)) >>> + return NULL; >>> >> >> I realized that we are going a wrong direction here. Entirely skipping mem*() operation on any >> poisoned shadow value might only make things worse. Some bugs just don't have any serious consequences, >> but skipping the mem*() ops entirely might introduce such consequences, which wouldn't happen otherwise. >> >> So let's keep this code as this, no need to check the result of check_memory_region(). >> >> > Ok, we just need to determine whether size is negative number. If yes > then KASAN produce report and continue to execute mem*(). right? > Yes.
On Thu, 2019-11-21 at 16:03 +0300, Andrey Ryabinin wrote: > > On 11/21/19 4:02 PM, Walter Wu wrote: > > On Thu, 2019-11-21 at 15:26 +0300, Andrey Ryabinin wrote: > >> > >> On 11/12/19 9:53 AM, Walter Wu wrote: > >> > >>> diff --git a/mm/kasan/common.c b/mm/kasan/common.c > >>> index 6814d6d6a023..4bfce0af881f 100644 > >>> --- a/mm/kasan/common.c > >>> +++ b/mm/kasan/common.c > >>> @@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write); > >>> #undef memset > >>> void *memset(void *addr, int c, size_t len) > >>> { > >>> - check_memory_region((unsigned long)addr, len, true, _RET_IP_); > >>> + if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_)) > >>> + return NULL; > >>> > >>> return __memset(addr, c, len); > >>> } > >>> @@ -110,8 +111,9 @@ 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 (!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 +121,9 @@ 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 (!check_memory_region((unsigned long)src, len, false, _RET_IP_) || > >>> + !check_memory_region((unsigned long)dest, len, true, _RET_IP_)) > >>> + return NULL; > >>> > >> > >> I realized that we are going a wrong direction here. Entirely skipping mem*() operation on any > >> poisoned shadow value might only make things worse. Some bugs just don't have any serious consequences, > >> but skipping the mem*() ops entirely might introduce such consequences, which wouldn't happen otherwise. > >> > >> So let's keep this code as this, no need to check the result of check_memory_region(). > >> > >> > > Ok, we just need to determine whether size is negative number. If yes > > then KASAN produce report and continue to execute mem*(). right? > > > > Yes. Thanks for your suggestion. I will send a new v5 patch tomorrow. Walter
On Thu, Nov 21, 2019 at 1:27 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > > index 6814d6d6a023..4bfce0af881f 100644 > > --- a/mm/kasan/common.c > > +++ b/mm/kasan/common.c > > @@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write); > > #undef memset > > void *memset(void *addr, int c, size_t len) > > { > > - check_memory_region((unsigned long)addr, len, true, _RET_IP_); > > + if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_)) > > + return NULL; > > > > return __memset(addr, c, len); > > } > > @@ -110,8 +111,9 @@ 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 (!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 +121,9 @@ 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 (!check_memory_region((unsigned long)src, len, false, _RET_IP_) || > > + !check_memory_region((unsigned long)dest, len, true, _RET_IP_)) > > + return NULL; > > > > I realized that we are going a wrong direction here. Entirely skipping mem*() operation on any > poisoned shadow value might only make things worse. Some bugs just don't have any serious consequences, > but skipping the mem*() ops entirely might introduce such consequences, which wouldn't happen otherwise. > > So let's keep this code as this, no need to check the result of check_memory_region(). I suggested it. For our production runs it won't matter, we always panic on first report. If one does not panic, there is no right answer. You say: _some_ bugs don't have any serious consequences, but skipping the mem*() ops entirely might introduce such consequences. The opposite is true as well, right? :) And it's not hard to come up with a scenario where overwriting memory after free or out of bounds badly corrupts memory. I don't think we can somehow magically avoid bad consequences in all cases. What I was thinking about is tests. We need tests for this. And we tried to construct tests specifically so that they don't badly corrupt memory (e.g. OOB/UAF reads, or writes to unused redzones, etc), so that it's possible to run all of them to completion reliably. Skipping the actual memory options allows to write such tests for all possible scenarios. That's was my motivation.
On 11/21/19 10:58 PM, Dmitry Vyukov wrote: > On Thu, Nov 21, 2019 at 1:27 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: >>> diff --git a/mm/kasan/common.c b/mm/kasan/common.c >>> index 6814d6d6a023..4bfce0af881f 100644 >>> --- a/mm/kasan/common.c >>> +++ b/mm/kasan/common.c >>> @@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write); >>> #undef memset >>> void *memset(void *addr, int c, size_t len) >>> { >>> - check_memory_region((unsigned long)addr, len, true, _RET_IP_); >>> + if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_)) >>> + return NULL; >>> >>> return __memset(addr, c, len); >>> } >>> @@ -110,8 +111,9 @@ 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 (!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 +121,9 @@ 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 (!check_memory_region((unsigned long)src, len, false, _RET_IP_) || >>> + !check_memory_region((unsigned long)dest, len, true, _RET_IP_)) >>> + return NULL; >>> >> >> I realized that we are going a wrong direction here. Entirely skipping mem*() operation on any >> poisoned shadow value might only make things worse. Some bugs just don't have any serious consequences, >> but skipping the mem*() ops entirely might introduce such consequences, which wouldn't happen otherwise. >> >> So let's keep this code as this, no need to check the result of check_memory_region(). > > I suggested it. > > For our production runs it won't matter, we always panic on first report. > If one does not panic, there is no right answer. You say: _some_ bugs > don't have any serious consequences, but skipping the mem*() ops > entirely might introduce such consequences. The opposite is true as > well, right? :) And it's not hard to come up with a scenario where > overwriting memory after free or out of bounds badly corrupts memory. > I don't think we can somehow magically avoid bad consequences in all > cases. > Absolutely right. My point was that if it's bad consequences either way, than there is no point in complicating this code, it doesn't buy us anything. > What I was thinking about is tests. We need tests for this. And we > tried to construct tests specifically so that they don't badly corrupt > memory (e.g. OOB/UAF reads, or writes to unused redzones, etc), so > that it's possible to run all of them to completion reliably. Skipping > the actual memory options allows to write such tests for all possible > scenarios. That's was my motivation. But I see you point now. No objections to the patch in that case.
On 11/12/19 9:53 AM, Walter Wu wrote: > KASAN missed detecting size is a negative number in memset(), memcpy(), > and memmove(), it will cause out-of-bounds bug. So needs to be detected > by KASAN. > > If size is a negative number, then it has a reason to be defined as > out-of-bounds bug type. > 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. > > KASAN report is shown below: > > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0 > Read of size 18446744073709551608 at addr ffffff8069660904 by task cat/72 > > CPU: 2 PID: 72 Comm: cat Not tainted 5.4.0-rc1-next-20191004ajb-00001-gdb8af2f372b2-dirty #1 > Hardware name: linux,dummy-virt (DT) > Call trace: > dump_backtrace+0x0/0x288 > show_stack+0x14/0x20 > dump_stack+0x10c/0x164 > print_address_description.isra.9+0x68/0x378 > __kasan_report+0x164/0x1a0 > kasan_report+0xc/0x18 > check_memory_region+0x174/0x1d0 > memmove+0x34/0x88 > kmalloc_memmove_invalid_size+0x70/0xa0 > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341 > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Suggested-by: Dmitry Vyukov <dvyukov@google.com> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: Alexander Potapenko <glider@google.com> > Reported-by: kernel test robot <lkp@intel.com> > --- Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
On Fri, 2019-11-22 at 01:20 +0300, Andrey Ryabinin wrote: > > On 11/12/19 9:53 AM, Walter Wu wrote: > > KASAN missed detecting size is a negative number in memset(), memcpy(), > > and memmove(), it will cause out-of-bounds bug. So needs to be detected > > by KASAN. > > > > If size is a negative number, then it has a reason to be defined as > > out-of-bounds bug type. > > 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. > > > > KASAN report is shown below: > > > > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0 > > Read of size 18446744073709551608 at addr ffffff8069660904 by task cat/72 > > > > CPU: 2 PID: 72 Comm: cat Not tainted 5.4.0-rc1-next-20191004ajb-00001-gdb8af2f372b2-dirty #1 > > Hardware name: linux,dummy-virt (DT) > > Call trace: > > dump_backtrace+0x0/0x288 > > show_stack+0x14/0x20 > > dump_stack+0x10c/0x164 > > print_address_description.isra.9+0x68/0x378 > > __kasan_report+0x164/0x1a0 > > kasan_report+0xc/0x18 > > check_memory_region+0x174/0x1d0 > > memmove+0x34/0x88 > > kmalloc_memmove_invalid_size+0x70/0xa0 > > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341 > > > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com> > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > > Suggested-by: Dmitry Vyukov <dvyukov@google.com> > > Reviewed-by: Dmitry Vyukov <dvyukov@google.com> > > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > > Cc: Alexander Potapenko <glider@google.com> > > Reported-by: kernel test robot <lkp@intel.com> > > --- > > Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com> Hi Andrey, Dmitry, Thanks for your review and suggestion. Walter
diff --git a/include/linux/kasan.h b/include/linux/kasan.h index cc8a03cc9674..2ef6b8fc63ef 100644 --- a/include/linux/kasan.h +++ b/include/linux/kasan.h @@ -180,7 +180,7 @@ void kasan_init_tags(void); void *kasan_reset_tag(const void *addr); -void kasan_report(unsigned long addr, size_t size, +bool kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip); #else /* CONFIG_KASAN_SW_TAGS */ diff --git a/mm/kasan/common.c b/mm/kasan/common.c index 6814d6d6a023..4bfce0af881f 100644 --- a/mm/kasan/common.c +++ b/mm/kasan/common.c @@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write); #undef memset void *memset(void *addr, int c, size_t len) { - check_memory_region((unsigned long)addr, len, true, _RET_IP_); + if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_)) + return NULL; return __memset(addr, c, len); } @@ -110,8 +111,9 @@ 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 (!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 +121,9 @@ 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 (!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); } @@ -627,12 +630,20 @@ void kasan_free_shadow(const struct vm_struct *vm) } extern void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip); +extern bool report_enabled(void); -void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) +bool kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) { - unsigned long flags = user_access_save(); + unsigned long flags; + + if (likely(!report_enabled())) + return false; + + flags = user_access_save(); __kasan_report(addr, size, is_write, ip); user_access_restore(flags); + + return true; } #ifdef CONFIG_MEMORY_HOTPLUG diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c index 616f9dd82d12..56ff8885fe2e 100644 --- a/mm/kasan/generic.c +++ b/mm/kasan/generic.c @@ -173,17 +173,18 @@ static __always_inline bool check_memory_region_inline(unsigned long addr, if (unlikely(size == 0)) return true; + if (unlikely(addr + size < addr)) + return !kasan_report(addr, size, write, ret_ip); + if (unlikely((void *)addr < kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) { - kasan_report(addr, size, write, ret_ip); - return false; + return !kasan_report(addr, size, write, ret_ip); } if (likely(!memory_is_poisoned(addr, size))) return true; - kasan_report(addr, size, write, ret_ip); - return false; + return !kasan_report(addr, size, write, ret_ip); } bool check_memory_region(unsigned long addr, size_t size, bool write, diff --git a/mm/kasan/generic_report.c b/mm/kasan/generic_report.c index 36c645939bc9..c82bc3f52c9a 100644 --- a/mm/kasan/generic_report.c +++ b/mm/kasan/generic_report.c @@ -107,6 +107,17 @@ 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 a negative number, then it has reason to be + * defined as out-of-bounds bug type. + * + * 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. + */ + if (info->access_addr + info->access_size < info->access_addr) + return "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/kasan.h b/mm/kasan/kasan.h index 35cff6bbb716..afada2ce14bf 100644 --- a/mm/kasan/kasan.h +++ b/mm/kasan/kasan.h @@ -152,7 +152,7 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, void *find_first_bad_addr(void *addr, size_t size); const char *get_bug_type(struct kasan_access_info *info); -void kasan_report(unsigned long addr, size_t size, +bool kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip); void kasan_report_invalid_free(void *object, unsigned long ip); diff --git a/mm/kasan/report.c b/mm/kasan/report.c index 621782100eaa..c94f8e9c78d4 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; @@ -478,9 +478,6 @@ void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned lon void *untagged_addr; unsigned long flags; - if (likely(!report_enabled())) - return; - disable_trace_on_warning(); tagged_addr = (void *)addr; diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c index 0e987c9ca052..25b7734e7013 100644 --- a/mm/kasan/tags.c +++ b/mm/kasan/tags.c @@ -86,6 +86,9 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, if (unlikely(size == 0)) return true; + if (unlikely(addr + size < addr)) + return !kasan_report(addr, size, write, ret_ip); + tag = get_tag((const void *)addr); /* @@ -111,15 +114,13 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, untagged_addr = reset_tag((const void *)addr); if (unlikely(untagged_addr < kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) { - kasan_report(addr, size, write, ret_ip); - return false; + return !kasan_report(addr, size, write, ret_ip); } shadow_first = kasan_mem_to_shadow(untagged_addr); shadow_last = kasan_mem_to_shadow(untagged_addr + size - 1); for (shadow = shadow_first; shadow <= shadow_last; shadow++) { if (*shadow != tag) { - kasan_report(addr, size, write, ret_ip); - return false; + return !kasan_report(addr, size, write, ret_ip); } } diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c index 969ae08f59d7..1d412760551a 100644 --- a/mm/kasan/tags_report.c +++ b/mm/kasan/tags_report.c @@ -36,6 +36,17 @@ const char *get_bug_type(struct kasan_access_info *info) { + /* + * If access_size is a negative number, then it has reason to be + * defined as out-of-bounds bug type. + * + * 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. + */ + if (info->access_addr + info->access_size < info->access_addr) + return "out-of-bounds"; + #ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY struct kasan_alloc_meta *alloc_meta; struct kmem_cache *cache;