On Fri, 2019-09-27 at 21:41 +0200, Dmitry Vyukov wrote:

On Fri, Sep 27, 2019 at 4:22 PM Walter Wu [off-list ref] wrote:

quoted

On Fri, 2019-09-27 at 15:07 +0200, Dmitry Vyukov wrote:

quoted

On Fri, Sep 27, 2019 at 5:43 AM Walter Wu [off-list ref] wrote:

quoted

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 <redacted>
Reported-by: Dmitry Vyukov <dvyukov@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);

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.

quoted

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)

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

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.

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

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel