Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Tue, 2019-10-08 at 14:11 +0200, Dmitry Vyukov wrote: > On Tue, Oct 8, 2019 at 1:42 PM Qian Cai wrote: > > > On Oct 8, 2019, at 7:02 AM, Walter Wu wrote: > > > I don't know very well in UBSAN, but I try to build ubsan kernel and > > > test a negative number in memset and kmalloc_memmove_invalid_size(), it > > > look like no check. > > > > It sounds like more important to figure out why the UBSAN is not working in > > this case rather than duplicating functionality elsewhere. > > Detecting out-of-bounds accesses is the direct KASAN responsibility. > Even more direct than for KUBSAN. We are not even adding > functionality, it's just a plain bug in KASAN code, it tricks itself > into thinking that access size is 0. > Maybe it's already detected by KUBSAN too? Thanks for your response. I survey the KUBSAN, it don't check size is negative in memset/memcpy/memmove, we try to verify our uni testing too, it don't report the bug in KUBSAN, so it needs to report this bug by KASAN. The reason is like what you said. so we still send the patch.
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Tue, Oct 8, 2019 at 1:42 PM Qian Cai wrote: > > On Oct 8, 2019, at 7:02 AM, Walter Wu wrote: > > I don't know very well in UBSAN, but I try to build ubsan kernel and > > test a negative number in memset and kmalloc_memmove_invalid_size(), it > > look like no check. > > It sounds like more important to figure out why the UBSAN is not working in > this case rather than duplicating functionality elsewhere. Detecting out-of-bounds accesses is the direct KASAN responsibility. Even more direct than for KUBSAN. We are not even adding functionality, it's just a plain bug in KASAN code, it tricks itself into thinking that access size is 0. Maybe it's already detected by KUBSAN too?
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Tue, 2019-10-08 at 07:42 -0400, Qian Cai wrote: > > > On Oct 8, 2019, at 7:02 AM, Walter Wu wrote: > > > > I don't know very well in UBSAN, but I try to build ubsan kernel and > > test a negative number in memset and kmalloc_memmove_invalid_size(), it > > look like no check. > > It sounds like more important to figure out why the UBSAN is not working in > this case rather than duplicating functionality elsewhere. Maybe we can let the maintainer and reviewer decide it :) And We want to say if size is negative numbers, it look like an out-of-bounds, too. so KASAN make sense to detect it.
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
> On Oct 8, 2019, at 7:02 AM, Walter Wu wrote: > > I don't know very well in UBSAN, but I try to build ubsan kernel and > test a negative number in memset and kmalloc_memmove_invalid_size(), it > look like no check. It sounds like more important to figure out why the UBSAN is not working in this case rather than duplicating functionality elsewhere.
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Tue, 2019-10-08 at 05:47 -0400, Qian Cai wrote: > > > On Oct 8, 2019, at 2:16 AM, Walter Wu wrote: > > > > It is an undefined behavior to pass a negative numbers to > >memset()/memcpy()/memmove(), so need to be detected by KASAN. > > Why can’t this be detected by UBSAN? I don't know very well in UBSAN, but I try to build ubsan kernel and test a negative number in memset and kmalloc_memmove_invalid_size(), it look like no check.
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
> On Oct 8, 2019, at 2:16 AM, Walter Wu wrote: > > It is an undefined behavior to pass a negative numbers to >memset()/memcpy()/memmove(), so need to be detected by KASAN. Why can’t this be detected by UBSAN?
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-10-07 at 15:33 +0200, Dmitry Vyukov wrote:
> On Mon, Oct 7, 2019 at 2:33 PM Walter Wu wrote:
> > On Mon, 2019-10-07 at 14:19 +0200, Dmitry Vyukov wrote:
> > > On Mon, Oct 7, 2019 at 2:03 PM Walter Wu
> > > wrote:
> > > My idea was just to always print "heap-out-of-bounds" and don't
> > > differentiate if the size come from userspace or not.
> >
> > Got it.
> > Would you have any other concern about this patch?
>
>
> Last versions of the patch looked good to me except for the bug title.
> The comment may also need some updating if you change the title.
Updated, thanks again again.
The patchsets help to produce KASAN report when size is negative numbers
in memory operation function. It is helpful for programmer to solve the
undefined behavior issue. Patch 1 based on Dmitry's review and
suggestion, patch 2 is a test in order to verify the patch 1.
[1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
[2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
Walter Wu (2):
kasan: detect invalid size in memory operation function
kasan: add test for invalid size in memmove
lib/test_kasan.c | 18 ++
mm/kasan/common.c | 13 -
mm/kasan/generic.c| 5 +
mm/kasan/generic_report.c | 18 ++
mm/kasan/tags.c | 5 +
mm/kasan/tags_report.c| 17 +
6 files changed, 71 insertions(+), 5 deletions(-)
commit 1eb58140ac67debabdca705bafaadea934eb7820
Author: Walter-zh Wu
Date: Fri Oct 4 18:38:31 2019 +0800
kasan: detect negative size in memory operation function
It is an undefined behavior to pass a negative numbers to
memset()/memcpy()/memmove(), so need to be detected by KASAN.
If 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.
KASAN report:
BUG: KASAN: heap-out-of-bounds in kmalloc_memmove_invalid_size
+0x70/0xa0
Read of size 18446744073709551608 at addr ff8069660904 by task
cat/72
CPU: 2 PID: 72 Comm: cat Not tainted
5.4.0-rc1-next-20191004ajb-1-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
Reported -by: Dmitry Vyukov
Suggested-by: Dmitry Vyukov
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 6814d6d6a023..6ef0abd27f06 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);
}
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 616f9dd82d12..02148a317d27 100644
---
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, Oct 7, 2019 at 2:33 PM Walter Wu wrote: > On Mon, 2019-10-07 at 14:19 +0200, Dmitry Vyukov wrote: > > On Mon, Oct 7, 2019 at 2:03 PM Walter Wu wrote: > > My idea was just to always print "heap-out-of-bounds" and don't > > differentiate if the size come from userspace or not. > > Got it. > Would you have any other concern about this patch? Last versions of the patch looked good to me except for the bug title. The comment may also need some updating if you change the title.
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-10-07 at 14:19 +0200, Dmitry Vyukov wrote:
> On Mon, Oct 7, 2019 at 2:03 PM Walter Wu wrote:
> > > > > > > > > > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu
> > > > > > > > > > wrote:
> > > > > > > > > > > The patchsets help to produce KASAN report when size is
> > > > > > > > > > > negative numbers
> > > > > > > > > > > in memory operation function. It is helpful for
> > > > > > > > > > > programmer to solve the
> > > > > > > > > > > undefined behavior issue. Patch 1 based on Dmitry's
> > > > > > > > > > > review and
> > > > > > > > > > > suggestion, patch 2 is a test in order to verify the
> > > > > > > > > > > patch 1.
> > > > > > > > > > >
> > > > > > > > > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > > > > > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > > > > > > > > > >
> > > > > > > > > > > Walter Wu (2):
> > > > > > > > > > > kasan: detect invalid size in memory operation function
> > > > > > > > > > > kasan: add test for invalid size in memmove
> > > > > > > > > > >
> > > > > > > > > > > lib/test_kasan.c | 18 ++
> > > > > > > > > > > mm/kasan/common.c | 13 -
> > > > > > > > > > > mm/kasan/generic.c| 5 +
> > > > > > > > > > > mm/kasan/generic_report.c | 12
> > > > > > > > > > > mm/kasan/tags.c | 5 +
> > > > > > > > > > > mm/kasan/tags_report.c| 12
> > > > > > > > > > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > > > > > > > > > Author: Walter-zh Wu
> > > > > > > > > > > Date: Fri Oct 4 18:38:31 2019 +0800
> > > > > > > > > > >
> > > > > > > > > > > kasan: detect invalid size in memory operation
> > > > > > > > > > > function
> > > > > > > > > > >
> > > > > > > > > > > It is an undefined behavior to pass a negative
> > > > > > > > > > > numbers to
> > > > > > > > > > > memset()/memcpy()/memmove()
> > > > > > > > > > > , so need to be detected by KASAN.
> > > > > > > > > > >
> > > > > > > > > > > If size is negative numbers, then it has two reasons
> > > > > > > > > > > to be defined
> > > > > > > > > > > as 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) Don't generate new bug type in order to prevent
> > > > > > > > > > > duplicate reports
> > > > > > > > > > > by
> > > > > > > > > > > some systems, e.g. syzbot.
> > > > > > > > > > >
> > > > > > > > > > > KASAN report:
> > > > > > > > > > >
> > > > > > > > > > > BUG: KASAN: out-of-bounds in
> > > > > > > > > > > kmalloc_memmove_invalid_size+0x70/0xa0
> > > > > > > > > > > Read of size 18446744073709551608 at addr
> > > > > > > > > > > ff8069660904 by task
> > > > > > > > > > > cat/72
> > > > > > > > > > >
> > > > > > > > > > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > > > > > > > > > 5.4.0-rc1-next-20191004ajb-1-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
> > > > > > > > > > > Reported -by: Dmitry Vyukov
> > > > > > > > > > > Suggested-by: Dmitry Vyukov
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > > > > > > > > index 6814d6d6a023..6ef0abd27f06 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,
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, Oct 7, 2019 at 2:03 PM Walter Wu wrote:
> > > > > > > > > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu
> > > > > > > > > wrote:
> > > > > > > > > > The patchsets help to produce KASAN report when size is
> > > > > > > > > > negative numbers
> > > > > > > > > > in memory operation function. It is helpful for programmer
> > > > > > > > > > to solve the
> > > > > > > > > > undefined behavior issue. Patch 1 based on Dmitry's review
> > > > > > > > > > and
> > > > > > > > > > suggestion, patch 2 is a test in order to verify the patch
> > > > > > > > > > 1.
> > > > > > > > > >
> > > > > > > > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > > > > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > > > > > > > > >
> > > > > > > > > > Walter Wu (2):
> > > > > > > > > > kasan: detect invalid size in memory operation function
> > > > > > > > > > kasan: add test for invalid size in memmove
> > > > > > > > > >
> > > > > > > > > > lib/test_kasan.c | 18 ++
> > > > > > > > > > mm/kasan/common.c | 13 -
> > > > > > > > > > mm/kasan/generic.c| 5 +
> > > > > > > > > > mm/kasan/generic_report.c | 12
> > > > > > > > > > mm/kasan/tags.c | 5 +
> > > > > > > > > > mm/kasan/tags_report.c| 12
> > > > > > > > > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > > > > > > > > Author: Walter-zh Wu
> > > > > > > > > > Date: Fri Oct 4 18:38:31 2019 +0800
> > > > > > > > > >
> > > > > > > > > > kasan: detect invalid size in memory operation function
> > > > > > > > > >
> > > > > > > > > > It is an undefined behavior to pass a negative numbers
> > > > > > > > > > to
> > > > > > > > > > memset()/memcpy()/memmove()
> > > > > > > > > > , so need to be detected by KASAN.
> > > > > > > > > >
> > > > > > > > > > If size is negative numbers, then it has two reasons to
> > > > > > > > > > be defined
> > > > > > > > > > as 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) Don't generate new bug type in order to prevent
> > > > > > > > > > duplicate reports
> > > > > > > > > > by
> > > > > > > > > > some systems, e.g. syzbot.
> > > > > > > > > >
> > > > > > > > > > KASAN report:
> > > > > > > > > >
> > > > > > > > > > BUG: KASAN: out-of-bounds in
> > > > > > > > > > kmalloc_memmove_invalid_size+0x70/0xa0
> > > > > > > > > > Read of size 18446744073709551608 at addr
> > > > > > > > > > ff8069660904 by task
> > > > > > > > > > cat/72
> > > > > > > > > >
> > > > > > > > > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > > > > > > > > 5.4.0-rc1-next-20191004ajb-1-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
> > > > > > > > > > Reported -by: Dmitry Vyukov
> > > > > > > > > > Suggested-by: Dmitry Vyukov
> > > > > > > > > >
> > > > > > > > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > > > > > > > index 6814d6d6a023..6ef0abd27f06 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)
> > > > > > >
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-10-07 at 12:51 +0200, Dmitry Vyukov wrote:
> On Mon, Oct 7, 2019 at 11:50 AM Walter Wu wrote:
> >
> > On Mon, 2019-10-07 at 17:28 +0800, Walter Wu wrote:
> > > On Mon, 2019-10-07 at 11:10 +0200, Dmitry Vyukov wrote:
> > > > On Mon, Oct 7, 2019 at 11:03 AM Walter Wu
> > > > wrote:
> > > > >
> > > > > On Mon, 2019-10-07 at 10:54 +0200, Dmitry Vyukov wrote:
> > > > > > On Mon, Oct 7, 2019 at 10:52 AM Walter Wu
> > > > > > wrote:
> > > > > > >
> > > > > > > On Mon, 2019-10-07 at 10:24 +0200, Dmitry Vyukov wrote:
> > > > > > > > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu
> > > > > > > > wrote:
> > > > > > > > > The patchsets help to produce KASAN report when size is
> > > > > > > > > negative numbers
> > > > > > > > > in memory operation function. It is helpful for programmer to
> > > > > > > > > solve the
> > > > > > > > > undefined behavior issue. Patch 1 based on Dmitry's review and
> > > > > > > > > suggestion, patch 2 is a test in order to verify the patch 1.
> > > > > > > > >
> > > > > > > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > > > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > > > > > > > >
> > > > > > > > > Walter Wu (2):
> > > > > > > > > kasan: detect invalid size in memory operation function
> > > > > > > > > kasan: add test for invalid size in memmove
> > > > > > > > >
> > > > > > > > > lib/test_kasan.c | 18 ++
> > > > > > > > > mm/kasan/common.c | 13 -
> > > > > > > > > mm/kasan/generic.c| 5 +
> > > > > > > > > mm/kasan/generic_report.c | 12
> > > > > > > > > mm/kasan/tags.c | 5 +
> > > > > > > > > mm/kasan/tags_report.c| 12
> > > > > > > > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > > > > > > > Author: Walter-zh Wu
> > > > > > > > > Date: Fri Oct 4 18:38:31 2019 +0800
> > > > > > > > >
> > > > > > > > > kasan: detect invalid size in memory operation function
> > > > > > > > >
> > > > > > > > > It is an undefined behavior to pass a negative numbers to
> > > > > > > > > memset()/memcpy()/memmove()
> > > > > > > > > , so need to be detected by KASAN.
> > > > > > > > >
> > > > > > > > > If size is negative numbers, then it has two reasons to
> > > > > > > > > be defined
> > > > > > > > > as 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) Don't generate new bug type in order to prevent
> > > > > > > > > duplicate reports
> > > > > > > > > by
> > > > > > > > > some systems, e.g. syzbot.
> > > > > > > > >
> > > > > > > > > KASAN report:
> > > > > > > > >
> > > > > > > > > BUG: KASAN: out-of-bounds in
> > > > > > > > > kmalloc_memmove_invalid_size+0x70/0xa0
> > > > > > > > > Read of size 18446744073709551608 at addr
> > > > > > > > > ff8069660904 by task
> > > > > > > > > cat/72
> > > > > > > > >
> > > > > > > > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > > > > > > > 5.4.0-rc1-next-20191004ajb-1-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
> > > > > > > > > Reported -by: Dmitry Vyukov
> > > > > > > > > Suggested-by: Dmitry Vyukov
> > > > > > > > >
> > > > > > > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > > > > > > index 6814d6d6a023..6ef0abd27f06 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_))
> > > > > > > > > +
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, Oct 7, 2019 at 11:50 AM Walter Wu wrote:
>
> On Mon, 2019-10-07 at 17:28 +0800, Walter Wu wrote:
> > On Mon, 2019-10-07 at 11:10 +0200, Dmitry Vyukov wrote:
> > > On Mon, Oct 7, 2019 at 11:03 AM Walter Wu
> > > wrote:
> > > >
> > > > On Mon, 2019-10-07 at 10:54 +0200, Dmitry Vyukov wrote:
> > > > > On Mon, Oct 7, 2019 at 10:52 AM Walter Wu
> > > > > wrote:
> > > > > >
> > > > > > On Mon, 2019-10-07 at 10:24 +0200, Dmitry Vyukov wrote:
> > > > > > > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu
> > > > > > > wrote:
> > > > > > > > The patchsets help to produce KASAN report when size is
> > > > > > > > negative numbers
> > > > > > > > in memory operation function. It is helpful for programmer to
> > > > > > > > solve the
> > > > > > > > undefined behavior issue. Patch 1 based on Dmitry's review and
> > > > > > > > suggestion, patch 2 is a test in order to verify the patch 1.
> > > > > > > >
> > > > > > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > > > > > > >
> > > > > > > > Walter Wu (2):
> > > > > > > > kasan: detect invalid size in memory operation function
> > > > > > > > kasan: add test for invalid size in memmove
> > > > > > > >
> > > > > > > > lib/test_kasan.c | 18 ++
> > > > > > > > mm/kasan/common.c | 13 -
> > > > > > > > mm/kasan/generic.c| 5 +
> > > > > > > > mm/kasan/generic_report.c | 12
> > > > > > > > mm/kasan/tags.c | 5 +
> > > > > > > > mm/kasan/tags_report.c| 12
> > > > > > > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > > > > > > Author: Walter-zh Wu
> > > > > > > > Date: Fri Oct 4 18:38:31 2019 +0800
> > > > > > > >
> > > > > > > > kasan: detect invalid size in memory operation function
> > > > > > > >
> > > > > > > > It is an undefined behavior to pass a negative numbers to
> > > > > > > > memset()/memcpy()/memmove()
> > > > > > > > , so need to be detected by KASAN.
> > > > > > > >
> > > > > > > > If size is negative numbers, then it has two reasons to be
> > > > > > > > defined
> > > > > > > > as 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) Don't generate new bug type in order to prevent
> > > > > > > > duplicate reports
> > > > > > > > by
> > > > > > > > some systems, e.g. syzbot.
> > > > > > > >
> > > > > > > > KASAN report:
> > > > > > > >
> > > > > > > > BUG: KASAN: out-of-bounds in
> > > > > > > > kmalloc_memmove_invalid_size+0x70/0xa0
> > > > > > > > Read of size 18446744073709551608 at addr ff8069660904
> > > > > > > > by task
> > > > > > > > cat/72
> > > > > > > >
> > > > > > > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > > > > > > 5.4.0-rc1-next-20191004ajb-1-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
> > > > > > > > Reported -by: Dmitry Vyukov
> > > > > > > > Suggested-by: Dmitry Vyukov
> > > > > > > >
> > > > > > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > > > > > index 6814d6d6a023..6ef0abd27f06 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
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-10-07 at 17:28 +0800, Walter Wu wrote:
> On Mon, 2019-10-07 at 11:10 +0200, Dmitry Vyukov wrote:
> > On Mon, Oct 7, 2019 at 11:03 AM Walter Wu wrote:
> > >
> > > On Mon, 2019-10-07 at 10:54 +0200, Dmitry Vyukov wrote:
> > > > On Mon, Oct 7, 2019 at 10:52 AM Walter Wu
> > > > wrote:
> > > > >
> > > > > On Mon, 2019-10-07 at 10:24 +0200, Dmitry Vyukov wrote:
> > > > > > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu
> > > > > > wrote:
> > > > > > > The patchsets help to produce KASAN report when size is negative
> > > > > > > numbers
> > > > > > > in memory operation function. It is helpful for programmer to
> > > > > > > solve the
> > > > > > > undefined behavior issue. Patch 1 based on Dmitry's review and
> > > > > > > suggestion, patch 2 is a test in order to verify the patch 1.
> > > > > > >
> > > > > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > > > > > >
> > > > > > > Walter Wu (2):
> > > > > > > kasan: detect invalid size in memory operation function
> > > > > > > kasan: add test for invalid size in memmove
> > > > > > >
> > > > > > > lib/test_kasan.c | 18 ++
> > > > > > > mm/kasan/common.c | 13 -
> > > > > > > mm/kasan/generic.c| 5 +
> > > > > > > mm/kasan/generic_report.c | 12
> > > > > > > mm/kasan/tags.c | 5 +
> > > > > > > mm/kasan/tags_report.c| 12
> > > > > > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > > > > > Author: Walter-zh Wu
> > > > > > > Date: Fri Oct 4 18:38:31 2019 +0800
> > > > > > >
> > > > > > > kasan: detect invalid size in memory operation function
> > > > > > >
> > > > > > > It is an undefined behavior to pass a negative numbers to
> > > > > > > memset()/memcpy()/memmove()
> > > > > > > , so need to be detected by KASAN.
> > > > > > >
> > > > > > > If size is negative numbers, then it has two reasons to be
> > > > > > > defined
> > > > > > > as 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) Don't generate new bug type in order to prevent duplicate
> > > > > > > reports
> > > > > > > by
> > > > > > > some systems, e.g. syzbot.
> > > > > > >
> > > > > > > KASAN report:
> > > > > > >
> > > > > > > BUG: KASAN: out-of-bounds in
> > > > > > > kmalloc_memmove_invalid_size+0x70/0xa0
> > > > > > > Read of size 18446744073709551608 at addr ff8069660904
> > > > > > > by task
> > > > > > > cat/72
> > > > > > >
> > > > > > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > > > > > 5.4.0-rc1-next-20191004ajb-1-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
> > > > > > > Reported -by: Dmitry Vyukov
> > > > > > > Suggested-by: Dmitry Vyukov
> > > > > > >
> > > > > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > > > > index 6814d6d6a023..6ef0abd27f06 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
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-10-07 at 11:10 +0200, Dmitry Vyukov wrote:
> On Mon, Oct 7, 2019 at 11:03 AM Walter Wu wrote:
> >
> > On Mon, 2019-10-07 at 10:54 +0200, Dmitry Vyukov wrote:
> > > On Mon, Oct 7, 2019 at 10:52 AM Walter Wu
> > > wrote:
> > > >
> > > > On Mon, 2019-10-07 at 10:24 +0200, Dmitry Vyukov wrote:
> > > > > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu
> > > > > wrote:
> > > > > > The patchsets help to produce KASAN report when size is negative
> > > > > > numbers
> > > > > > in memory operation function. It is helpful for programmer to solve
> > > > > > the
> > > > > > undefined behavior issue. Patch 1 based on Dmitry's review and
> > > > > > suggestion, patch 2 is a test in order to verify the patch 1.
> > > > > >
> > > > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > > > > >
> > > > > > Walter Wu (2):
> > > > > > kasan: detect invalid size in memory operation function
> > > > > > kasan: add test for invalid size in memmove
> > > > > >
> > > > > > lib/test_kasan.c | 18 ++
> > > > > > mm/kasan/common.c | 13 -
> > > > > > mm/kasan/generic.c| 5 +
> > > > > > mm/kasan/generic_report.c | 12
> > > > > > mm/kasan/tags.c | 5 +
> > > > > > mm/kasan/tags_report.c| 12
> > > > > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > > > > Author: Walter-zh Wu
> > > > > > Date: Fri Oct 4 18:38:31 2019 +0800
> > > > > >
> > > > > > kasan: detect invalid size in memory operation function
> > > > > >
> > > > > > It is an undefined behavior to pass a negative numbers to
> > > > > > memset()/memcpy()/memmove()
> > > > > > , so need to be detected by KASAN.
> > > > > >
> > > > > > If size is negative numbers, then it has two reasons to be
> > > > > > defined
> > > > > > as 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) Don't generate new bug type in order to prevent duplicate
> > > > > > reports
> > > > > > by
> > > > > > some systems, e.g. syzbot.
> > > > > >
> > > > > > KASAN report:
> > > > > >
> > > > > > BUG: KASAN: out-of-bounds in
> > > > > > kmalloc_memmove_invalid_size+0x70/0xa0
> > > > > > Read of size 18446744073709551608 at addr ff8069660904 by
> > > > > > task
> > > > > > cat/72
> > > > > >
> > > > > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > > > > 5.4.0-rc1-next-20191004ajb-1-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
> > > > > > Reported -by: Dmitry Vyukov
> > > > > > Suggested-by: Dmitry Vyukov
> > > > > >
> > > > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > > > index 6814d6d6a023..6ef0abd27f06 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);
> > > > > > }
> >
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, Oct 7, 2019 at 11:03 AM Walter Wu wrote:
>
> On Mon, 2019-10-07 at 10:54 +0200, Dmitry Vyukov wrote:
> > On Mon, Oct 7, 2019 at 10:52 AM Walter Wu wrote:
> > >
> > > On Mon, 2019-10-07 at 10:24 +0200, Dmitry Vyukov wrote:
> > > > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu
> > > > wrote:
> > > > > The patchsets help to produce KASAN report when size is negative
> > > > > numbers
> > > > > in memory operation function. It is helpful for programmer to solve
> > > > > the
> > > > > undefined behavior issue. Patch 1 based on Dmitry's review and
> > > > > suggestion, patch 2 is a test in order to verify the patch 1.
> > > > >
> > > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > > > >
> > > > > Walter Wu (2):
> > > > > kasan: detect invalid size in memory operation function
> > > > > kasan: add test for invalid size in memmove
> > > > >
> > > > > lib/test_kasan.c | 18 ++
> > > > > mm/kasan/common.c | 13 -
> > > > > mm/kasan/generic.c| 5 +
> > > > > mm/kasan/generic_report.c | 12
> > > > > mm/kasan/tags.c | 5 +
> > > > > mm/kasan/tags_report.c| 12
> > > > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > > > Author: Walter-zh Wu
> > > > > Date: Fri Oct 4 18:38:31 2019 +0800
> > > > >
> > > > > kasan: detect invalid size in memory operation function
> > > > >
> > > > > It is an undefined behavior to pass a negative numbers to
> > > > > memset()/memcpy()/memmove()
> > > > > , so need to be detected by KASAN.
> > > > >
> > > > > If size is negative numbers, then it has two reasons to be defined
> > > > > as 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) Don't generate new bug type in order to prevent duplicate
> > > > > reports
> > > > > by
> > > > > some systems, e.g. syzbot.
> > > > >
> > > > > KASAN report:
> > > > >
> > > > > BUG: KASAN: out-of-bounds in
> > > > > kmalloc_memmove_invalid_size+0x70/0xa0
> > > > > Read of size 18446744073709551608 at addr ff8069660904 by
> > > > > task
> > > > > cat/72
> > > > >
> > > > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > > > 5.4.0-rc1-next-20191004ajb-1-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
> > > > > Reported -by: Dmitry Vyukov
> > > > > Suggested-by: Dmitry Vyukov
> > > > >
> > > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > > index 6814d6d6a023..6ef0abd27f06 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,
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-10-07 at 10:54 +0200, Dmitry Vyukov wrote:
> On Mon, Oct 7, 2019 at 10:52 AM Walter Wu wrote:
> >
> > On Mon, 2019-10-07 at 10:24 +0200, Dmitry Vyukov wrote:
> > > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu
> > > wrote:
> > > > The patchsets help to produce KASAN report when size is negative numbers
> > > > in memory operation function. It is helpful for programmer to solve the
> > > > undefined behavior issue. Patch 1 based on Dmitry's review and
> > > > suggestion, patch 2 is a test in order to verify the patch 1.
> > > >
> > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > > >
> > > > Walter Wu (2):
> > > > kasan: detect invalid size in memory operation function
> > > > kasan: add test for invalid size in memmove
> > > >
> > > > lib/test_kasan.c | 18 ++
> > > > mm/kasan/common.c | 13 -
> > > > mm/kasan/generic.c| 5 +
> > > > mm/kasan/generic_report.c | 12
> > > > mm/kasan/tags.c | 5 +
> > > > mm/kasan/tags_report.c| 12
> > > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > > >
> > > >
> > > >
> > > >
> > > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > > Author: Walter-zh Wu
> > > > Date: Fri Oct 4 18:38:31 2019 +0800
> > > >
> > > > kasan: detect invalid size in memory operation function
> > > >
> > > > It is an undefined behavior to pass a negative numbers to
> > > > memset()/memcpy()/memmove()
> > > > , so need to be detected by KASAN.
> > > >
> > > > If size is negative numbers, then it has two reasons to be defined
> > > > as 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) Don't generate new bug type in order to prevent duplicate reports
> > > > by
> > > > some systems, e.g. syzbot.
> > > >
> > > > KASAN report:
> > > >
> > > > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> > > > Read of size 18446744073709551608 at addr ff8069660904 by task
> > > > cat/72
> > > >
> > > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > > 5.4.0-rc1-next-20191004ajb-1-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
> > > > Reported -by: Dmitry Vyukov
> > > > Suggested-by: Dmitry Vyukov
> > > >
> > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > index 6814d6d6a023..6ef0abd27f06 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);
> > > > }
> > > > diff --git a/mm/kasan/generic.c
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, Oct 7, 2019 at 10:52 AM Walter Wu wrote:
>
> On Mon, 2019-10-07 at 10:24 +0200, Dmitry Vyukov wrote:
> > On Mon, Oct 7, 2019 at 10:18 AM Walter Wu wrote:
> > > The patchsets help to produce KASAN report when size is negative numbers
> > > in memory operation function. It is helpful for programmer to solve the
> > > undefined behavior issue. Patch 1 based on Dmitry's review and
> > > suggestion, patch 2 is a test in order to verify the patch 1.
> > >
> > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > >
> > > Walter Wu (2):
> > > kasan: detect invalid size in memory operation function
> > > kasan: add test for invalid size in memmove
> > >
> > > lib/test_kasan.c | 18 ++
> > > mm/kasan/common.c | 13 -
> > > mm/kasan/generic.c| 5 +
> > > mm/kasan/generic_report.c | 12
> > > mm/kasan/tags.c | 5 +
> > > mm/kasan/tags_report.c| 12
> > > 6 files changed, 60 insertions(+), 5 deletions(-)
> > >
> > >
> > >
> > >
> > > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > > Author: Walter-zh Wu
> > > Date: Fri Oct 4 18:38:31 2019 +0800
> > >
> > > kasan: detect invalid size in memory operation function
> > >
> > > It is an undefined behavior to pass a negative numbers to
> > > memset()/memcpy()/memmove()
> > > , so need to be detected by KASAN.
> > >
> > > If size is negative numbers, then it has two reasons to be defined
> > > as 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) Don't generate new bug type in order to prevent duplicate reports
> > > by
> > > some systems, e.g. syzbot.
> > >
> > > KASAN report:
> > >
> > > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> > > Read of size 18446744073709551608 at addr ff8069660904 by task
> > > cat/72
> > >
> > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > 5.4.0-rc1-next-20191004ajb-1-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
> > > Reported -by: Dmitry Vyukov
> > > Suggested-by: Dmitry Vyukov
> > >
> > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > index 6814d6d6a023..6ef0abd27f06 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);
> > > }
> > > 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;
>
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-10-07 at 10:24 +0200, Dmitry Vyukov wrote:
> On Mon, Oct 7, 2019 at 10:18 AM Walter Wu wrote:
> > The patchsets help to produce KASAN report when size is negative numbers
> > in memory operation function. It is helpful for programmer to solve the
> > undefined behavior issue. Patch 1 based on Dmitry's review and
> > suggestion, patch 2 is a test in order to verify the patch 1.
> >
> > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> >
> > Walter Wu (2):
> > kasan: detect invalid size in memory operation function
> > kasan: add test for invalid size in memmove
> >
> > lib/test_kasan.c | 18 ++
> > mm/kasan/common.c | 13 -
> > mm/kasan/generic.c| 5 +
> > mm/kasan/generic_report.c | 12
> > mm/kasan/tags.c | 5 +
> > mm/kasan/tags_report.c| 12
> > 6 files changed, 60 insertions(+), 5 deletions(-)
> >
> >
> >
> >
> > commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> > Author: Walter-zh Wu
> > Date: Fri Oct 4 18:38:31 2019 +0800
> >
> > kasan: detect invalid size in memory operation function
> >
> > It is an undefined behavior to pass a negative numbers to
> > memset()/memcpy()/memmove()
> > , so need to be detected by KASAN.
> >
> > If size is negative numbers, then it has two reasons to be defined
> > as 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) Don't generate new bug type in order to prevent duplicate reports
> > by
> > some systems, e.g. syzbot.
> >
> > KASAN report:
> >
> > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> > Read of size 18446744073709551608 at addr ff8069660904 by task
> > cat/72
> >
> > CPU: 2 PID: 72 Comm: cat Not tainted
> > 5.4.0-rc1-next-20191004ajb-1-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
> > Reported -by: Dmitry Vyukov
> > Suggested-by: Dmitry Vyukov
> >
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 6814d6d6a023..6ef0abd27f06 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);
> > }
> > 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)) {
> > + kasan_report(addr, size, write, ret_ip);
> > + return false;
> > + }
> > +
> > if (unlikely((void *)addr <
> > kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) {
> >
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, Oct 7, 2019 at 10:18 AM Walter Wu wrote:
> The patchsets help to produce KASAN report when size is negative numbers
> in memory operation function. It is helpful for programmer to solve the
> undefined behavior issue. Patch 1 based on Dmitry's review and
> suggestion, patch 2 is a test in order to verify the patch 1.
>
> [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
>
> Walter Wu (2):
> kasan: detect invalid size in memory operation function
> kasan: add test for invalid size in memmove
>
> lib/test_kasan.c | 18 ++
> mm/kasan/common.c | 13 -
> mm/kasan/generic.c| 5 +
> mm/kasan/generic_report.c | 12
> mm/kasan/tags.c | 5 +
> mm/kasan/tags_report.c| 12
> 6 files changed, 60 insertions(+), 5 deletions(-)
>
>
>
>
> commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
> Author: Walter-zh Wu
> Date: Fri Oct 4 18:38:31 2019 +0800
>
> kasan: detect invalid size in memory operation function
>
> It is an undefined behavior to pass a negative numbers to
> memset()/memcpy()/memmove()
> , so need to be detected by KASAN.
>
> If size is negative numbers, then it has two reasons to be defined
> as 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) Don't generate new bug type in order to prevent duplicate reports
> by
> some systems, e.g. syzbot.
>
> KASAN report:
>
> BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> Read of size 18446744073709551608 at addr ff8069660904 by task
> cat/72
>
> CPU: 2 PID: 72 Comm: cat Not tainted
> 5.4.0-rc1-next-20191004ajb-1-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
> Reported -by: Dmitry Vyukov
> Suggested-by: Dmitry Vyukov
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 6814d6d6a023..6ef0abd27f06 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);
> }
> 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)) {
> + 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..ed0eb94cb811 100644
> --- a/mm/kasan/generic_report.c
> +++ b/mm/kasan/generic_report.c
> @@ -107,6 +107,18 @@ static const char *get_wild_bug_type(struct
> kasan_access_info *info)
>
> const
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-10-07 at 09:29 +0200, Dmitry Vyukov wrote:
> > > > diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c
> > > > index 969ae08f59d7..19b9e364b397 100644
> > > > --- a/mm/kasan/tags_report.c
> > > > +++ b/mm/kasan/tags_report.c
> > > > @@ -36,6 +36,16 @@
> > > >
> > > > const char *get_bug_type(struct kasan_access_info *info)
> > > > {
> > > > + /*
> > > > +* if access_size < 0, then it will be larger than ULONG_MAX/2,
> > > > +* so that this can qualify as out-of-bounds.
> > > > +* out-of-bounds is the _least_ frequent KASAN bug type. So
> > > > saying
> > > > +* out-of-bounds has downsides of both approaches and won't
> > > > prevent
> > > > +* duplicate reports by syzbot.
> > > > +*/
> > > > + if ((long)info->access_size < 0)
> > > > + return "out-of-bounds";
> > >
> > >
> > > wait, no :)
> > > I meant we change it to heap-out-of-bounds and explain why we are
> > > saying this is a heap-out-of-bounds.
> > > The current comment effectively says we are doing non useful thing for
> > > no reason, it does not eliminate any of my questions as a reader of
> > > this code :)
> > >
> > Ok, the current comment may not enough to be understood why we use OOB
> > to represent size<0 bug. We can modify it as below :)
> >
> > If access_size < 0, then it has two reasons to be defined as
> > out-of-bounds.
> > 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) Don't generate new bug type in order to prevent duplicate reports by
> > some systems, e.g. syzbot."
>
> Looks good to me. I think it should provide enough hooks for future
> readers to understand why we do this.
>
Thanks for your review and suggestion again.
If no other questions, We will send this patchset.
The patchsets help to produce KASAN report when size is negative numbers
in memory operation function. It is helpful for programmer to solve the
undefined behavior issue. Patch 1 based on Dmitry's review and
suggestion, patch 2 is a test in order to verify the patch 1.
[1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
[2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
Walter Wu (2):
kasan: detect invalid size in memory operation function
kasan: add test for invalid size in memmove
lib/test_kasan.c | 18 ++
mm/kasan/common.c | 13 -
mm/kasan/generic.c| 5 +
mm/kasan/generic_report.c | 12
mm/kasan/tags.c | 5 +
mm/kasan/tags_report.c| 12
6 files changed, 60 insertions(+), 5 deletions(-)
commit 5b3b68660b3d420fd2bd792f2d9fd3ccb8877ef7
Author: Walter-zh Wu
Date: Fri Oct 4 18:38:31 2019 +0800
kasan: detect invalid size in memory operation function
It is an undefined behavior to pass a negative numbers to
memset()/memcpy()/memmove()
, so need to be detected by KASAN.
If size is negative numbers, then it has two reasons to be defined
as 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) Don't generate new bug type in order to prevent duplicate reports
by
some systems, e.g. syzbot.
KASAN report:
BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
Read of size 18446744073709551608 at addr ff8069660904 by task
cat/72
CPU: 2 PID: 72 Comm: cat Not tainted
5.4.0-rc1-next-20191004ajb-1-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
Reported -by: Dmitry Vyukov
Suggested-by: Dmitry Vyukov
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 6814d6d6a023..6ef0abd27f06 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_);
-
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, Oct 7, 2019 at 5:23 AM Walter Wu wrote:
> > > > > > "out-of-bounds" is the _least_ frequent KASAN bug type. So saying
> > > > > > "out-of-bounds" has downsides of both approaches and won't prevent
> > > > > > duplicate reports by syzbot...
> > > > > >
> > > > > maybe i should add your comment into the comment in get_bug_type?
> > > >
> > > > Yes, that's exactly what I meant above:
> > > >
> > > > "I would change get_bug_type() to return "slab-out-of-bounds" (as the
> > > > most common OOB) in such case (with a comment)."
> > > >
> > > > ;)
> > >
> > >
> > > The patchset help to produce KASAN report when size is negative size in
> > > memory operation function. It is helpful for programmer to solve the
> > > undefined behavior issue. Patch 1 based on Dmitry's suggestion and
> > > review, patch 2 is a test in order to verify the patch 1.
> > >
> > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> > >
> > > Walter Wu (2):
> > > kasan: detect invalid size in memory operation function
> > > kasan: add test for invalid size in memmove
> > >
> > > lib/test_kasan.c | 18 ++
> > > mm/kasan/common.c | 13 -
> > > mm/kasan/generic.c| 5 +
> > > mm/kasan/generic_report.c | 10 ++
> > > mm/kasan/tags.c | 5 +
> > > mm/kasan/tags_report.c| 10 ++
> > > 6 files changed, 56 insertions(+), 5 deletions(-)
> > >
> > >
> > >
> > >
> > > commit 0bc50c759a425fa0aafb7ef623aa1598b3542c67
> > > Author: Walter Wu
> > > Date: Fri Oct 4 18:38:31 2019 +0800
> > >
> > > kasan: detect invalid size in memory operation function
> > >
> > > It is an undefined behavior to pass a negative value to
> > > memset()/memcpy()/memmove()
> > > , so need to be detected by KASAN.
> > >
> > > If size is negative value, then it will be larger than ULONG_MAX/2,
> > > so that we will qualify as out-of-bounds issue.
> > >
> > > KASAN report:
> > >
> > > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> > > Read of size 18446744073709551608 at addr ff8069660904 by task
> > > cat/72
> > >
> > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > 5.4.0-rc1-next-20191004ajb-1-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
> > > Reported -by: Dmitry Vyukov
> > > Suggested-by: Dmitry Vyukov
> > >
> > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > index 6814d6d6a023..6ef0abd27f06 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);
> > > }
> > > 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,
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, 2019-10-04 at 15:52 +0200, Dmitry Vyukov wrote:
> On Fri, Oct 4, 2019 at 2:05 PM Walter Wu wrote:
> >
> > On Fri, 2019-10-04 at 11:54 +0200, Dmitry Vyukov wrote:
> > > > > "out-of-bounds" is the _least_ frequent KASAN bug type. So saying
> > > > > "out-of-bounds" has downsides of both approaches and won't prevent
> > > > > duplicate reports by syzbot...
> > > > >
> > > > maybe i should add your comment into the comment in get_bug_type?
> > >
> > > Yes, that's exactly what I meant above:
> > >
> > > "I would change get_bug_type() to return "slab-out-of-bounds" (as the
> > > most common OOB) in such case (with a comment)."
> > >
> > > ;)
> >
> >
> > The patchset help to produce KASAN report when size is negative size in
> > memory operation function. It is helpful for programmer to solve the
> > undefined behavior issue. Patch 1 based on Dmitry's suggestion and
> > review, patch 2 is a test in order to verify the patch 1.
> >
> > [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
> >
> > Walter Wu (2):
> > kasan: detect invalid size in memory operation function
> > kasan: add test for invalid size in memmove
> >
> > lib/test_kasan.c | 18 ++
> > mm/kasan/common.c | 13 -
> > mm/kasan/generic.c| 5 +
> > mm/kasan/generic_report.c | 10 ++
> > mm/kasan/tags.c | 5 +
> > mm/kasan/tags_report.c| 10 ++
> > 6 files changed, 56 insertions(+), 5 deletions(-)
> >
> >
> >
> >
> > commit 0bc50c759a425fa0aafb7ef623aa1598b3542c67
> > Author: Walter Wu
> > Date: Fri Oct 4 18:38:31 2019 +0800
> >
> > kasan: detect invalid size in memory operation function
> >
> > It is an undefined behavior to pass a negative value to
> > memset()/memcpy()/memmove()
> > , so need to be detected by KASAN.
> >
> > If size is negative value, then it will be larger than ULONG_MAX/2,
> > so that we will qualify as out-of-bounds issue.
> >
> > KASAN report:
> >
> > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> > Read of size 18446744073709551608 at addr ff8069660904 by task
> > cat/72
> >
> > CPU: 2 PID: 72 Comm: cat Not tainted
> > 5.4.0-rc1-next-20191004ajb-1-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
> > Reported -by: Dmitry Vyukov
> > Suggested-by: Dmitry Vyukov
> >
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 6814d6d6a023..6ef0abd27f06 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);
> > }
> > 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)) {
> > +
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, Oct 4, 2019 at 2:05 PM Walter Wu wrote:
>
> On Fri, 2019-10-04 at 11:54 +0200, Dmitry Vyukov wrote:
> > > > "out-of-bounds" is the _least_ frequent KASAN bug type. So saying
> > > > "out-of-bounds" has downsides of both approaches and won't prevent
> > > > duplicate reports by syzbot...
> > > >
> > > maybe i should add your comment into the comment in get_bug_type?
> >
> > Yes, that's exactly what I meant above:
> >
> > "I would change get_bug_type() to return "slab-out-of-bounds" (as the
> > most common OOB) in such case (with a comment)."
> >
> > ;)
>
>
> The patchset help to produce KASAN report when size is negative size in
> memory operation function. It is helpful for programmer to solve the
> undefined behavior issue. Patch 1 based on Dmitry's suggestion and
> review, patch 2 is a test in order to verify the patch 1.
>
> [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
> [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
>
> Walter Wu (2):
> kasan: detect invalid size in memory operation function
> kasan: add test for invalid size in memmove
>
> lib/test_kasan.c | 18 ++
> mm/kasan/common.c | 13 -
> mm/kasan/generic.c| 5 +
> mm/kasan/generic_report.c | 10 ++
> mm/kasan/tags.c | 5 +
> mm/kasan/tags_report.c| 10 ++
> 6 files changed, 56 insertions(+), 5 deletions(-)
>
>
>
>
> commit 0bc50c759a425fa0aafb7ef623aa1598b3542c67
> Author: Walter Wu
> Date: Fri Oct 4 18:38:31 2019 +0800
>
> kasan: detect invalid size in memory operation function
>
> It is an undefined behavior to pass a negative value to
> memset()/memcpy()/memmove()
> , so need to be detected by KASAN.
>
> If size is negative value, then it will be larger than ULONG_MAX/2,
> so that we will qualify as out-of-bounds issue.
>
> KASAN report:
>
> BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> Read of size 18446744073709551608 at addr ff8069660904 by task
> cat/72
>
> CPU: 2 PID: 72 Comm: cat Not tainted
> 5.4.0-rc1-next-20191004ajb-1-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
> Reported -by: Dmitry Vyukov
> Suggested-by: Dmitry Vyukov
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 6814d6d6a023..6ef0abd27f06 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);
> }
> 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)) {
> + 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
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, 2019-10-04 at 11:54 +0200, Dmitry Vyukov wrote:
> > > "out-of-bounds" is the _least_ frequent KASAN bug type. So saying
> > > "out-of-bounds" has downsides of both approaches and won't prevent
> > > duplicate reports by syzbot...
> > >
> > maybe i should add your comment into the comment in get_bug_type?
>
> Yes, that's exactly what I meant above:
>
> "I would change get_bug_type() to return "slab-out-of-bounds" (as the
> most common OOB) in such case (with a comment)."
>
> ;)
The patchset help to produce KASAN report when size is negative size in
memory operation function. It is helpful for programmer to solve the
undefined behavior issue. Patch 1 based on Dmitry's suggestion and
review, patch 2 is a test in order to verify the patch 1.
[1]https://bugzilla.kernel.org/show_bug.cgi?id=199341
[2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh...@mediatek.com/
Walter Wu (2):
kasan: detect invalid size in memory operation function
kasan: add test for invalid size in memmove
lib/test_kasan.c | 18 ++
mm/kasan/common.c | 13 -
mm/kasan/generic.c| 5 +
mm/kasan/generic_report.c | 10 ++
mm/kasan/tags.c | 5 +
mm/kasan/tags_report.c| 10 ++
6 files changed, 56 insertions(+), 5 deletions(-)
commit 0bc50c759a425fa0aafb7ef623aa1598b3542c67
Author: Walter Wu
Date: Fri Oct 4 18:38:31 2019 +0800
kasan: detect invalid size in memory operation function
It is an undefined behavior to pass a negative value to
memset()/memcpy()/memmove()
, so need to be detected by KASAN.
If size is negative value, then it will be larger than ULONG_MAX/2,
so that we will qualify as out-of-bounds issue.
KASAN report:
BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
Read of size 18446744073709551608 at addr ff8069660904 by task
cat/72
CPU: 2 PID: 72 Comm: cat Not tainted
5.4.0-rc1-next-20191004ajb-1-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
Reported -by: Dmitry Vyukov
Suggested-by: Dmitry Vyukov
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 6814d6d6a023..6ef0abd27f06 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);
}
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)) {
+ 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..23951a453681 100644
--- a/mm/kasan/generic_report.c
+++ b/mm/kasan/generic_report.c
@@ -107,6 +107,16 @@ static const char *get_wild_bug_type(struct
kasan_access_info *info)
const char *get_bug_type(struct kasan_access_info *info)
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, Oct 4, 2019 at 11:44 AM Walter Wu wrote:
>
> On Fri, 2019-10-04 at 11:18 +0200, Dmitry Vyukov wrote:
> > On Fri, Oct 4, 2019 at 10:02 AM Walter Wu wrote:
> > >
> > > On Fri, 2019-10-04 at 12:42 +0800, Walter Wu wrote:
> > > > On Thu, 2019-10-03 at 16:53 +0200, Dmitry Vyukov wrote:
> > > > > On Thu, Oct 3, 2019 at 3:51 PM Walter Wu
> > > > > wrote:>
> > > > > >
> > > > > > static void print_error_description(struct kasan_access_info *info)
> > > > > > {
> > > > > > - pr_err("BUG: KASAN: %s in %pS\n",
> > > > > > - get_bug_type(info), (void *)info->ip);
> > > > > > - pr_err("%s of size %zu at addr %px by task %s/%d\n",
> > > > > > - info->is_write ? "Write" : "Read",
> > > > > > info->access_size,
> > > > > > - info->access_addr, current->comm,
> > > > > > task_pid_nr(current));
> > > > > > + if ((long)info->access_size < 0) {
> > > > > > + pr_err("BUG: KASAN: invalid size %zu in %pS\n",
> > > > > > + info->access_size, (void *)info->ip);
> > > > >
> > > > > I would not introduce a new bug type.
> > > > > These are parsed and used by some systems, e.g. syzbot. If size is
> > > > > user-controllable, then a new bug type for this will mean 2 bug
> > > > > reports.
> > > > > It also won't harm to print Read/Write, definitely the address, so no
> > > > > reason to special case this out of a dozen of report formats.
> > > > > This can qualify as out-of-bounds (definitely will cross some
> > > > > bounds!), so I would change get_bug_type() to return
> > > > > "slab-out-of-bounds" (as the most common OOB) in such case (with a
> > > > > comment).
> > > > >
> > > > Print Read/Write and address information, it is ok.
> > > > But if we can directly point to the root cause of this problem, why we
> > > > not do it? see 1) and 2) to get a point, if we print OOB, then user
> > > > needs one minute to think what is root case of this problem, but if we
> > > > print invalid size, then user can directly get root case. this is my
> > > > original thinking.
> > > > 1)Invalid size is true then OOB is true.
> > > > 2)OOB is true then invalid size may be true or false.
> > > >
> > > > But I see you say some systems have used bug report so that avoid this
> > > > trouble, i will print the wrong type is "out-of-bound" in a unified way
> > > > when size<0.
> > > >
> > >
> > > Updated my patch, please help to review it.
> > > thanks.
> > >
> > > commit 13e10a7e4264eb25c5a14193068027afc9c261f6
> > > Author: Walter-zh Wu
> > > Date: Fri Oct 4 15:27:17 2019 +0800
> > >
> > > kasan: detect negative size in memory operation function
> > >
> > > It is an undefined behavior to pass a negative value to
> > > memset()/memcpy()/memmove()
> > > , so need to be detected by KASAN.
> > >
> > > If size is negative value, then it will be larger than ULONG_MAX/2,
> > > so that we will qualify as out-of-bounds issue.
> > >
> > > KASAN report:
> > >
> > > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> > > Read of size 18446744073709551608 at addr ff8069660904 by task
> > > cat/72
> > >
> > > CPU: 2 PID: 72 Comm: cat Not tainted
> > > 5.4.0-rc1-next-20191004ajb-1-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
> > > Reported -by: Dmitry Vyukov
> > > Suggested-by: Dmitry Vyukov
> > >
> > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > index 49cc4d570a40..06942cf585cc 100644
> > > --- a/lib/test_kasan.c
> > > +++ b/lib/test_kasan.c
> > > @@ -283,6 +283,23 @@ static noinline void __init
> > > kmalloc_oob_in_memset(void)
> > > kfree(ptr);
> > > }
> > >
> > > +static noinline void __init kmalloc_memmove_invalid_size(void)
> > > +{
> > > + char *ptr;
> > > + size_t size = 64;
> > > +
> > > + pr_info("invalid size in memmove\n");
> > > + ptr = kmalloc(size, GFP_KERNEL);
> > > + if (!ptr) {
> > > + pr_err("Allocation failed\n");
> > > + return;
> > > + }
> > > +
> > > + memset((char *)ptr, 0, 64);
> > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > + kfree(ptr);
> > > +}
> > > +
> > > static noinline void __init kmalloc_uaf(void)
> > > {
> > > char *ptr;
> > > @@ -773,6 +790,7 @@ static int __init kmalloc_tests_init(void)
> > > kmalloc_oob_memset_4();
> > > kmalloc_oob_memset_8();
> > > kmalloc_oob_memset_16();
> > > +
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, 2019-10-04 at 11:18 +0200, Dmitry Vyukov wrote:
> On Fri, Oct 4, 2019 at 10:02 AM Walter Wu wrote:
> >
> > On Fri, 2019-10-04 at 12:42 +0800, Walter Wu wrote:
> > > On Thu, 2019-10-03 at 16:53 +0200, Dmitry Vyukov wrote:
> > > > On Thu, Oct 3, 2019 at 3:51 PM Walter Wu
> > > > wrote:>
> > > > >
> > > > > static void print_error_description(struct kasan_access_info *info)
> > > > > {
> > > > > - pr_err("BUG: KASAN: %s in %pS\n",
> > > > > - get_bug_type(info), (void *)info->ip);
> > > > > - pr_err("%s of size %zu at addr %px by task %s/%d\n",
> > > > > - info->is_write ? "Write" : "Read", info->access_size,
> > > > > - info->access_addr, current->comm,
> > > > > task_pid_nr(current));
> > > > > + if ((long)info->access_size < 0) {
> > > > > + pr_err("BUG: KASAN: invalid size %zu in %pS\n",
> > > > > + info->access_size, (void *)info->ip);
> > > >
> > > > I would not introduce a new bug type.
> > > > These are parsed and used by some systems, e.g. syzbot. If size is
> > > > user-controllable, then a new bug type for this will mean 2 bug
> > > > reports.
> > > > It also won't harm to print Read/Write, definitely the address, so no
> > > > reason to special case this out of a dozen of report formats.
> > > > This can qualify as out-of-bounds (definitely will cross some
> > > > bounds!), so I would change get_bug_type() to return
> > > > "slab-out-of-bounds" (as the most common OOB) in such case (with a
> > > > comment).
> > > >
> > > Print Read/Write and address information, it is ok.
> > > But if we can directly point to the root cause of this problem, why we
> > > not do it? see 1) and 2) to get a point, if we print OOB, then user
> > > needs one minute to think what is root case of this problem, but if we
> > > print invalid size, then user can directly get root case. this is my
> > > original thinking.
> > > 1)Invalid size is true then OOB is true.
> > > 2)OOB is true then invalid size may be true or false.
> > >
> > > But I see you say some systems have used bug report so that avoid this
> > > trouble, i will print the wrong type is "out-of-bound" in a unified way
> > > when size<0.
> > >
> >
> > Updated my patch, please help to review it.
> > thanks.
> >
> > commit 13e10a7e4264eb25c5a14193068027afc9c261f6
> > Author: Walter-zh Wu
> > Date: Fri Oct 4 15:27:17 2019 +0800
> >
> > kasan: detect negative size in memory operation function
> >
> > It is an undefined behavior to pass a negative value to
> > memset()/memcpy()/memmove()
> > , so need to be detected by KASAN.
> >
> > If size is negative value, then it will be larger than ULONG_MAX/2,
> > so that we will qualify as out-of-bounds issue.
> >
> > KASAN report:
> >
> > BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> > Read of size 18446744073709551608 at addr ff8069660904 by task
> > cat/72
> >
> > CPU: 2 PID: 72 Comm: cat Not tainted
> > 5.4.0-rc1-next-20191004ajb-1-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
> > Reported -by: Dmitry Vyukov
> > Suggested-by: Dmitry Vyukov
> >
> > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > index 49cc4d570a40..06942cf585cc 100644
> > --- a/lib/test_kasan.c
> > +++ b/lib/test_kasan.c
> > @@ -283,6 +283,23 @@ static noinline void __init
> > kmalloc_oob_in_memset(void)
> > kfree(ptr);
> > }
> >
> > +static noinline void __init kmalloc_memmove_invalid_size(void)
> > +{
> > + char *ptr;
> > + size_t size = 64;
> > +
> > + pr_info("invalid size in memmove\n");
> > + ptr = kmalloc(size, GFP_KERNEL);
> > + if (!ptr) {
> > + pr_err("Allocation failed\n");
> > + return;
> > + }
> > +
> > + memset((char *)ptr, 0, 64);
> > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > + kfree(ptr);
> > +}
> > +
> > static noinline void __init kmalloc_uaf(void)
> > {
> > char *ptr;
> > @@ -773,6 +790,7 @@ static int __init kmalloc_tests_init(void)
> > kmalloc_oob_memset_4();
> > kmalloc_oob_memset_8();
> > kmalloc_oob_memset_16();
> > + kmalloc_memmove_invalid_size();
> > kmalloc_uaf();
> > kmalloc_uaf_memset();
> > kmalloc_uaf2();
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 6814d6d6a023..97dd6eecc3e7 100644
> > --- a/mm/kasan/common.c
> > +++ b/mm/kasan/common.c
> > @@ -102,7 +102,8 @@
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, Oct 4, 2019 at 10:02 AM Walter Wu wrote:
>
> On Fri, 2019-10-04 at 12:42 +0800, Walter Wu wrote:
> > On Thu, 2019-10-03 at 16:53 +0200, Dmitry Vyukov wrote:
> > > On Thu, Oct 3, 2019 at 3:51 PM Walter Wu
> > > wrote:>
> > > >
> > > > static void print_error_description(struct kasan_access_info *info)
> > > > {
> > > > - pr_err("BUG: KASAN: %s in %pS\n",
> > > > - get_bug_type(info), (void *)info->ip);
> > > > - pr_err("%s of size %zu at addr %px by task %s/%d\n",
> > > > - info->is_write ? "Write" : "Read", info->access_size,
> > > > - info->access_addr, current->comm, task_pid_nr(current));
> > > > + if ((long)info->access_size < 0) {
> > > > + pr_err("BUG: KASAN: invalid size %zu in %pS\n",
> > > > + info->access_size, (void *)info->ip);
> > >
> > > I would not introduce a new bug type.
> > > These are parsed and used by some systems, e.g. syzbot. If size is
> > > user-controllable, then a new bug type for this will mean 2 bug
> > > reports.
> > > It also won't harm to print Read/Write, definitely the address, so no
> > > reason to special case this out of a dozen of report formats.
> > > This can qualify as out-of-bounds (definitely will cross some
> > > bounds!), so I would change get_bug_type() to return
> > > "slab-out-of-bounds" (as the most common OOB) in such case (with a
> > > comment).
> > >
> > Print Read/Write and address information, it is ok.
> > But if we can directly point to the root cause of this problem, why we
> > not do it? see 1) and 2) to get a point, if we print OOB, then user
> > needs one minute to think what is root case of this problem, but if we
> > print invalid size, then user can directly get root case. this is my
> > original thinking.
> > 1)Invalid size is true then OOB is true.
> > 2)OOB is true then invalid size may be true or false.
> >
> > But I see you say some systems have used bug report so that avoid this
> > trouble, i will print the wrong type is "out-of-bound" in a unified way
> > when size<0.
> >
>
> Updated my patch, please help to review it.
> thanks.
>
> commit 13e10a7e4264eb25c5a14193068027afc9c261f6
> Author: Walter-zh Wu
> Date: Fri Oct 4 15:27:17 2019 +0800
>
> kasan: detect negative size in memory operation function
>
> It is an undefined behavior to pass a negative value to
> memset()/memcpy()/memmove()
> , so need to be detected by KASAN.
>
> If size is negative value, then it will be larger than ULONG_MAX/2,
> so that we will qualify as out-of-bounds issue.
>
> KASAN report:
>
> BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
> Read of size 18446744073709551608 at addr ff8069660904 by task
> cat/72
>
> CPU: 2 PID: 72 Comm: cat Not tainted
> 5.4.0-rc1-next-20191004ajb-1-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
> Reported -by: Dmitry Vyukov
> Suggested-by: Dmitry Vyukov
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 49cc4d570a40..06942cf585cc 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -283,6 +283,23 @@ static noinline void __init
> kmalloc_oob_in_memset(void)
> kfree(ptr);
> }
>
> +static noinline void __init kmalloc_memmove_invalid_size(void)
> +{
> + char *ptr;
> + size_t size = 64;
> +
> + pr_info("invalid size in memmove\n");
> + ptr = kmalloc(size, GFP_KERNEL);
> + if (!ptr) {
> + pr_err("Allocation failed\n");
> + return;
> + }
> +
> + memset((char *)ptr, 0, 64);
> + memmove((char *)ptr, (char *)ptr + 4, -2);
> + kfree(ptr);
> +}
> +
> static noinline void __init kmalloc_uaf(void)
> {
> char *ptr;
> @@ -773,6 +790,7 @@ static int __init kmalloc_tests_init(void)
> kmalloc_oob_memset_4();
> kmalloc_oob_memset_8();
> kmalloc_oob_memset_16();
> + kmalloc_memmove_invalid_size();
> kmalloc_uaf();
> kmalloc_uaf_memset();
> kmalloc_uaf2();
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 6814d6d6a023..97dd6eecc3e7 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,
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, 2019-10-04 at 12:42 +0800, Walter Wu wrote:
> On Thu, 2019-10-03 at 16:53 +0200, Dmitry Vyukov wrote:
> > On Thu, Oct 3, 2019 at 3:51 PM Walter Wu wrote:>
> > >
> > > static void print_error_description(struct kasan_access_info *info)
> > > {
> > > - pr_err("BUG: KASAN: %s in %pS\n",
> > > - get_bug_type(info), (void *)info->ip);
> > > - pr_err("%s of size %zu at addr %px by task %s/%d\n",
> > > - info->is_write ? "Write" : "Read", info->access_size,
> > > - info->access_addr, current->comm, task_pid_nr(current));
> > > + if ((long)info->access_size < 0) {
> > > + pr_err("BUG: KASAN: invalid size %zu in %pS\n",
> > > + info->access_size, (void *)info->ip);
> >
> > I would not introduce a new bug type.
> > These are parsed and used by some systems, e.g. syzbot. If size is
> > user-controllable, then a new bug type for this will mean 2 bug
> > reports.
> > It also won't harm to print Read/Write, definitely the address, so no
> > reason to special case this out of a dozen of report formats.
> > This can qualify as out-of-bounds (definitely will cross some
> > bounds!), so I would change get_bug_type() to return
> > "slab-out-of-bounds" (as the most common OOB) in such case (with a
> > comment).
> >
> Print Read/Write and address information, it is ok.
> But if we can directly point to the root cause of this problem, why we
> not do it? see 1) and 2) to get a point, if we print OOB, then user
> needs one minute to think what is root case of this problem, but if we
> print invalid size, then user can directly get root case. this is my
> original thinking.
> 1)Invalid size is true then OOB is true.
> 2)OOB is true then invalid size may be true or false.
>
> But I see you say some systems have used bug report so that avoid this
> trouble, i will print the wrong type is "out-of-bound" in a unified way
> when size<0.
>
Updated my patch, please help to review it.
thanks.
commit 13e10a7e4264eb25c5a14193068027afc9c261f6
Author: Walter-zh Wu
Date: Fri Oct 4 15:27:17 2019 +0800
kasan: detect negative size in memory operation function
It is an undefined behavior to pass a negative value to
memset()/memcpy()/memmove()
, so need to be detected by KASAN.
If size is negative value, then it will be larger than ULONG_MAX/2,
so that we will qualify as out-of-bounds issue.
KASAN report:
BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0
Read of size 18446744073709551608 at addr ff8069660904 by task
cat/72
CPU: 2 PID: 72 Comm: cat Not tainted
5.4.0-rc1-next-20191004ajb-1-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
Reported -by: Dmitry Vyukov
Suggested-by: Dmitry Vyukov
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 49cc4d570a40..06942cf585cc 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -283,6 +283,23 @@ static noinline void __init
kmalloc_oob_in_memset(void)
kfree(ptr);
}
+static noinline void __init kmalloc_memmove_invalid_size(void)
+{
+ char *ptr;
+ size_t size = 64;
+
+ pr_info("invalid size in memmove\n");
+ ptr = kmalloc(size, GFP_KERNEL);
+ if (!ptr) {
+ pr_err("Allocation failed\n");
+ return;
+ }
+
+ memset((char *)ptr, 0, 64);
+ memmove((char *)ptr, (char *)ptr + 4, -2);
+ kfree(ptr);
+}
+
static noinline void __init kmalloc_uaf(void)
{
char *ptr;
@@ -773,6 +790,7 @@ static int __init kmalloc_tests_init(void)
kmalloc_oob_memset_4();
kmalloc_oob_memset_8();
kmalloc_oob_memset_16();
+ kmalloc_memmove_invalid_size();
kmalloc_uaf();
kmalloc_uaf_memset();
kmalloc_uaf2();
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 6814d6d6a023..97dd6eecc3e7 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,7 +111,8 @@ 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_);
+ if (!check_memory_region((unsigned long)src, len,
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Thu, 2019-10-03 at 16:53 +0200, Dmitry Vyukov wrote:
> On Thu, Oct 3, 2019 at 3:51 PM Walter Wu wrote:>
> > how about this?
> >
> > commit fd64691026e7ccb8d2946d0804b0621ac177df38
> > Author: Walter Wu
> > Date: Fri Sep 27 09:54:18 2019 +0800
> >
> > kasan: detect invalid size in memory operation function
> >
> > It is an undefined behavior to pass a negative value to
> > memset()/memcpy()/memmove()
> > , so need to be detected by KASAN.
> >
> > KASAN report:
> >
> > BUG: KASAN: invalid size 18446744073709551614 in
> > kmalloc_memmove_invalid_size+0x70/0xa0
> >
> > CPU: 1 PID: 91 Comm: cat Not tainted
> > 5.3.0-rc1ajb-1-g31943bbc21ce-dirty #7
> > Hardware name: linux,dummy-virt (DT)
> > Call trace:
> > dump_backtrace+0x0/0x278
> > show_stack+0x14/0x20
> > dump_stack+0x108/0x15c
> > print_address_description+0x64/0x368
> > __kasan_report+0x108/0x1a4
> > kasan_report+0xc/0x18
> > check_memory_region+0x15c/0x1b8
> > memmove+0x34/0x88
> > kmalloc_memmove_invalid_size+0x70/0xa0
> >
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> >
> > Signed-off-by: Walter Wu
> > Reported-by: Dmitry Vyukov
> >
> > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > index b63b367a94e8..e4e517a51860 100644
> > --- a/lib/test_kasan.c
> > +++ b/lib/test_kasan.c
> > @@ -280,6 +280,23 @@ static noinline void __init
> > kmalloc_oob_in_memset(void)
> > kfree(ptr);
> > }
> >
> > +static noinline void __init kmalloc_memmove_invalid_size(void)
> > +{
> > + char *ptr;
> > + size_t size = 64;
> > +
> > + pr_info("invalid size in memmove\n");
> > + ptr = kmalloc(size, GFP_KERNEL);
> > + if (!ptr) {
> > + pr_err("Allocation failed\n");
> > + return;
> > + }
> > +
> > + memset((char *)ptr, 0, 64);
> > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > + kfree(ptr);
> > +}
> > +
> > static noinline void __init kmalloc_uaf(void)
> > {
> > char *ptr;
> > @@ -734,6 +751,7 @@ static int __init kmalloc_tests_init(void)
> > kmalloc_oob_memset_4();
> > kmalloc_oob_memset_8();
> > kmalloc_oob_memset_16();
> > + kmalloc_memmove_invalid_size;
> > kmalloc_uaf();
> > kmalloc_uaf_memset();
> > kmalloc_uaf2();
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 2277b82902d8..5fd377af7457 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;
>
> Overall approach looks good to me.
> A good question is what we should return here. All bets are off after
> a report, but we still try to "minimize damage". One may argue for
> returning addr here and in other functions. But the more I think about
> this, the more I think it does not matter.
>
agreed
>
> > return __memset(addr, c, len);
> > }
> > @@ -110,7 +111,8 @@ 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_);
> > + if(!check_memory_region((unsigned long)src, len, false, _RET_IP_))
> > + return NULL;
> > check_memory_region((unsigned long)dest, len, true, _RET_IP_);
> >
> > return __memmove(dest, src, len);
> > @@ -119,7 +121,8 @@ 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_);
> > + if(!check_memory_region((unsigned long)src, len, false, _RET_IP_))
> > + return NULL;
> > check_memory_region((unsigned long)dest, len, true, _RET_IP_);
> >
> > 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)) {
> > + 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/report.c b/mm/kasan/report.c
> > index 0e5f965f1882..0cd317ef30f5 100644
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Thu, Oct 3, 2019 at 3:51 PM Walter Wu wrote:>
> how about this?
>
> commit fd64691026e7ccb8d2946d0804b0621ac177df38
> Author: Walter Wu
> Date: Fri Sep 27 09:54:18 2019 +0800
>
> kasan: detect invalid size in memory operation function
>
> It is an undefined behavior to pass a negative value to
> memset()/memcpy()/memmove()
> , so need to be detected by KASAN.
>
> KASAN report:
>
> BUG: KASAN: invalid size 18446744073709551614 in
> kmalloc_memmove_invalid_size+0x70/0xa0
>
> CPU: 1 PID: 91 Comm: cat Not tainted
> 5.3.0-rc1ajb-1-g31943bbc21ce-dirty #7
> Hardware name: linux,dummy-virt (DT)
> Call trace:
> dump_backtrace+0x0/0x278
> show_stack+0x14/0x20
> dump_stack+0x108/0x15c
> print_address_description+0x64/0x368
> __kasan_report+0x108/0x1a4
> kasan_report+0xc/0x18
> check_memory_region+0x15c/0x1b8
> memmove+0x34/0x88
> kmalloc_memmove_invalid_size+0x70/0xa0
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
>
> Signed-off-by: Walter Wu
> Reported-by: Dmitry Vyukov
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index b63b367a94e8..e4e517a51860 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -280,6 +280,23 @@ static noinline void __init
> kmalloc_oob_in_memset(void)
> kfree(ptr);
> }
>
> +static noinline void __init kmalloc_memmove_invalid_size(void)
> +{
> + char *ptr;
> + size_t size = 64;
> +
> + pr_info("invalid size in memmove\n");
> + ptr = kmalloc(size, GFP_KERNEL);
> + if (!ptr) {
> + pr_err("Allocation failed\n");
> + return;
> + }
> +
> + memset((char *)ptr, 0, 64);
> + memmove((char *)ptr, (char *)ptr + 4, -2);
> + kfree(ptr);
> +}
> +
> static noinline void __init kmalloc_uaf(void)
> {
> char *ptr;
> @@ -734,6 +751,7 @@ static int __init kmalloc_tests_init(void)
> kmalloc_oob_memset_4();
> kmalloc_oob_memset_8();
> kmalloc_oob_memset_16();
> + kmalloc_memmove_invalid_size;
> kmalloc_uaf();
> kmalloc_uaf_memset();
> kmalloc_uaf2();
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 2277b82902d8..5fd377af7457 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;
Overall approach looks good to me.
A good question is what we should return here. All bets are off after
a report, but we still try to "minimize damage". One may argue for
returning addr here and in other functions. But the more I think about
this, the more I think it does not matter.
> return __memset(addr, c, len);
> }
> @@ -110,7 +111,8 @@ 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_);
> + if(!check_memory_region((unsigned long)src, len, false, _RET_IP_))
> + return NULL;
> check_memory_region((unsigned long)dest, len, true, _RET_IP_);
>
> return __memmove(dest, src, len);
> @@ -119,7 +121,8 @@ 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_);
> + if(!check_memory_region((unsigned long)src, len, false, _RET_IP_))
> + return NULL;
> check_memory_region((unsigned long)dest, len, true, _RET_IP_);
>
> 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)) {
> + 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/report.c b/mm/kasan/report.c
> index 0e5f965f1882..0cd317ef30f5 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -68,11 +68,16 @@ __setup("kasan_multi_shot", kasan_set_multi_shot);
>
> static void print_error_description(struct kasan_access_info *info)
> {
> - pr_err("BUG: KASAN: %s in %pS\n",
> - get_bug_type(info), (void *)info->ip);
> - pr_err("%s of size %zu at addr %px by task %s/%d\n",
> - info->is_write ?
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Thu, 2019-10-03 at 17:38 +0800, Walter Wu wrote:
> On Thu, 2019-10-03 at 08:26 +0200, Dmitry Vyukov wrote:
> > On Thu, Oct 3, 2019 at 4:18 AM Walter Wu wrote:
> > >
> > > On Wed, 2019-10-02 at 15:57 +0200, Dmitry Vyukov wrote:
> > > > On Wed, Oct 2, 2019 at 2:15 PM Walter Wu
> > > > wrote:
> > > > >
> > > > > On Mon, 2019-09-30 at 12:36 +0800, Walter Wu wrote:
> > > > > > On Fri, 2019-09-27 at 21:41 +0200, Dmitry Vyukov wrote:
> > > > > > > On Fri, Sep 27, 2019 at 4:22 PM Walter Wu
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> > > > > > > > > On Fri, Sep 27, 2019 at 5:43 AM Walter Wu
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > memmove() and memcpy() have missing underflow issues.
> > > > > > > > > > When -7 <= size < 0, then KASAN will miss to catch the
> > > > > > > > > > underflow issue.
> > > > > > > > > > It looks like shadow start address and shadow end address
> > > > > > > > > > is the same,
> > > > > > > > > > so it does not actually check anything.
> > > > > > > > > >
> > > > > > > > > > The following test is indeed not caught by KASAN:
> > > > > > > > > >
> > > > > > > > > > char *p = kmalloc(64, GFP_KERNEL);
> > > > > > > > > > memset((char *)p, 0, 64);
> > > > > > > > > > memmove((char *)p, (char *)p + 4, -2);
> > > > > > > > > > kfree((char*)p);
> > > > > > > > > >
> > > > > > > > > > It should be checked here:
> > > > > > > > > >
> > > > > > > > > > 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_);
> > > > > > > > > >
> > > > > > > > > > return __memmove(dest, src, len);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > We fix the shadow end address which is calculated, then
> > > > > > > > > > generic KASAN
> > > > > > > > > > get the right shadow end address and detect this underflow
> > > > > > > > > > issue.
> > > > > > > > > >
> > > > > > > > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Walter Wu
> > > > > > > > > > Reported-by: Dmitry Vyukov
> > > > > > > > > > ---
> > > > > > > > > > lib/test_kasan.c | 36
> > > > > > > > > >
> > > > > > > > > > mm/kasan/generic.c | 8 ++--
> > > > > > > > > > 2 files changed, 42 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > > > > > > > > index b63b367a94e8..8bd014852556 100644
> > > > > > > > > > --- a/lib/test_kasan.c
> > > > > > > > > > +++ b/lib/test_kasan.c
> > > > > > > > > > @@ -280,6 +280,40 @@ static noinline void __init
> > > > > > > > > > kmalloc_oob_in_memset(void)
> > > > > > > > > > kfree(ptr);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +static noinline void __init
> > > > > > > > > > kmalloc_oob_in_memmove_underflow(void)
> > > > > > > > > > +{
> > > > > > > > > > + char *ptr;
> > > > > > > > > > + size_t size = 64;
> > > > > > > > > > +
> > > > > > > > > > + pr_info("underflow out-of-bounds in memmove\n");
> > > > > > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > > > > > + if (!ptr) {
> > > > > > > > > > + pr_err("Allocation failed\n");
> > > > > > > > > > + return;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + memset((char *)ptr, 0, 64);
> > > > > > > > > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > > > > > > > > + kfree(ptr);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static noinline void __init
> > > > > > > > > > kmalloc_oob_in_memmove_overflow(void)
> > > > > > > > > > +{
> > > > > > > > > > + char *ptr;
> > > > > > > > > > + size_t size = 64;
> > > > > > > > > > +
> > > > > > > > > > + pr_info("overflow out-of-bounds in memmove\n");
> > > > > > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > > > > > + if (!ptr) {
> > > > > > > > > > + pr_err("Allocation failed\n");
> > > > > > > > > > + return;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + memset((char *)ptr, 0, 64);
> > > > > > > > > > + memmove((char *)ptr + size, (char *)ptr, 2);
> > > > > > > > > > + kfree(ptr);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > static noinline void __init kmalloc_uaf(void)
> > > > > > > > > > {
> > > > > > > > > > char *ptr;
> > > > > > > > > > @@ -734,6 +768,8 @@ static int __init
> > > > > > > > > > kmalloc_tests_init(void)
> > > > > > > > > > kmalloc_oob_memset_4();
> > > > > > > > > > kmalloc_oob_memset_8();
> > > > > > > > > >
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Thu, 2019-10-03 at 08:26 +0200, Dmitry Vyukov wrote:
> On Thu, Oct 3, 2019 at 4:18 AM Walter Wu wrote:
> >
> > On Wed, 2019-10-02 at 15:57 +0200, Dmitry Vyukov wrote:
> > > On Wed, Oct 2, 2019 at 2:15 PM Walter Wu
> > > wrote:
> > > >
> > > > On Mon, 2019-09-30 at 12:36 +0800, Walter Wu wrote:
> > > > > On Fri, 2019-09-27 at 21:41 +0200, Dmitry Vyukov wrote:
> > > > > > On Fri, Sep 27, 2019 at 4:22 PM Walter Wu
> > > > > > wrote:
> > > > > > >
> > > > > > > On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> > > > > > > > On Fri, Sep 27, 2019 at 5:43 AM Walter Wu
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > memmove() and memcpy() have missing underflow issues.
> > > > > > > > > When -7 <= size < 0, then KASAN will miss to catch the
> > > > > > > > > underflow issue.
> > > > > > > > > It looks like shadow start address and shadow end address is
> > > > > > > > > the same,
> > > > > > > > > so it does not actually check anything.
> > > > > > > > >
> > > > > > > > > The following test is indeed not caught by KASAN:
> > > > > > > > >
> > > > > > > > > char *p = kmalloc(64, GFP_KERNEL);
> > > > > > > > > memset((char *)p, 0, 64);
> > > > > > > > > memmove((char *)p, (char *)p + 4, -2);
> > > > > > > > > kfree((char*)p);
> > > > > > > > >
> > > > > > > > > It should be checked here:
> > > > > > > > >
> > > > > > > > > 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_);
> > > > > > > > >
> > > > > > > > > return __memmove(dest, src, len);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > We fix the shadow end address which is calculated, then
> > > > > > > > > generic KASAN
> > > > > > > > > get the right shadow end address and detect this underflow
> > > > > > > > > issue.
> > > > > > > > >
> > > > > > > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > > > >
> > > > > > > > > Signed-off-by: Walter Wu
> > > > > > > > > Reported-by: Dmitry Vyukov
> > > > > > > > > ---
> > > > > > > > > lib/test_kasan.c | 36
> > > > > > > > > mm/kasan/generic.c | 8 ++--
> > > > > > > > > 2 files changed, 42 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > > > > > > > index b63b367a94e8..8bd014852556 100644
> > > > > > > > > --- a/lib/test_kasan.c
> > > > > > > > > +++ b/lib/test_kasan.c
> > > > > > > > > @@ -280,6 +280,40 @@ static noinline void __init
> > > > > > > > > kmalloc_oob_in_memset(void)
> > > > > > > > > kfree(ptr);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +static noinline void __init
> > > > > > > > > kmalloc_oob_in_memmove_underflow(void)
> > > > > > > > > +{
> > > > > > > > > + char *ptr;
> > > > > > > > > + size_t size = 64;
> > > > > > > > > +
> > > > > > > > > + pr_info("underflow out-of-bounds in memmove\n");
> > > > > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > > > > + if (!ptr) {
> > > > > > > > > + pr_err("Allocation failed\n");
> > > > > > > > > + return;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + memset((char *)ptr, 0, 64);
> > > > > > > > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > > > > > > > + kfree(ptr);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static noinline void __init
> > > > > > > > > kmalloc_oob_in_memmove_overflow(void)
> > > > > > > > > +{
> > > > > > > > > + char *ptr;
> > > > > > > > > + size_t size = 64;
> > > > > > > > > +
> > > > > > > > > + pr_info("overflow out-of-bounds in memmove\n");
> > > > > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > > > > + if (!ptr) {
> > > > > > > > > + pr_err("Allocation failed\n");
> > > > > > > > > + return;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + memset((char *)ptr, 0, 64);
> > > > > > > > > + memmove((char *)ptr + size, (char *)ptr, 2);
> > > > > > > > > + kfree(ptr);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > static noinline void __init kmalloc_uaf(void)
> > > > > > > > > {
> > > > > > > > > char *ptr;
> > > > > > > > > @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> > > > > > > > > kmalloc_oob_memset_4();
> > > > > > > > > kmalloc_oob_memset_8();
> > > > > > > > > kmalloc_oob_memset_16();
> > > > > > > > > + kmalloc_oob_in_memmove_underflow();
> > > > > > > > > + kmalloc_oob_in_memmove_overflow();
> > > > > > > > > kmalloc_uaf();
> > > > > > > > > kmalloc_uaf_memset();
> > > > > > > > > kmalloc_uaf2();
> > > > > > > > > diff
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Thu, Oct 3, 2019 at 4:18 AM Walter Wu wrote:
>
> On Wed, 2019-10-02 at 15:57 +0200, Dmitry Vyukov wrote:
> > On Wed, Oct 2, 2019 at 2:15 PM Walter Wu wrote:
> > >
> > > On Mon, 2019-09-30 at 12:36 +0800, Walter Wu wrote:
> > > > On Fri, 2019-09-27 at 21:41 +0200, Dmitry Vyukov wrote:
> > > > > On Fri, Sep 27, 2019 at 4:22 PM Walter Wu
> > > > > wrote:
> > > > > >
> > > > > > On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> > > > > > > On Fri, Sep 27, 2019 at 5:43 AM Walter Wu
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > memmove() and memcpy() have missing underflow issues.
> > > > > > > > When -7 <= size < 0, then KASAN will miss to catch the
> > > > > > > > underflow issue.
> > > > > > > > It looks like shadow start address and shadow end address is
> > > > > > > > the same,
> > > > > > > > so it does not actually check anything.
> > > > > > > >
> > > > > > > > The following test is indeed not caught by KASAN:
> > > > > > > >
> > > > > > > > char *p = kmalloc(64, GFP_KERNEL);
> > > > > > > > memset((char *)p, 0, 64);
> > > > > > > > memmove((char *)p, (char *)p + 4, -2);
> > > > > > > > kfree((char*)p);
> > > > > > > >
> > > > > > > > It should be checked here:
> > > > > > > >
> > > > > > > > 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_);
> > > > > > > >
> > > > > > > > return __memmove(dest, src, len);
> > > > > > > > }
> > > > > > > >
> > > > > > > > We fix the shadow end address which is calculated, then generic
> > > > > > > > KASAN
> > > > > > > > get the right shadow end address and detect this underflow
> > > > > > > > issue.
> > > > > > > >
> > > > > > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > > >
> > > > > > > > Signed-off-by: Walter Wu
> > > > > > > > Reported-by: Dmitry Vyukov
> > > > > > > > ---
> > > > > > > > lib/test_kasan.c | 36
> > > > > > > > mm/kasan/generic.c | 8 ++--
> > > > > > > > 2 files changed, 42 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > > > > > > index b63b367a94e8..8bd014852556 100644
> > > > > > > > --- a/lib/test_kasan.c
> > > > > > > > +++ b/lib/test_kasan.c
> > > > > > > > @@ -280,6 +280,40 @@ static noinline void __init
> > > > > > > > kmalloc_oob_in_memset(void)
> > > > > > > > kfree(ptr);
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static noinline void __init
> > > > > > > > kmalloc_oob_in_memmove_underflow(void)
> > > > > > > > +{
> > > > > > > > + char *ptr;
> > > > > > > > + size_t size = 64;
> > > > > > > > +
> > > > > > > > + pr_info("underflow out-of-bounds in memmove\n");
> > > > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > > > + if (!ptr) {
> > > > > > > > + pr_err("Allocation failed\n");
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + memset((char *)ptr, 0, 64);
> > > > > > > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > > > > > > + kfree(ptr);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static noinline void __init
> > > > > > > > kmalloc_oob_in_memmove_overflow(void)
> > > > > > > > +{
> > > > > > > > + char *ptr;
> > > > > > > > + size_t size = 64;
> > > > > > > > +
> > > > > > > > + pr_info("overflow out-of-bounds in memmove\n");
> > > > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > > > + if (!ptr) {
> > > > > > > > + pr_err("Allocation failed\n");
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + memset((char *)ptr, 0, 64);
> > > > > > > > + memmove((char *)ptr + size, (char *)ptr, 2);
> > > > > > > > + kfree(ptr);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static noinline void __init kmalloc_uaf(void)
> > > > > > > > {
> > > > > > > > char *ptr;
> > > > > > > > @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> > > > > > > > kmalloc_oob_memset_4();
> > > > > > > > kmalloc_oob_memset_8();
> > > > > > > > kmalloc_oob_memset_16();
> > > > > > > > + kmalloc_oob_in_memmove_underflow();
> > > > > > > > + kmalloc_oob_in_memmove_overflow();
> > > > > > > > kmalloc_uaf();
> > > > > > > > kmalloc_uaf_memset();
> > > > > > > > kmalloc_uaf2();
> > > > > > > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > > > > > > > index 616f9dd82d12..34ca23d59e67 100644
> > > > > > > > --- a/mm/kasan/generic.c
> > > > > > > > +++ b/mm/kasan/generic.c
> > > > > > > > @@ -131,9 +131,13 @@ static __always_inline bool
> > > > > > > >
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Wed, 2019-10-02 at 15:57 +0200, Dmitry Vyukov wrote:
> On Wed, Oct 2, 2019 at 2:15 PM Walter Wu wrote:
> >
> > On Mon, 2019-09-30 at 12:36 +0800, Walter Wu wrote:
> > > On Fri, 2019-09-27 at 21:41 +0200, Dmitry Vyukov wrote:
> > > > On Fri, Sep 27, 2019 at 4:22 PM Walter Wu
> > > > wrote:
> > > > >
> > > > > On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> > > > > > On Fri, Sep 27, 2019 at 5:43 AM Walter Wu
> > > > > > wrote:
> > > > > > >
> > > > > > > memmove() and memcpy() have missing underflow issues.
> > > > > > > When -7 <= size < 0, then KASAN will miss to catch the underflow
> > > > > > > issue.
> > > > > > > It looks like shadow start address and shadow end address is the
> > > > > > > same,
> > > > > > > so it does not actually check anything.
> > > > > > >
> > > > > > > The following test is indeed not caught by KASAN:
> > > > > > >
> > > > > > > char *p = kmalloc(64, GFP_KERNEL);
> > > > > > > memset((char *)p, 0, 64);
> > > > > > > memmove((char *)p, (char *)p + 4, -2);
> > > > > > > kfree((char*)p);
> > > > > > >
> > > > > > > It should be checked here:
> > > > > > >
> > > > > > > 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_);
> > > > > > >
> > > > > > > return __memmove(dest, src, len);
> > > > > > > }
> > > > > > >
> > > > > > > We fix the shadow end address which is calculated, then generic
> > > > > > > KASAN
> > > > > > > get the right shadow end address and detect this underflow issue.
> > > > > > >
> > > > > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > > >
> > > > > > > Signed-off-by: Walter Wu
> > > > > > > Reported-by: Dmitry Vyukov
> > > > > > > ---
> > > > > > > lib/test_kasan.c | 36
> > > > > > > mm/kasan/generic.c | 8 ++--
> > > > > > > 2 files changed, 42 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > > > > > index b63b367a94e8..8bd014852556 100644
> > > > > > > --- a/lib/test_kasan.c
> > > > > > > +++ b/lib/test_kasan.c
> > > > > > > @@ -280,6 +280,40 @@ static noinline void __init
> > > > > > > kmalloc_oob_in_memset(void)
> > > > > > > kfree(ptr);
> > > > > > > }
> > > > > > >
> > > > > > > +static noinline void __init
> > > > > > > kmalloc_oob_in_memmove_underflow(void)
> > > > > > > +{
> > > > > > > + char *ptr;
> > > > > > > + size_t size = 64;
> > > > > > > +
> > > > > > > + pr_info("underflow out-of-bounds in memmove\n");
> > > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > > + if (!ptr) {
> > > > > > > + pr_err("Allocation failed\n");
> > > > > > > + return;
> > > > > > > + }
> > > > > > > +
> > > > > > > + memset((char *)ptr, 0, 64);
> > > > > > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > > > > > + kfree(ptr);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static noinline void __init kmalloc_oob_in_memmove_overflow(void)
> > > > > > > +{
> > > > > > > + char *ptr;
> > > > > > > + size_t size = 64;
> > > > > > > +
> > > > > > > + pr_info("overflow out-of-bounds in memmove\n");
> > > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > > + if (!ptr) {
> > > > > > > + pr_err("Allocation failed\n");
> > > > > > > + return;
> > > > > > > + }
> > > > > > > +
> > > > > > > + memset((char *)ptr, 0, 64);
> > > > > > > + memmove((char *)ptr + size, (char *)ptr, 2);
> > > > > > > + kfree(ptr);
> > > > > > > +}
> > > > > > > +
> > > > > > > static noinline void __init kmalloc_uaf(void)
> > > > > > > {
> > > > > > > char *ptr;
> > > > > > > @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> > > > > > > kmalloc_oob_memset_4();
> > > > > > > kmalloc_oob_memset_8();
> > > > > > > kmalloc_oob_memset_16();
> > > > > > > + kmalloc_oob_in_memmove_underflow();
> > > > > > > + kmalloc_oob_in_memmove_overflow();
> > > > > > > kmalloc_uaf();
> > > > > > > kmalloc_uaf_memset();
> > > > > > > kmalloc_uaf2();
> > > > > > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > > > > > > index 616f9dd82d12..34ca23d59e67 100644
> > > > > > > --- a/mm/kasan/generic.c
> > > > > > > +++ b/mm/kasan/generic.c
> > > > > > > @@ -131,9 +131,13 @@ static __always_inline bool
> > > > > > > memory_is_poisoned_n(unsigned long addr,
> > > > > > > size_t size)
> > > > > > > {
> > > > > > > unsigned long ret;
> > > > > > > + void *shadow_start = kasan_mem_to_shadow((void *)addr);
> > > > > > > + void *shadow_end = kasan_mem_to_shadow((void *)addr +
>
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Wed, Oct 2, 2019 at 2:15 PM Walter Wu wrote:
>
> On Mon, 2019-09-30 at 12:36 +0800, Walter Wu wrote:
> > On Fri, 2019-09-27 at 21:41 +0200, Dmitry Vyukov wrote:
> > > On Fri, Sep 27, 2019 at 4:22 PM Walter Wu
> > > wrote:
> > > >
> > > > On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> > > > > On Fri, Sep 27, 2019 at 5:43 AM Walter Wu
> > > > > wrote:
> > > > > >
> > > > > > memmove() and memcpy() have missing underflow issues.
> > > > > > When -7 <= size < 0, then KASAN will miss to catch the underflow
> > > > > > issue.
> > > > > > It looks like shadow start address and shadow end address is the
> > > > > > same,
> > > > > > so it does not actually check anything.
> > > > > >
> > > > > > The following test is indeed not caught by KASAN:
> > > > > >
> > > > > > char *p = kmalloc(64, GFP_KERNEL);
> > > > > > memset((char *)p, 0, 64);
> > > > > > memmove((char *)p, (char *)p + 4, -2);
> > > > > > kfree((char*)p);
> > > > > >
> > > > > > It should be checked here:
> > > > > >
> > > > > > 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_);
> > > > > >
> > > > > > return __memmove(dest, src, len);
> > > > > > }
> > > > > >
> > > > > > We fix the shadow end address which is calculated, then generic
> > > > > > KASAN
> > > > > > get the right shadow end address and detect this underflow issue.
> > > > > >
> > > > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > > >
> > > > > > Signed-off-by: Walter Wu
> > > > > > Reported-by: Dmitry Vyukov
> > > > > > ---
> > > > > > lib/test_kasan.c | 36
> > > > > > mm/kasan/generic.c | 8 ++--
> > > > > > 2 files changed, 42 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > > > > index b63b367a94e8..8bd014852556 100644
> > > > > > --- a/lib/test_kasan.c
> > > > > > +++ b/lib/test_kasan.c
> > > > > > @@ -280,6 +280,40 @@ static noinline void __init
> > > > > > kmalloc_oob_in_memset(void)
> > > > > > kfree(ptr);
> > > > > > }
> > > > > >
> > > > > > +static noinline void __init kmalloc_oob_in_memmove_underflow(void)
> > > > > > +{
> > > > > > + char *ptr;
> > > > > > + size_t size = 64;
> > > > > > +
> > > > > > + pr_info("underflow out-of-bounds in memmove\n");
> > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > + if (!ptr) {
> > > > > > + pr_err("Allocation failed\n");
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > > > + memset((char *)ptr, 0, 64);
> > > > > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > > > > + kfree(ptr);
> > > > > > +}
> > > > > > +
> > > > > > +static noinline void __init kmalloc_oob_in_memmove_overflow(void)
> > > > > > +{
> > > > > > + char *ptr;
> > > > > > + size_t size = 64;
> > > > > > +
> > > > > > + pr_info("overflow out-of-bounds in memmove\n");
> > > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > > + if (!ptr) {
> > > > > > + pr_err("Allocation failed\n");
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > > > + memset((char *)ptr, 0, 64);
> > > > > > + memmove((char *)ptr + size, (char *)ptr, 2);
> > > > > > + kfree(ptr);
> > > > > > +}
> > > > > > +
> > > > > > static noinline void __init kmalloc_uaf(void)
> > > > > > {
> > > > > > char *ptr;
> > > > > > @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> > > > > > kmalloc_oob_memset_4();
> > > > > > kmalloc_oob_memset_8();
> > > > > > kmalloc_oob_memset_16();
> > > > > > + kmalloc_oob_in_memmove_underflow();
> > > > > > + kmalloc_oob_in_memmove_overflow();
> > > > > > kmalloc_uaf();
> > > > > > kmalloc_uaf_memset();
> > > > > > kmalloc_uaf2();
> > > > > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > > > > > index 616f9dd82d12..34ca23d59e67 100644
> > > > > > --- a/mm/kasan/generic.c
> > > > > > +++ b/mm/kasan/generic.c
> > > > > > @@ -131,9 +131,13 @@ static __always_inline bool
> > > > > > memory_is_poisoned_n(unsigned long addr,
> > > > > > size_t size)
> > > > > > {
> > > > > > unsigned long ret;
> > > > > > + void *shadow_start = kasan_mem_to_shadow((void *)addr);
> > > > > > + void *shadow_end = kasan_mem_to_shadow((void *)addr + size
> > > > > > - 1) + 1;
> > > > > >
> > > > > > - ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
> > > > > > - kasan_mem_to_shadow((void *)addr + size -
> > > > > > 1) + 1);
> > > > > > + if ((long)size < 0)
> > > > > > + shadow_end =
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-09-30 at 12:36 +0800, Walter Wu wrote:
> On Fri, 2019-09-27 at 21:41 +0200, Dmitry Vyukov wrote:
> > On Fri, Sep 27, 2019 at 4:22 PM Walter Wu wrote:
> > >
> > > On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> > > > On Fri, Sep 27, 2019 at 5:43 AM Walter Wu
> > > > wrote:
> > > > >
> > > > > memmove() and memcpy() have missing underflow issues.
> > > > > When -7 <= size < 0, then KASAN will miss to catch the underflow
> > > > > issue.
> > > > > It looks like shadow start address and shadow end address is the same,
> > > > > so it does not actually check anything.
> > > > >
> > > > > The following test is indeed not caught by KASAN:
> > > > >
> > > > > char *p = kmalloc(64, GFP_KERNEL);
> > > > > memset((char *)p, 0, 64);
> > > > > memmove((char *)p, (char *)p + 4, -2);
> > > > > kfree((char*)p);
> > > > >
> > > > > It should be checked here:
> > > > >
> > > > > 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_);
> > > > >
> > > > > return __memmove(dest, src, len);
> > > > > }
> > > > >
> > > > > We fix the shadow end address which is calculated, then generic KASAN
> > > > > get the right shadow end address and detect this underflow issue.
> > > > >
> > > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > > >
> > > > > Signed-off-by: Walter Wu
> > > > > Reported-by: Dmitry Vyukov
> > > > > ---
> > > > > lib/test_kasan.c | 36
> > > > > mm/kasan/generic.c | 8 ++--
> > > > > 2 files changed, 42 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > > > index b63b367a94e8..8bd014852556 100644
> > > > > --- a/lib/test_kasan.c
> > > > > +++ b/lib/test_kasan.c
> > > > > @@ -280,6 +280,40 @@ static noinline void __init
> > > > > kmalloc_oob_in_memset(void)
> > > > > kfree(ptr);
> > > > > }
> > > > >
> > > > > +static noinline void __init kmalloc_oob_in_memmove_underflow(void)
> > > > > +{
> > > > > + char *ptr;
> > > > > + size_t size = 64;
> > > > > +
> > > > > + pr_info("underflow out-of-bounds in memmove\n");
> > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > + if (!ptr) {
> > > > > + pr_err("Allocation failed\n");
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + memset((char *)ptr, 0, 64);
> > > > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > > > + kfree(ptr);
> > > > > +}
> > > > > +
> > > > > +static noinline void __init kmalloc_oob_in_memmove_overflow(void)
> > > > > +{
> > > > > + char *ptr;
> > > > > + size_t size = 64;
> > > > > +
> > > > > + pr_info("overflow out-of-bounds in memmove\n");
> > > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > > + if (!ptr) {
> > > > > + pr_err("Allocation failed\n");
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + memset((char *)ptr, 0, 64);
> > > > > + memmove((char *)ptr + size, (char *)ptr, 2);
> > > > > + kfree(ptr);
> > > > > +}
> > > > > +
> > > > > static noinline void __init kmalloc_uaf(void)
> > > > > {
> > > > > char *ptr;
> > > > > @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> > > > > kmalloc_oob_memset_4();
> > > > > kmalloc_oob_memset_8();
> > > > > kmalloc_oob_memset_16();
> > > > > + kmalloc_oob_in_memmove_underflow();
> > > > > + kmalloc_oob_in_memmove_overflow();
> > > > > kmalloc_uaf();
> > > > > kmalloc_uaf_memset();
> > > > > kmalloc_uaf2();
> > > > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > > > > index 616f9dd82d12..34ca23d59e67 100644
> > > > > --- a/mm/kasan/generic.c
> > > > > +++ b/mm/kasan/generic.c
> > > > > @@ -131,9 +131,13 @@ static __always_inline bool
> > > > > memory_is_poisoned_n(unsigned long addr,
> > > > > size_t size)
> > > > > {
> > > > > unsigned long ret;
> > > > > + void *shadow_start = kasan_mem_to_shadow((void *)addr);
> > > > > + void *shadow_end = kasan_mem_to_shadow((void *)addr + size -
> > > > > 1) + 1;
> > > > >
> > > > > - ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
> > > > > - kasan_mem_to_shadow((void *)addr + size - 1)
> > > > > + 1);
> > > > > + if ((long)size < 0)
> > > > > + shadow_end = kasan_mem_to_shadow((void *)addr + size);
> > > >
> > > > Hi Walter,
> > > >
> > > > Thanks for working on this.
> > > >
> > > > If size<0, does it make sense to continue at all? We will still check
> > > > 1PB of shadow memory? What happens when we pass such huge range to
> > > > memory_is_nonzero?
> > > > Perhaps it's better to produce an error
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Tue, 2019-10-01 at 05:01 +0200, Dmitry Vyukov wrote:
> On Tue, Oct 1, 2019 at 4:36 AM Walter Wu wrote:
> >
> > On Mon, 2019-09-30 at 10:57 +0200, Marc Gonzalez wrote:
> > > On 30/09/2019 06:36, Walter Wu wrote:
> > >
> > > > bool check_memory_region(unsigned long addr, size_t size, bool write,
> > > > unsigned long ret_ip)
> > > > {
> > > > + if (long(size) < 0) {
> > > > + kasan_report_invalid_size(src, dest, len, _RET_IP_);
> > > > + return false;
> > > > + }
> > > > +
> > > > return check_memory_region_inline(addr, size, write, ret_ip);
> > > > }
> > >
> > > Is it expected that memcpy/memmove may sometimes (incorrectly) be passed
> > > a negative value? (It would indeed turn up as a "large" size_t)
> > >
> > > IMO, casting to long is suspicious.
> > >
> > > There seem to be some two implicit assumptions.
> > >
> > > 1) size >= ULONG_MAX/2 is invalid input
> > > 2) casting a size >= ULONG_MAX/2 to long yields a negative value
> > >
> > > 1) seems reasonable because we can't copy more than half of memory to
> > > the other half of memory. I suppose the constraint could be even tighter,
> > > but it's not clear where to draw the line, especially when considering
> > > 32b vs 64b arches.
> > >
> > > 2) is implementation-defined, and gcc works "as expected" (clang too
> > > probably) https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html
> > >
> > > A comment might be warranted to explain the rationale.
> > > Regards.
> >
> > Thanks for your suggestion.
> > Yes, It is passed a negative value issue in memcpy/memmove/memset.
> > Our current idea should be assumption 1 and only consider 64b arch,
> > because KASAN only supports 64b. In fact, we really can't use so much
> > memory in 64b arch. so assumption 1 make sense.
>
> Note there are arm KASAN patches floating around, so we should not
> make assumptions about 64-bit arch.
I think arm KASAN patch doesn't merge in mainline, because virtual
memory of shadow memory is so bigger, the kernel virtual memory only has
1GB or 2GB in 32-bit arch, it is hard to solve the issue. it may need
some trade-off.
>
> But there seems to be a number of such casts already:
>
It seems that everyone is the same assumption.
> $ find -name "*.c" -exec egrep "\(long\).* < 0" {} \; -print
> } else if ((long) delta < 0) {
> ./kernel/time/timer.c
> if ((long)state < 0)
> ./drivers/thermal/thermal_sysfs.c
> if ((long)delay < 0)
> ./drivers/infiniband/core/addr.c
> if ((long)tmo < 0)
> ./drivers/net/wireless/st/cw1200/pm.c
> if (pos < 0 || (long) pos != pos || (ssize_t) count < 0)
> ./sound/core/info.c
> if ((long)hwrpb->sys_type < 0) {
> ./arch/alpha/kernel/setup.c
> if ((long)m->driver_data < 0)
> ./arch/x86/kernel/apic/apic.c
> if ((long) size < 0L)
> if ((long)addr < 0L) {
> ./arch/sparc/mm/init_64.c
> if ((long)lpid < 0)
> ./arch/powerpc/kvm/book3s_hv.c
> if ((long)regs->regs[insn.mm_i_format.rs] < 0)
> if ((long)regs->regs[insn.i_format.rs] < 0) {
> if ((long)regs->regs[insn.i_format.rs] < 0) {
> ./arch/mips/kernel/branch.c
> if ((long)arch->gprs[insn.i_format.rs] < 0)
> if ((long)arch->gprs[insn.i_format.rs] < 0)
> ./arch/mips/kvm/emulate.c
> if ((long)regs->regs[insn.i_format.rs] < 0)
> ./arch/mips/math-emu/cp1emu.c
> if ((int32_t)(long)prom_vec < 0) {
> ./arch/mips/sibyte/common/cfe.c
> if (msgsz > ns->msg_ctlmax || (long) msgsz < 0 || msqid < 0)
> if (msqid < 0 || (long) bufsz < 0)
> ./ipc/msg.c
> if ((long)x < 0)
> ./mm/page-writeback.c
> if ((long)(next - val) < 0) {
> ./mm/memcontrol.c
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Tue, Oct 1, 2019 at 4:36 AM Walter Wu wrote:
>
> On Mon, 2019-09-30 at 10:57 +0200, Marc Gonzalez wrote:
> > On 30/09/2019 06:36, Walter Wu wrote:
> >
> > > bool check_memory_region(unsigned long addr, size_t size, bool write,
> > > unsigned long ret_ip)
> > > {
> > > + if (long(size) < 0) {
> > > + kasan_report_invalid_size(src, dest, len, _RET_IP_);
> > > + return false;
> > > + }
> > > +
> > > return check_memory_region_inline(addr, size, write, ret_ip);
> > > }
> >
> > Is it expected that memcpy/memmove may sometimes (incorrectly) be passed
> > a negative value? (It would indeed turn up as a "large" size_t)
> >
> > IMO, casting to long is suspicious.
> >
> > There seem to be some two implicit assumptions.
> >
> > 1) size >= ULONG_MAX/2 is invalid input
> > 2) casting a size >= ULONG_MAX/2 to long yields a negative value
> >
> > 1) seems reasonable because we can't copy more than half of memory to
> > the other half of memory. I suppose the constraint could be even tighter,
> > but it's not clear where to draw the line, especially when considering
> > 32b vs 64b arches.
> >
> > 2) is implementation-defined, and gcc works "as expected" (clang too
> > probably) https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html
> >
> > A comment might be warranted to explain the rationale.
> > Regards.
>
> Thanks for your suggestion.
> Yes, It is passed a negative value issue in memcpy/memmove/memset.
> Our current idea should be assumption 1 and only consider 64b arch,
> because KASAN only supports 64b. In fact, we really can't use so much
> memory in 64b arch. so assumption 1 make sense.
Note there are arm KASAN patches floating around, so we should not
make assumptions about 64-bit arch.
But there seems to be a number of such casts already:
$ find -name "*.c" -exec egrep "\(long\).* < 0" {} \; -print
} else if ((long) delta < 0) {
./kernel/time/timer.c
if ((long)state < 0)
./drivers/thermal/thermal_sysfs.c
if ((long)delay < 0)
./drivers/infiniband/core/addr.c
if ((long)tmo < 0)
./drivers/net/wireless/st/cw1200/pm.c
if (pos < 0 || (long) pos != pos || (ssize_t) count < 0)
./sound/core/info.c
if ((long)hwrpb->sys_type < 0) {
./arch/alpha/kernel/setup.c
if ((long)m->driver_data < 0)
./arch/x86/kernel/apic/apic.c
if ((long) size < 0L)
if ((long)addr < 0L) {
./arch/sparc/mm/init_64.c
if ((long)lpid < 0)
./arch/powerpc/kvm/book3s_hv.c
if ((long)regs->regs[insn.mm_i_format.rs] < 0)
if ((long)regs->regs[insn.i_format.rs] < 0) {
if ((long)regs->regs[insn.i_format.rs] < 0) {
./arch/mips/kernel/branch.c
if ((long)arch->gprs[insn.i_format.rs] < 0)
if ((long)arch->gprs[insn.i_format.rs] < 0)
./arch/mips/kvm/emulate.c
if ((long)regs->regs[insn.i_format.rs] < 0)
./arch/mips/math-emu/cp1emu.c
if ((int32_t)(long)prom_vec < 0) {
./arch/mips/sibyte/common/cfe.c
if (msgsz > ns->msg_ctlmax || (long) msgsz < 0 || msqid < 0)
if (msqid < 0 || (long) bufsz < 0)
./ipc/msg.c
if ((long)x < 0)
./mm/page-writeback.c
if ((long)(next - val) < 0) {
./mm/memcontrol.c
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Mon, 2019-09-30 at 10:57 +0200, Marc Gonzalez wrote:
> On 30/09/2019 06:36, Walter Wu wrote:
>
> > bool check_memory_region(unsigned long addr, size_t size, bool write,
> > unsigned long ret_ip)
> > {
> > + if (long(size) < 0) {
> > + kasan_report_invalid_size(src, dest, len, _RET_IP_);
> > + return false;
> > + }
> > +
> > return check_memory_region_inline(addr, size, write, ret_ip);
> > }
>
> Is it expected that memcpy/memmove may sometimes (incorrectly) be passed
> a negative value? (It would indeed turn up as a "large" size_t)
>
> IMO, casting to long is suspicious.
>
> There seem to be some two implicit assumptions.
>
> 1) size >= ULONG_MAX/2 is invalid input
> 2) casting a size >= ULONG_MAX/2 to long yields a negative value
>
> 1) seems reasonable because we can't copy more than half of memory to
> the other half of memory. I suppose the constraint could be even tighter,
> but it's not clear where to draw the line, especially when considering
> 32b vs 64b arches.
>
> 2) is implementation-defined, and gcc works "as expected" (clang too
> probably) https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html
>
> A comment might be warranted to explain the rationale.
> Regards.
Thanks for your suggestion.
Yes, It is passed a negative value issue in memcpy/memmove/memset.
Our current idea should be assumption 1 and only consider 64b arch,
because KASAN only supports 64b. In fact, we really can't use so much
memory in 64b arch. so assumption 1 make sense.
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On 30/09/2019 06:36, Walter Wu wrote:
> bool check_memory_region(unsigned long addr, size_t size, bool write,
> unsigned long ret_ip)
> {
> + if (long(size) < 0) {
> + kasan_report_invalid_size(src, dest, len, _RET_IP_);
> + return false;
> + }
> +
> return check_memory_region_inline(addr, size, write, ret_ip);
> }
Is it expected that memcpy/memmove may sometimes (incorrectly) be passed
a negative value? (It would indeed turn up as a "large" size_t)
IMO, casting to long is suspicious.
There seem to be some two implicit assumptions.
1) size >= ULONG_MAX/2 is invalid input
2) casting a size >= ULONG_MAX/2 to long yields a negative value
1) seems reasonable because we can't copy more than half of memory to
the other half of memory. I suppose the constraint could be even tighter,
but it's not clear where to draw the line, especially when considering
32b vs 64b arches.
2) is implementation-defined, and gcc works "as expected" (clang too
probably) https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html
A comment might be warranted to explain the rationale.
Regards.
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, 2019-09-27 at 21:41 +0200, Dmitry Vyukov wrote:
> On Fri, Sep 27, 2019 at 4:22 PM Walter Wu wrote:
> >
> > On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> > > On Fri, Sep 27, 2019 at 5:43 AM Walter Wu
> > > wrote:
> > > >
> > > > memmove() and memcpy() have missing underflow issues.
> > > > When -7 <= size < 0, then KASAN will miss to catch the underflow issue.
> > > > It looks like shadow start address and shadow end address is the same,
> > > > so it does not actually check anything.
> > > >
> > > > The following test is indeed not caught by KASAN:
> > > >
> > > > char *p = kmalloc(64, GFP_KERNEL);
> > > > memset((char *)p, 0, 64);
> > > > memmove((char *)p, (char *)p + 4, -2);
> > > > kfree((char*)p);
> > > >
> > > > It should be checked here:
> > > >
> > > > 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_);
> > > >
> > > > return __memmove(dest, src, len);
> > > > }
> > > >
> > > > We fix the shadow end address which is calculated, then generic KASAN
> > > > get the right shadow end address and detect this underflow issue.
> > > >
> > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > > >
> > > > Signed-off-by: Walter Wu
> > > > Reported-by: Dmitry Vyukov
> > > > ---
> > > > lib/test_kasan.c | 36
> > > > mm/kasan/generic.c | 8 ++--
> > > > 2 files changed, 42 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > > index b63b367a94e8..8bd014852556 100644
> > > > --- a/lib/test_kasan.c
> > > > +++ b/lib/test_kasan.c
> > > > @@ -280,6 +280,40 @@ static noinline void __init
> > > > kmalloc_oob_in_memset(void)
> > > > kfree(ptr);
> > > > }
> > > >
> > > > +static noinline void __init kmalloc_oob_in_memmove_underflow(void)
> > > > +{
> > > > + char *ptr;
> > > > + size_t size = 64;
> > > > +
> > > > + pr_info("underflow out-of-bounds in memmove\n");
> > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > + if (!ptr) {
> > > > + pr_err("Allocation failed\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + memset((char *)ptr, 0, 64);
> > > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > > + kfree(ptr);
> > > > +}
> > > > +
> > > > +static noinline void __init kmalloc_oob_in_memmove_overflow(void)
> > > > +{
> > > > + char *ptr;
> > > > + size_t size = 64;
> > > > +
> > > > + pr_info("overflow out-of-bounds in memmove\n");
> > > > + ptr = kmalloc(size, GFP_KERNEL);
> > > > + if (!ptr) {
> > > > + pr_err("Allocation failed\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + memset((char *)ptr, 0, 64);
> > > > + memmove((char *)ptr + size, (char *)ptr, 2);
> > > > + kfree(ptr);
> > > > +}
> > > > +
> > > > static noinline void __init kmalloc_uaf(void)
> > > > {
> > > > char *ptr;
> > > > @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> > > > kmalloc_oob_memset_4();
> > > > kmalloc_oob_memset_8();
> > > > kmalloc_oob_memset_16();
> > > > + kmalloc_oob_in_memmove_underflow();
> > > > + kmalloc_oob_in_memmove_overflow();
> > > > kmalloc_uaf();
> > > > kmalloc_uaf_memset();
> > > > kmalloc_uaf2();
> > > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > > > index 616f9dd82d12..34ca23d59e67 100644
> > > > --- a/mm/kasan/generic.c
> > > > +++ b/mm/kasan/generic.c
> > > > @@ -131,9 +131,13 @@ static __always_inline bool
> > > > memory_is_poisoned_n(unsigned long addr,
> > > > size_t size)
> > > > {
> > > > unsigned long ret;
> > > > + void *shadow_start = kasan_mem_to_shadow((void *)addr);
> > > > + void *shadow_end = kasan_mem_to_shadow((void *)addr + size - 1)
> > > > + 1;
> > > >
> > > > - ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
> > > > - kasan_mem_to_shadow((void *)addr + size - 1) +
> > > > 1);
> > > > + if ((long)size < 0)
> > > > + shadow_end = kasan_mem_to_shadow((void *)addr + size);
> > >
> > > Hi Walter,
> > >
> > > Thanks for working on this.
> > >
> > > If size<0, does it make sense to continue at all? We will still check
> > > 1PB of shadow memory? What happens when we pass such huge range to
> > > memory_is_nonzero?
> > > Perhaps it's better to produce an error and bail out immediately if
> > > size<0?
> >
> > I agree with what you said. when size<0, it is indeed an unreasonable
> > behavior, it should be blocked from continuing to do.
> >
> >
> > > Also, what's the failure mode of the tests? Didn't they badly corrupt
> > > memory? We tried to keep tests such
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, Sep 27, 2019 at 4:22 PM Walter Wu wrote:
>
> On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> > On Fri, Sep 27, 2019 at 5:43 AM Walter Wu wrote:
> > >
> > > memmove() and memcpy() have missing underflow issues.
> > > When -7 <= size < 0, then KASAN will miss to catch the underflow issue.
> > > It looks like shadow start address and shadow end address is the same,
> > > so it does not actually check anything.
> > >
> > > The following test is indeed not caught by KASAN:
> > >
> > > char *p = kmalloc(64, GFP_KERNEL);
> > > memset((char *)p, 0, 64);
> > > memmove((char *)p, (char *)p + 4, -2);
> > > kfree((char*)p);
> > >
> > > It should be checked here:
> > >
> > > 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_);
> > >
> > > return __memmove(dest, src, len);
> > > }
> > >
> > > We fix the shadow end address which is calculated, then generic KASAN
> > > get the right shadow end address and detect this underflow issue.
> > >
> > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> > >
> > > Signed-off-by: Walter Wu
> > > Reported-by: Dmitry Vyukov
> > > ---
> > > lib/test_kasan.c | 36
> > > mm/kasan/generic.c | 8 ++--
> > > 2 files changed, 42 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > index b63b367a94e8..8bd014852556 100644
> > > --- a/lib/test_kasan.c
> > > +++ b/lib/test_kasan.c
> > > @@ -280,6 +280,40 @@ static noinline void __init
> > > kmalloc_oob_in_memset(void)
> > > kfree(ptr);
> > > }
> > >
> > > +static noinline void __init kmalloc_oob_in_memmove_underflow(void)
> > > +{
> > > + char *ptr;
> > > + size_t size = 64;
> > > +
> > > + pr_info("underflow out-of-bounds in memmove\n");
> > > + ptr = kmalloc(size, GFP_KERNEL);
> > > + if (!ptr) {
> > > + pr_err("Allocation failed\n");
> > > + return;
> > > + }
> > > +
> > > + memset((char *)ptr, 0, 64);
> > > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > > + kfree(ptr);
> > > +}
> > > +
> > > +static noinline void __init kmalloc_oob_in_memmove_overflow(void)
> > > +{
> > > + char *ptr;
> > > + size_t size = 64;
> > > +
> > > + pr_info("overflow out-of-bounds in memmove\n");
> > > + ptr = kmalloc(size, GFP_KERNEL);
> > > + if (!ptr) {
> > > + pr_err("Allocation failed\n");
> > > + return;
> > > + }
> > > +
> > > + memset((char *)ptr, 0, 64);
> > > + memmove((char *)ptr + size, (char *)ptr, 2);
> > > + kfree(ptr);
> > > +}
> > > +
> > > static noinline void __init kmalloc_uaf(void)
> > > {
> > > char *ptr;
> > > @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> > > kmalloc_oob_memset_4();
> > > kmalloc_oob_memset_8();
> > > kmalloc_oob_memset_16();
> > > + kmalloc_oob_in_memmove_underflow();
> > > + kmalloc_oob_in_memmove_overflow();
> > > kmalloc_uaf();
> > > kmalloc_uaf_memset();
> > > kmalloc_uaf2();
> > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > > index 616f9dd82d12..34ca23d59e67 100644
> > > --- a/mm/kasan/generic.c
> > > +++ b/mm/kasan/generic.c
> > > @@ -131,9 +131,13 @@ static __always_inline bool
> > > memory_is_poisoned_n(unsigned long addr,
> > > size_t size)
> > > {
> > > unsigned long ret;
> > > + void *shadow_start = kasan_mem_to_shadow((void *)addr);
> > > + void *shadow_end = kasan_mem_to_shadow((void *)addr + size - 1) +
> > > 1;
> > >
> > > - ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
> > > - kasan_mem_to_shadow((void *)addr + size - 1) + 1);
> > > + if ((long)size < 0)
> > > + shadow_end = kasan_mem_to_shadow((void *)addr + size);
> >
> > Hi Walter,
> >
> > Thanks for working on this.
> >
> > If size<0, does it make sense to continue at all? We will still check
> > 1PB of shadow memory? What happens when we pass such huge range to
> > memory_is_nonzero?
> > Perhaps it's better to produce an error and bail out immediately if size<0?
>
> I agree with what you said. when size<0, it is indeed an unreasonable
> behavior, it should be blocked from continuing to do.
>
>
> > Also, what's the failure mode of the tests? Didn't they badly corrupt
> > memory? We tried to keep tests such that they produce the KASAN
> > reports, but don't badly corrupt memory b/c/ we need to run all of
> > them.
>
> Maybe we should first produce KASAN reports and then go to execute
> memmove() or do nothing? It looks like it’s doing the following.or?
>
> void *memmove(void *dest, const void *src, size_t len)
> {
> + if
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:
> On Fri, Sep 27, 2019 at 5:43 AM Walter Wu wrote:
> >
> > memmove() and memcpy() have missing underflow issues.
> > When -7 <= size < 0, then KASAN will miss to catch the underflow issue.
> > It looks like shadow start address and shadow end address is the same,
> > so it does not actually check anything.
> >
> > The following test is indeed not caught by KASAN:
> >
> > char *p = kmalloc(64, GFP_KERNEL);
> > memset((char *)p, 0, 64);
> > memmove((char *)p, (char *)p + 4, -2);
> > kfree((char*)p);
> >
> > It should be checked here:
> >
> > 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_);
> >
> > return __memmove(dest, src, len);
> > }
> >
> > We fix the shadow end address which is calculated, then generic KASAN
> > get the right shadow end address and detect this underflow issue.
> >
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
> >
> > Signed-off-by: Walter Wu
> > Reported-by: Dmitry Vyukov
> > ---
> > lib/test_kasan.c | 36
> > mm/kasan/generic.c | 8 ++--
> > 2 files changed, 42 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > index b63b367a94e8..8bd014852556 100644
> > --- a/lib/test_kasan.c
> > +++ b/lib/test_kasan.c
> > @@ -280,6 +280,40 @@ static noinline void __init kmalloc_oob_in_memset(void)
> > kfree(ptr);
> > }
> >
> > +static noinline void __init kmalloc_oob_in_memmove_underflow(void)
> > +{
> > + char *ptr;
> > + size_t size = 64;
> > +
> > + pr_info("underflow out-of-bounds in memmove\n");
> > + ptr = kmalloc(size, GFP_KERNEL);
> > + if (!ptr) {
> > + pr_err("Allocation failed\n");
> > + return;
> > + }
> > +
> > + memset((char *)ptr, 0, 64);
> > + memmove((char *)ptr, (char *)ptr + 4, -2);
> > + kfree(ptr);
> > +}
> > +
> > +static noinline void __init kmalloc_oob_in_memmove_overflow(void)
> > +{
> > + char *ptr;
> > + size_t size = 64;
> > +
> > + pr_info("overflow out-of-bounds in memmove\n");
> > + ptr = kmalloc(size, GFP_KERNEL);
> > + if (!ptr) {
> > + pr_err("Allocation failed\n");
> > + return;
> > + }
> > +
> > + memset((char *)ptr, 0, 64);
> > + memmove((char *)ptr + size, (char *)ptr, 2);
> > + kfree(ptr);
> > +}
> > +
> > static noinline void __init kmalloc_uaf(void)
> > {
> > char *ptr;
> > @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> > kmalloc_oob_memset_4();
> > kmalloc_oob_memset_8();
> > kmalloc_oob_memset_16();
> > + kmalloc_oob_in_memmove_underflow();
> > + kmalloc_oob_in_memmove_overflow();
> > kmalloc_uaf();
> > kmalloc_uaf_memset();
> > kmalloc_uaf2();
> > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > index 616f9dd82d12..34ca23d59e67 100644
> > --- a/mm/kasan/generic.c
> > +++ b/mm/kasan/generic.c
> > @@ -131,9 +131,13 @@ static __always_inline bool
> > memory_is_poisoned_n(unsigned long addr,
> > size_t size)
> > {
> > unsigned long ret;
> > + void *shadow_start = kasan_mem_to_shadow((void *)addr);
> > + void *shadow_end = kasan_mem_to_shadow((void *)addr + size - 1) + 1;
> >
> > - ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
> > - kasan_mem_to_shadow((void *)addr + size - 1) + 1);
> > + if ((long)size < 0)
> > + shadow_end = kasan_mem_to_shadow((void *)addr + size);
>
> Hi Walter,
>
> Thanks for working on this.
>
> If size<0, does it make sense to continue at all? We will still check
> 1PB of shadow memory? What happens when we pass such huge range to
> memory_is_nonzero?
> Perhaps it's better to produce an error and bail out immediately if size<0?
I agree with what you said. when size<0, it is indeed an unreasonable
behavior, it should be blocked from continuing to do.
> Also, what's the failure mode of the tests? Didn't they badly corrupt
> memory? We tried to keep tests such that they produce the KASAN
> reports, but don't badly corrupt memory b/c/ we need to run all of
> them.
Maybe we should first produce KASAN reports and then go to execute
memmove() or do nothing? It looks like it’s doing the following.or?
void *memmove(void *dest, const void *src, size_t len)
{
+ if (long(len) <= 0)
+ kasan_report_invalid_size(src, dest, len, _RET_IP_);
+
check_memory_region((unsigned long)src, len, false, _RET_IP_);
check_memory_region((unsigned long)dest, len, true, _RET_IP_);
Re: [PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
On Fri, Sep 27, 2019 at 5:43 AM Walter Wu wrote:
>
> memmove() and memcpy() have missing underflow issues.
> When -7 <= size < 0, then KASAN will miss to catch the underflow issue.
> It looks like shadow start address and shadow end address is the same,
> so it does not actually check anything.
>
> The following test is indeed not caught by KASAN:
>
> char *p = kmalloc(64, GFP_KERNEL);
> memset((char *)p, 0, 64);
> memmove((char *)p, (char *)p + 4, -2);
> kfree((char*)p);
>
> It should be checked here:
>
> 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_);
>
> return __memmove(dest, src, len);
> }
>
> We fix the shadow end address which is calculated, then generic KASAN
> get the right shadow end address and detect this underflow issue.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
>
> Signed-off-by: Walter Wu
> Reported-by: Dmitry Vyukov
> ---
> lib/test_kasan.c | 36
> mm/kasan/generic.c | 8 ++--
> 2 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index b63b367a94e8..8bd014852556 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -280,6 +280,40 @@ static noinline void __init kmalloc_oob_in_memset(void)
> kfree(ptr);
> }
>
> +static noinline void __init kmalloc_oob_in_memmove_underflow(void)
> +{
> + char *ptr;
> + size_t size = 64;
> +
> + pr_info("underflow out-of-bounds in memmove\n");
> + ptr = kmalloc(size, GFP_KERNEL);
> + if (!ptr) {
> + pr_err("Allocation failed\n");
> + return;
> + }
> +
> + memset((char *)ptr, 0, 64);
> + memmove((char *)ptr, (char *)ptr + 4, -2);
> + kfree(ptr);
> +}
> +
> +static noinline void __init kmalloc_oob_in_memmove_overflow(void)
> +{
> + char *ptr;
> + size_t size = 64;
> +
> + pr_info("overflow out-of-bounds in memmove\n");
> + ptr = kmalloc(size, GFP_KERNEL);
> + if (!ptr) {
> + pr_err("Allocation failed\n");
> + return;
> + }
> +
> + memset((char *)ptr, 0, 64);
> + memmove((char *)ptr + size, (char *)ptr, 2);
> + kfree(ptr);
> +}
> +
> static noinline void __init kmalloc_uaf(void)
> {
> char *ptr;
> @@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
> kmalloc_oob_memset_4();
> kmalloc_oob_memset_8();
> kmalloc_oob_memset_16();
> + kmalloc_oob_in_memmove_underflow();
> + kmalloc_oob_in_memmove_overflow();
> kmalloc_uaf();
> kmalloc_uaf_memset();
> kmalloc_uaf2();
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 616f9dd82d12..34ca23d59e67 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -131,9 +131,13 @@ static __always_inline bool
> memory_is_poisoned_n(unsigned long addr,
> size_t size)
> {
> unsigned long ret;
> + void *shadow_start = kasan_mem_to_shadow((void *)addr);
> + void *shadow_end = kasan_mem_to_shadow((void *)addr + size - 1) + 1;
>
> - ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
> - kasan_mem_to_shadow((void *)addr + size - 1) + 1);
> + if ((long)size < 0)
> + shadow_end = kasan_mem_to_shadow((void *)addr + size);
Hi Walter,
Thanks for working on this.
If size<0, does it make sense to continue at all? We will still check
1PB of shadow memory? What happens when we pass such huge range to
memory_is_nonzero?
Perhaps it's better to produce an error and bail out immediately if size<0?
Also, what's the failure mode of the tests? Didn't they badly corrupt
memory? We tried to keep tests such that they produce the KASAN
reports, but don't badly corrupt memory b/c/ we need to run all of
them.
> + ret = memory_is_nonzero(shadow_start, shadow_end);
>
> if (unlikely(ret)) {
> unsigned long last_byte = addr + size - 1;
> --
> 2.18.0
>
> --
> You received this message because you are subscribed to the Google Groups
> "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kasan-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kasan-dev/20190927034338.15813-1-walter-zh.wu%40mediatek.com.
[PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y
memmove() and memcpy() have missing underflow issues.
When -7 <= size < 0, then KASAN will miss to catch the underflow issue.
It looks like shadow start address and shadow end address is the same,
so it does not actually check anything.
The following test is indeed not caught by KASAN:
char *p = kmalloc(64, GFP_KERNEL);
memset((char *)p, 0, 64);
memmove((char *)p, (char *)p + 4, -2);
kfree((char*)p);
It should be checked here:
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_);
return __memmove(dest, src, len);
}
We fix the shadow end address which is calculated, then generic KASAN
get the right shadow end address and detect this underflow issue.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=199341
Signed-off-by: Walter Wu
Reported-by: Dmitry Vyukov
---
lib/test_kasan.c | 36
mm/kasan/generic.c | 8 ++--
2 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index b63b367a94e8..8bd014852556 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -280,6 +280,40 @@ static noinline void __init kmalloc_oob_in_memset(void)
kfree(ptr);
}
+static noinline void __init kmalloc_oob_in_memmove_underflow(void)
+{
+ char *ptr;
+ size_t size = 64;
+
+ pr_info("underflow out-of-bounds in memmove\n");
+ ptr = kmalloc(size, GFP_KERNEL);
+ if (!ptr) {
+ pr_err("Allocation failed\n");
+ return;
+ }
+
+ memset((char *)ptr, 0, 64);
+ memmove((char *)ptr, (char *)ptr + 4, -2);
+ kfree(ptr);
+}
+
+static noinline void __init kmalloc_oob_in_memmove_overflow(void)
+{
+ char *ptr;
+ size_t size = 64;
+
+ pr_info("overflow out-of-bounds in memmove\n");
+ ptr = kmalloc(size, GFP_KERNEL);
+ if (!ptr) {
+ pr_err("Allocation failed\n");
+ return;
+ }
+
+ memset((char *)ptr, 0, 64);
+ memmove((char *)ptr + size, (char *)ptr, 2);
+ kfree(ptr);
+}
+
static noinline void __init kmalloc_uaf(void)
{
char *ptr;
@@ -734,6 +768,8 @@ static int __init kmalloc_tests_init(void)
kmalloc_oob_memset_4();
kmalloc_oob_memset_8();
kmalloc_oob_memset_16();
+ kmalloc_oob_in_memmove_underflow();
+ kmalloc_oob_in_memmove_overflow();
kmalloc_uaf();
kmalloc_uaf_memset();
kmalloc_uaf2();
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 616f9dd82d12..34ca23d59e67 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -131,9 +131,13 @@ static __always_inline bool memory_is_poisoned_n(unsigned
long addr,
size_t size)
{
unsigned long ret;
+ void *shadow_start = kasan_mem_to_shadow((void *)addr);
+ void *shadow_end = kasan_mem_to_shadow((void *)addr + size - 1) + 1;
- ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
- kasan_mem_to_shadow((void *)addr + size - 1) + 1);
+ if ((long)size < 0)
+ shadow_end = kasan_mem_to_shadow((void *)addr + size);
+
+ ret = memory_is_nonzero(shadow_start, shadow_end);
if (unlikely(ret)) {
unsigned long last_byte = addr + size - 1;
--
2.18.0
