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 <[email protected]>
Reported-by: Dmitry Vyukov <[email protected]>
Suggested-by: Dmitry Vyukov <[email protected]>
Reviewed-by: Dmitry Vyukov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Alexander Potapenko <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
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;
--
2.18.0
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 <[email protected]>
> Reported-by: Dmitry Vyukov <[email protected]>
> Suggested-by: Dmitry Vyukov <[email protected]>
> Reviewed-by: Dmitry Vyukov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> ---
> 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 <[email protected]> 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/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 <[email protected]>
> Reported-by: Dmitry Vyukov <[email protected]>
> Suggested-by: Dmitry Vyukov <[email protected]>
> Reviewed-by: Dmitry Vyukov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> ---
Reviewed-by: Andrey Ryabinin <[email protected]>
On 11/21/19 10:58 PM, Dmitry Vyukov wrote:
> On Thu, Nov 21, 2019 at 1:27 PM Andrey Ryabinin <[email protected]> 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 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 <[email protected]>
> > Reported-by: Dmitry Vyukov <[email protected]>
> > Suggested-by: Dmitry Vyukov <[email protected]>
> > Reviewed-by: Dmitry Vyukov <[email protected]>
> > Cc: Andrey Ryabinin <[email protected]>
> > Cc: Alexander Potapenko <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > ---
>
> Reviewed-by: Andrey Ryabinin <[email protected]>
Hi Andrey, Dmitry,
Thanks for your review and suggestion.
Walter