[PATCH] kasan: fix the missing underflow in memmove and memcpy with CONFIG_KASAN_GENERIC=y

[Walter Wu]

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 <walter...@mediatek.com>
Reported-by: Dmitry Vyukov <dvy...@google.com>
---
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

[Dmitry Vyukov]

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+...@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.

[Walter Wu]

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

[Dmitry Vyukov]

/\/\/\/\/\/\

This check needs to be inside of check_memory_region, otherwise we
will have similar problems in all other places that use
check_memory_region.
But check_memory_region already returns a bool, so we could check that
bool and return early.

[Walter Wu]

Thanks for your reminder.

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

}

> But check_memory_region already returns a bool, so we could check that
> bool and return early.

When size<0, we should only show one KASAN report, and should we only
limit to return when size<0 is true? If yse, then __memmove() will do
nothing.

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_)
+ && long(size) < 0)
+ return;
+

check_memory_region((unsigned long)dest, len, true, _RET_IP_);

return __memmove(dest, src, len);

>
>

[Walter Wu]

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.

[Dmitry Vyukov]

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

[Walter Wu]

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.

[Walter Wu]

Hi Dmitry,

What do you think the following code is better than the above one.
In memmmove/memset/memcpy, they need to determine whether size < 0 is
true. we directly determine whether size is negative in memmove and
return early. it avoid to generate repeated KASAN report. Is it better?

void *memmove(void *dest, const void *src, size_t len)
{

+ if (long(size) < 0) {
+ kasan_report_invalid_size(src, dest, len, _RET_IP_);

+ return;
+ }

+
check_memory_region((unsigned long)src, len, false, _RET_IP_);
check_memory_region((unsigned long)dest, len, true, _RET_IP_);

check_memory_region() still has to check whether the size is negative.
but memmove/memset/memcpy generate invalid size KASAN report will not be
there.

[Dmitry Vyukov]

If check_memory_region() will do the check, why do we need to
duplicate it inside of memmove and all other range functions?

I would do:

void *memmove(void *dest, const void *src, size_t len)
{

if (check_memory_region((unsigned long)src, len, false, _RET_IP_))
return;

This avoids duplicating the check, adds minimal amount of code to
range functions and avoids adding kasan_report_invalid_size.

[Walter Wu]

Yes, I know it has duplication, but if we don't have to determine size<0
in memmove, then all check_memory_region return false will do nothing,
it includes other memory corruption behaviors, this is my original
concern.

> I would do:
>
> void *memmove(void *dest, const void *src, size_t len)
> {
> if (check_memory_region((unsigned long)src, len, false, _RET_IP_))
> return;

if check_memory_region return TRUE is to do nothing, but it is no memory
corruption? Should it return early when check_memory_region return a
FALSE?

>
> This avoids duplicating the check, adds minimal amount of code to
> range functions and avoids adding kasan_report_invalid_size.

Thanks for your suggestion.
We originally want to show complete information(destination address,
source address, and its length), but add minimal amount of code into
kasan_report(), it should be good.

[Dmitry Vyukov]

But they will produce a KASAN report, right? They are asked to check
if 18446744073709551614 bytes are good. 18446744073709551614 bytes
can't be good.

> it includes other memory corruption behaviors, this is my original
> concern.
>
> > I would do:
> >
> > void *memmove(void *dest, const void *src, size_t len)
> > {
> > if (check_memory_region((unsigned long)src, len, false, _RET_IP_))
> > return;
> if check_memory_region return TRUE is to do nothing, but it is no memory
> corruption? Should it return early when check_memory_region return a
> FALSE?

Maybe. I just meant the overall idea: check_memory_region should
detect that 18446744073709551614 bytes are bad, print an error, return
an indication that bytes were bad, memmove should return early if the
range is bad.

> > This avoids duplicating the check, adds minimal amount of code to
> > range functions and avoids adding kasan_report_invalid_size.
> Thanks for your suggestion.
> We originally want to show complete information(destination address,
> source address, and its length), but add minimal amount of code into
> kasan_report(), it should be good.
>
>

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1570069078.19702.57.camel%40mtksdccf07.

[Walter Wu]

ok, i will send new patch.
Thanks for your review.

[Walter Wu]

how about this?

commit fd64691026e7ccb8d2946d0804b0621ac177df38
Author: Walter Wu <walter...@mediatek.com>
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-00001-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 <walter...@mediatek.com>
Reported-by: Dmitry Vyukov <dvy...@google.com>

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;

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 ? "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);
+ } else {
+ 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));
+ }
}

static DEFINE_SPINLOCK(report_lock);
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 0e987c9ca052..b829535a3ad7 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -86,6 +86,11 @@ bool check_memory_region(unsigned long addr, size_t
size, bool write,
if (unlikely(size == 0))
return true;

+ if (unlikely((long)size < 0)) {
+ kasan_report(addr, size, write, ret_ip);

+ return false;
+ }
+

tag = get_tag((const void *)addr);

/*

[Dmitry Vyukov]

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.

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).

> + } else {
> + 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));
> + }
> }
>
> static DEFINE_SPINLOCK(report_lock);
> diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> index 0e987c9ca052..b829535a3ad7 100644
> --- a/mm/kasan/tags.c
> +++ b/mm/kasan/tags.c
> @@ -86,6 +86,11 @@ bool check_memory_region(unsigned long addr, size_t
> size, bool write,
> if (unlikely(size == 0))
> return true;
>
> + if (unlikely((long)size < 0)) {
> + kasan_report(addr, size, write, ret_ip);
> + return false;
> + }
> +
> tag = get_tag((const void *)addr);
>
> /*
>
>

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1570110681.19702.64.camel%40mtksdccf07.

[Marc Gonzalez]

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);
> }

[Walter Wu]

agreed

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.

[Walter Wu]

Updated my patch, please help to review it.
thanks.

commit 13e10a7e4264eb25c5a14193068027afc9c261f6
Author: Walter-zh Wu <walter...@mediatek.com>
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 ffffff8069660904 by task
cat/72

CPU: 2 PID: 72 Comm: cat Not tainted
5.4.0-rc1-next-20191004ajb-00001-gdb8af2f372b2-dirty #1

Hardware name: linux,dummy-virt (DT)
Call trace:

dump_backtrace+0x0/0x288
show_stack+0x14/0x20
dump_stack+0x10c/0x164
print_address_description.isra.9+0x68/0x378
__kasan_report+0x164/0x1a0
kasan_report+0xc/0x18
check_memory_region+0x174/0x1d0

memmove+0x34/0x88
kmalloc_memmove_invalid_size+0x70/0xa0

[1] https://bugzilla.kernel.org/show_bug.cgi?id=199341

Signed-off-by: Walter Wu <walter...@mediatek.com>
Reported -by: Dmitry Vyukov <dvy...@google.com>

Suggested-by: Dmitry Vyukov <dvy...@google.com>

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, 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/generic_report.c b/mm/kasan/generic_report.c
index 36c645939bc9..ae9596210394 100644
--- a/mm/kasan/generic_report.c
+++ b/mm/kasan/generic_report.c
@@ -107,6 +107,13 @@ static const char *get_wild_bug_type(struct
kasan_access_info *info)

const char *get_bug_type(struct kasan_access_info *info)
{
+ /*
+ * if access_size < 0, then it will be larger than ULONG_MAX/2,
+ * so that this can qualify as out-of-bounds.
+ */

+ if ((long)info->access_size < 0)

+ return "out-of-bounds";
+
if (addr_has_shadow(info->access_addr))
return get_shadow_bug_type(info);
return get_wild_bug_type(info);

diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 0e987c9ca052..b829535a3ad7 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -86,6 +86,11 @@ bool check_memory_region(unsigned long addr, size_t
size, bool write,

if (unlikely(size == 0))
return true;

+ if (unlikely((long)size < 0)) {
+ kasan_report(addr, size, write, ret_ip);
+ return false;
+ }
+

tag = get_tag((const void *)addr);

/*

diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c
index 969ae08f59d7..1e1ca81214b5 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,6 +36,13 @@

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.
+ */

+ if ((long)info->access_size < 0)

+ return "out-of-bounds";
+
#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
struct kasan_alloc_meta *alloc_meta;
struct kmem_cache *cache;

[Dmitry Vyukov]

I would check both calls.
The current code seems to be over-specialized for handling of invalid
size (you assume that if it's invalid size, then the first
check_memory_region will detect it and checking the second one is
pointless, right?).
But check_memory_region can return false in other cases too.
Also seeing first call checked, but the second not checked just hurts
my eyes when reading code (whenever I will read such code my first
reaction will be "why?").

"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...

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1570176131.19702.105.camel%40mtksdccf07.

[Walter Wu]

I can't agree with you any more about second point.

#undef memmove
void *memmove(void *dest, const void *src, size_t len)
{

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

maybe i should add your comment into the comment in get_bug_type?

[Dmitry Vyukov]

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)."

;)

[Walter Wu]

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.15...@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 <walter...@mediatek.com>
Date: Fri Oct 4 18:38:31 2019 +0800

kasan: detect invalid size in memory operation function

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)
{
+ /*
+ * 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";
+

if (addr_has_shadow(info->access_addr))
return get_shadow_bug_type(info);
return get_wild_bug_type(info);
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 0e987c9ca052..b829535a3ad7 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -86,6 +86,11 @@ bool check_memory_region(unsigned long addr, size_t
size, bool write,
if (unlikely(size == 0))
return true;

+ if (unlikely((long)size < 0)) {
+ kasan_report(addr, size, write, ret_ip);
+ return false;
+ }
+
tag = get_tag((const void *)addr);

/*
diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c

index 969ae08f59d7..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";
+
#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
struct kasan_alloc_meta *alloc_meta;
struct kmem_cache *cache;

commit fb5cf7bd16e939d1feef229af0211a8616c9ea03
Author: Walter Wu <walter...@mediatek.com>
Date: Fri Oct 4 18:32:03 2019 +0800

kasan: add test for invalid size in memmove

Test size is negative vaule in memmove in order to verify
if it correctly produce KASAN report.

Signed-off-by: Walter Wu <walter...@mediatek.com>

[Dmitry Vyukov]

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 :)

[Walter Wu]

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."

>
>
>

[Dmitry Vyukov]

Looks good to me. I think it should provide enough hooks for future
readers to understand why we do this.

[Walter Wu]

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.15...@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 <walter...@mediatek.com>

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.

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 char *get_bug_type(struct kasan_access_info *info)
{
+ /*

+ * If access_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.

+ */
+ if ((long)info->access_size < 0)
+ return "out-of-bounds";
+
if (addr_has_shadow(info->access_addr))
return get_shadow_bug_type(info);
return get_wild_bug_type(info);
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 0e987c9ca052..b829535a3ad7 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -86,6 +86,11 @@ bool check_memory_region(unsigned long addr, size_t
size, bool write,
if (unlikely(size == 0))
return true;

+ if (unlikely((long)size < 0)) {
+ kasan_report(addr, size, write, ret_ip);
+ return false;
+ }
+
tag = get_tag((const void *)addr);

/*
diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c

index 969ae08f59d7..012fbe3a793f 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,6 +36,18 @@

const char *get_bug_type(struct kasan_access_info *info)
{
+ /*

+ * If access_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.

+ */
+ if ((long)info->access_size < 0)
+ return "out-of-bounds";
+

#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
struct kasan_alloc_meta *alloc_meta;
struct kmem_cache *cache;








commit fb5cf7bd16e939d1feef229af0211a8616c9ea03

Author: Walter-zh Wu <walter...@mediatek.com>

Date: Fri Oct 4 18:32:03 2019 +0800

kasan: add test for invalid size in memmove

Test size is negative vaule in memmove in order to verify

if it correctly get KASAN report.

[Dmitry Vyukov]

"out-of-bounds" is the _least_ frequent KASAN bug type. It won't
prevent duplicates. "heap-out-of-bounds" is the frequent one.

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1570436289.4686.40.camel%40mtksdccf07.

[Walter Wu]

/*

* If access_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. "out-of-bounds" is the _least_
frequent KASAN bug type.
* It won't prevent duplicates. "heap-out-of-bounds" is the
frequent one.
*/

We directly add it into the comment.

[Dmitry Vyukov]

OK, let's start from the beginning: why do you return "out-of-bounds" here?

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1570438317.4686.44.camel%40mtksdccf07.

[Walter Wu]

Uh, comment 1 and 2 should explain it. :)

[Dmitry Vyukov]

The comment says it will cause duplicate reports. It does not explain
why you want syzbot to produce duplicate reports and spam kernel
developers... So why do you want that?

[Walter Wu]

We don't generate new bug type in order to prevent duplicate by some
systems, e.g. syzbot. Is it right? If yes, then it should not have
duplicate report.

[Walter Wu]

Sorry, because we don't generate new bug type. it should be duplicate
report(only one report which may be oob or size invlid),
the duplicate report goal is that invalid size is oob issue, too.

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.

[Dmitry Vyukov]

To prevent duplicates, the new crash title must not just match _any_
crash title that kernel can potentially produce. It must match exactly
the crash that kernel produces for this bug on other input data.

Consider, userspace passes size=123, KASAN produces "heap-out-of-bounds in foo".
Now userspace passes size=-1 and KASAN produces "invalid-size in foo".
This will be a duplicate bug report.
Now if KASAN will produce "out-of-bounds in foo", it will also lead to
a duplicate report.
Only iff KASAN will produce "heap-out-of-bounds in foo" for size=-1,
it will not lead to a duplicate report.

[Walter Wu]

I think it is not easy to avoid the duplicate report(mentioned above).
As far as my knowledge is concerned, KASAN is memory corruption detector
in kernel space, it should only detect memory corruption and don't
distinguish whether it is passed by userspace. if we want to do, then we
may need to parse backtrace to check if it has copy_form_user() or other
function?

[Dmitry Vyukov]

My idea was just to always print "heap-out-of-bounds" and don't
differentiate if the size come from userspace or not.

[Walter Wu]

Got it.
Would you have any other concern about this patch?

[Dmitry Vyukov]

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.

[Walter Wu]

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.15...@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 <walter...@mediatek.com>
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

index 36c645939bc9..52a92c7db697 100644
--- a/mm/kasan/generic_report.c
+++ b/mm/kasan/generic_report.c
@@ -107,6 +107,24 @@ static const char *get_wild_bug_type(struct

kasan_access_info *info)

const char *get_bug_type(struct kasan_access_info *info)
{
+ /*

+ * If access_size is negative numbers, then it has three reasons
+ * to be defined as heap-out-of-bounds bug type.

+ * 1) Casting negative numbers to size_t would indeed turn up as
+ * a large size_t and its value will be larger than ULONG_MAX/2,
+ * so that this can qualify as out-of-bounds.

+ * 2) If KASAN has new bug type and user-space passes negative size,
+ * then there are duplicate reports. So don't produce new bug type
+ * in order to prevent duplicate reports by some systems
+ * (e.g. syzbot) to report the same bug twice.
+ * 3) When size is negative numbers, it may be passed from user-space.
+ * So we always print heap-out-of-bounds in order to prevent that
+ * kernel-space and user-space have the same bug but have duplicate
+ * reports.

+ */
+ if ((long)info->access_size < 0)

+ return "heap-out-of-bounds";
+

if (addr_has_shadow(info->access_addr))
return get_shadow_bug_type(info);
return get_wild_bug_type(info);
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 0e987c9ca052..b829535a3ad7 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -86,6 +86,11 @@ bool check_memory_region(unsigned long addr, size_t
size, bool write,

if (unlikely(size == 0))
return true;

+ if (unlikely((long)size < 0)) {
+ kasan_report(addr, size, write, ret_ip);
+ return false;
+ }
+

tag = get_tag((const void *)addr);

/*
diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c

index 969ae08f59d7..f7ae474aef3a 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,6 +36,24 @@

const char *get_bug_type(struct kasan_access_info *info)
{
+ /*

+ * If access_size is negative numbers, then it has three reasons
+ * to be defined as heap-out-of-bounds bug type.

+ * 1) Casting negative numbers to size_t would indeed turn up as
+ * a large size_t and its value will be larger than ULONG_MAX/2,
+ * so that this can qualify as out-of-bounds.

+ * 2) If KASAN has new bug type and user-space passes negative size,
+ * then there are duplicate reports. So don't produce new bug type
+ * in order to prevent duplicate reports by some systems
+ * (e.g. syzbot) to report the same bug twice.
+ * 3) When size is negative numbers, it may be passed from user-space.
+ * So we always print heap-out-of-bounds in order to prevent that
+ * kernel-space and user-space have the same bug but have duplicate
+ * reports.

+ */
+ if ((long)info->access_size < 0)

+ return "heap-out-of-bounds";

+
#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
struct kasan_alloc_meta *alloc_meta;
struct kmem_cache *cache;





commit fb5cf7bd16e939d1feef229af0211a8616c9ea03

Author: Walter-zh Wu <walter...@mediatek.com>
Date: Fri Oct 4 18:32:03 2019 +0800

kasan: add test for invalid size in memmove

[Qian Cai]

> On Oct 8, 2019, at 2:16 AM, Walter Wu <walter...@mediatek.com> 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?

[Walter Wu]

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.

[Qian Cai]

> On Oct 8, 2019, at 7:02 AM, Walter Wu <walter...@mediatek.com> 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.

[Walter Wu]

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.

[Dmitry Vyukov]

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?

[Walter Wu]

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.