[PATCH 1/4] kasan/tests: add tests for user memory access functions

[PATCH 1/4] kasan/tests: add tests for user memory access functions

[Andrey Ryabinin]

This patch adds some tests for user memory access API.
KASAN doesn't pass these tests yet, but follow on patches will fix that.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
 lib/test_kasan.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index bd75a03..c640fdb 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -12,9 +12,12 @@
 #define pr_fmt(fmt) "kasan test: %s " fmt, __func__
 
 #include <linux/kernel.h>
+#include <linux/mman.h>
+#include <linux/mm.h>
 #include <linux/printk.h>
 #include <linux/slab.h>
 #include <linux/string.h>
+#include <linux/uaccess.h>
 #include <linux/module.h>
 
 static noinline void __init kmalloc_oob_right(void)
@@ -389,6 +392,51 @@ static noinline void __init ksize_unpoisons_memory(void)
 	kfree(ptr);
 }
 
+static noinline void __init copy_user_test(void)
+{
+	char *kmem;
+	char __user *usermem;
+	size_t size = 10;
+	int unused;
+
+	kmem = kmalloc(size, GFP_KERNEL);
+	if (!kmem)
+		return;
+
+	usermem = (char __user *)vm_mmap(NULL, 0, PAGE_SIZE,
+			    PROT_READ | PROT_WRITE | PROT_EXEC,
+			    MAP_ANONYMOUS | MAP_PRIVATE, 0);
+	if (IS_ERR(usermem)) {
+		pr_err("Failed to allocate user memory\n");
+		kfree(kmem);
+		return;
+	}
+
+	pr_info("out-of-bounds in copy_from_user()\n");
+	unused = copy_from_user(kmem, usermem, size + 1);
+
+	pr_info("out-of-bounds in copy_to_user()\n");
+	unused = copy_to_user(usermem, kmem, size + 1);
+
+	pr_info("out-of-bounds in __copy_from_user()\n");
+	unused = __copy_from_user(kmem, usermem, size + 1);
+
+	pr_info("out-of-bounds in __copy_to_user()\n");
+	unused = __copy_to_user(usermem, kmem, size + 1);
+
+	pr_info("out-of-bounds in __copy_from_user_inatomic()\n");
+	unused = __copy_from_user_inatomic(kmem, usermem, size + 1);
+
+	pr_info("out-of-bounds in __copy_to_user_inatomic()\n");
+	unused = __copy_to_user_inatomic(usermem, kmem, size + 1);
+
+	pr_info("out-of-bounds in strncpy_from_user()\n");
+	unused = strncpy_from_user(kmem, usermem, size + 1);
+
+	vm_munmap((unsigned long)usermem, PAGE_SIZE);
+	kfree(kmem);
+}
+
 static int __init kmalloc_tests_init(void)
 {
 	kmalloc_oob_right();
@@ -416,6 +464,7 @@ static int __init kmalloc_tests_init(void)
 	kasan_quarantine_cache();
 #endif
 	ksize_unpoisons_memory();
+	copy_user_test();
 	return -EAGAIN;
 }
 
-- 
2.7.3

[PATCH 2/4] mm/kasan: print name of mem[set,cpy,move]() caller in report

[Andrey Ryabinin]

When bogus memory access happens in mem[set,cpy,move]() it's usually
caller's fault. So don't blame mem[set,cpy,move]() in bug report, blame
the caller instead.

Before:
	BUG: KASAN: out-of-bounds access in memset+0x23/0x40 at <address>
After:
	BUG: KASAN: out-of-bounds access in <memset_caller> at <address>

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
 mm/kasan/kasan.c | 64 ++++++++++++++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index ef2e87b..6e4072c 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -273,32 +273,36 @@ static __always_inline bool memory_is_poisoned(unsigned long addr, size_t size)
 	return memory_is_poisoned_n(addr, size);
 }
 
-
-static __always_inline void check_memory_region(unsigned long addr,
-						size_t size, bool write)
+static __always_inline void check_memory_region_inline(unsigned long addr,
+						size_t size, bool write,
+						unsigned long ret_ip)
 {
 	if (unlikely(size == 0))
 		return;
 
 	if (unlikely((void *)addr <
 		kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) {
-		kasan_report(addr, size, write, _RET_IP_);
+		kasan_report(addr, size, write, ret_ip);
 		return;
 	}
 
 	if (likely(!memory_is_poisoned(addr, size)))
 		return;
 
-	kasan_report(addr, size, write, _RET_IP_);
+	kasan_report(addr, size, write, ret_ip);
 }
 
-void __asan_loadN(unsigned long addr, size_t size);
-void __asan_storeN(unsigned long addr, size_t size);
+static void check_memory_region(unsigned long addr,
+				size_t size, bool write,
+				unsigned long ret_ip)
+{
+	check_memory_region_inline(addr, size, write, ret_ip);
+}
 
 #undef memset
 void *memset(void *addr, int c, size_t len)
 {
-	__asan_storeN((unsigned long)addr, len);
+	check_memory_region((unsigned long)addr, len, true, _RET_IP_);
 
 	return __memset(addr, c, len);
 }
@@ -306,8 +310,8 @@ void *memset(void *addr, int c, size_t len)
 #undef memmove
 void *memmove(void *dest, const void *src, size_t len)
 {
-	__asan_loadN((unsigned long)src, len);
-	__asan_storeN((unsigned long)dest, 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);
 }
@@ -315,8 +319,8 @@ void *memmove(void *dest, const void *src, size_t len)
 #undef memcpy
 void *memcpy(void *dest, const void *src, size_t len)
 {
-	__asan_loadN((unsigned long)src, len);
-	__asan_storeN((unsigned long)dest, len);
+	check_memory_region((unsigned long)src, len, false, _RET_IP_);
+	check_memory_region((unsigned long)dest, len, true, _RET_IP_);
 
 	return __memcpy(dest, src, len);
 }
@@ -698,22 +702,22 @@ void __asan_unregister_globals(struct kasan_global *globals, size_t size)
 }
 EXPORT_SYMBOL(__asan_unregister_globals);
 
-#define DEFINE_ASAN_LOAD_STORE(size)				\
-	void __asan_load##size(unsigned long addr)		\
-	{							\
-		check_memory_region(addr, size, false);		\
-	}							\
-	EXPORT_SYMBOL(__asan_load##size);			\
-	__alias(__asan_load##size)				\
-	void __asan_load##size##_noabort(unsigned long);	\
-	EXPORT_SYMBOL(__asan_load##size##_noabort);		\
-	void __asan_store##size(unsigned long addr)		\
-	{							\
-		check_memory_region(addr, size, true);		\
-	}							\
-	EXPORT_SYMBOL(__asan_store##size);			\
-	__alias(__asan_store##size)				\
-	void __asan_store##size##_noabort(unsigned long);	\
+#define DEFINE_ASAN_LOAD_STORE(size)					\
+	void __asan_load##size(unsigned long addr)			\
+	{								\
+		check_memory_region_inline(addr, size, false, _RET_IP_);\
+	}								\
+	EXPORT_SYMBOL(__asan_load##size);				\
+	__alias(__asan_load##size)					\
+	void __asan_load##size##_noabort(unsigned long);		\
+	EXPORT_SYMBOL(__asan_load##size##_noabort);			\
+	void __asan_store##size(unsigned long addr)			\
+	{								\
+		check_memory_region_inline(addr, size, true, _RET_IP_);	\
+	}								\
+	EXPORT_SYMBOL(__asan_store##size);				\
+	__alias(__asan_store##size)					\
+	void __asan_store##size##_noabort(unsigned long);		\
 	EXPORT_SYMBOL(__asan_store##size##_noabort)
 
 DEFINE_ASAN_LOAD_STORE(1);
@@ -724,7 +728,7 @@ DEFINE_ASAN_LOAD_STORE(16);
 
 void __asan_loadN(unsigned long addr, size_t size)
 {
-	check_memory_region(addr, size, false);
+	check_memory_region(addr, size, false, _RET_IP_);
 }
 EXPORT_SYMBOL(__asan_loadN);
 
@@ -734,7 +738,7 @@ EXPORT_SYMBOL(__asan_loadN_noabort);
 
 void __asan_storeN(unsigned long addr, size_t size)
 {
-	check_memory_region(addr, size, true);
+	check_memory_region(addr, size, true, _RET_IP_);
 }
 EXPORT_SYMBOL(__asan_storeN);
 
-- 
2.7.3

[PATCH 3/4] mm/kasan: add API to check memory regions

[Andrey Ryabinin]

Memory access coded in an assembly won't be seen by KASAN as a compiler
can instrument only C code. Add kasan_check_[read,write]() API
which is going to be used to check a certain memory range.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
 MAINTAINERS                  |  2 +-
 include/linux/kasan-checks.h | 12 ++++++++++++
 mm/kasan/kasan.c             | 12 ++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/kasan-checks.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 43b85c1..3a9471c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6363,7 +6363,7 @@ S:	Maintained
 F:	arch/*/include/asm/kasan.h
 F:	arch/*/mm/kasan_init*
 F:	Documentation/kasan.txt
-F:	include/linux/kasan.h
+F:	include/linux/kasan*.h
 F:	lib/test_kasan.c
 F:	mm/kasan/
 F:	scripts/Makefile.kasan
diff --git a/include/linux/kasan-checks.h b/include/linux/kasan-checks.h
new file mode 100644
index 0000000..b7f8ace
--- /dev/null
+++ b/include/linux/kasan-checks.h
@@ -0,0 +1,12 @@
+#ifndef _LINUX_KASAN_CHECKS_H
+#define _LINUX_KASAN_CHECKS_H
+
+#ifdef CONFIG_KASAN
+void kasan_check_read(const void *p, unsigned int size);
+void kasan_check_write(const void *p, unsigned int size);
+#else
+static inline void kasan_check_read(const void *p, unsigned int size) { }
+static inline void kasan_check_write(const void *p, unsigned int size) { }
+#endif
+
+#endif
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 6e4072c..54f0ea7 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -299,6 +299,18 @@ static void check_memory_region(unsigned long addr,
 	check_memory_region_inline(addr, size, write, ret_ip);
 }
 
+void kasan_check_read(const void *p, unsigned int size)
+{
+	check_memory_region((unsigned long)p, size, false, _RET_IP_);
+}
+EXPORT_SYMBOL(kasan_check_read);
+
+void kasan_check_write(const void *p, unsigned int size)
+{
+	check_memory_region((unsigned long)p, size, true, _RET_IP_);
+}
+EXPORT_SYMBOL(kasan_check_write);
+
 #undef memset
 void *memset(void *addr, int c, size_t len)
 {
-- 
2.7.3

[PATCH 4/4] x86/kasan: Instrument user memory access API

[Andrey Ryabinin]

Exchange between user and kernel memory is coded in assembly language.
Which means that such accesses won't be spotted by KASAN as a compiler
instruments only C code.
Add explicit KASAN checks to user memory access API to ensure that
userspace writes to (or reads from) a valid kernel memory.

Note: Unlike others strncpy_from_user() is written mostly in C and KASAN
sees memory accesses in it. However, it makes sense to add explicit check
for all @count bytes that *potentially* could be written to the kernel.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: x86@kernel.org
---
 arch/x86/include/asm/uaccess.h    | 5 +++++
 arch/x86/include/asm/uaccess_64.h | 7 +++++++
 lib/strncpy_from_user.c           | 2 ++
 3 files changed, 14 insertions(+)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 0b17fad..5dd6d18 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -5,6 +5,7 @@
  */
 #include <linux/errno.h>
 #include <linux/compiler.h>
+#include <linux/kasan-checks.h>
 #include <linux/thread_info.h>
 #include <linux/string.h>
 #include <asm/asm.h>
@@ -732,6 +733,8 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
 
 	might_fault();
 
+	kasan_check_write(to, n);
+
 	/*
 	 * While we would like to have the compiler do the checking for us
 	 * even in the non-constant size case, any false positives there are
@@ -765,6 +768,8 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
 {
 	int sz = __compiletime_object_size(from);
 
+	kasan_check_read(from, n);
+
 	might_fault();
 
 	/* See the comment in copy_from_user() above. */
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 3076986..2eac2aa 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -7,6 +7,7 @@
 #include <linux/compiler.h>
 #include <linux/errno.h>
 #include <linux/lockdep.h>
+#include <linux/kasan-checks.h>
 #include <asm/alternative.h>
 #include <asm/cpufeatures.h>
 #include <asm/page.h>
@@ -109,6 +110,7 @@ static __always_inline __must_check
 int __copy_from_user(void *dst, const void __user *src, unsigned size)
 {
 	might_fault();
+	kasan_check_write(dst, size);
 	return __copy_from_user_nocheck(dst, src, size);
 }
 
@@ -175,6 +177,7 @@ static __always_inline __must_check
 int __copy_to_user(void __user *dst, const void *src, unsigned size)
 {
 	might_fault();
+	kasan_check_read(src, size);
 	return __copy_to_user_nocheck(dst, src, size);
 }
 
@@ -242,12 +245,14 @@ int __copy_in_user(void __user *dst, const void __user *src, unsigned size)
 static __must_check __always_inline int
 __copy_from_user_inatomic(void *dst, const void __user *src, unsigned size)
 {
+	kasan_check_write(dst, size);
 	return __copy_from_user_nocheck(dst, src, size);
 }
 
 static __must_check __always_inline int
 __copy_to_user_inatomic(void __user *dst, const void *src, unsigned size)
 {
+	kasan_check_read(src, size);
 	return __copy_to_user_nocheck(dst, src, size);
 }
 
@@ -258,6 +263,7 @@ static inline int
 __copy_from_user_nocache(void *dst, const void __user *src, unsigned size)
 {
 	might_fault();
+	kasan_check_write(dst, size);
 	return __copy_user_nocache(dst, src, size, 1);
 }
 
@@ -265,6 +271,7 @@ static inline int
 __copy_from_user_inatomic_nocache(void *dst, const void __user *src,
 				  unsigned size)
 {
+	kasan_check_write(dst, size);
 	return __copy_user_nocache(dst, src, size, 0);
 }
 
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 3384032..e3472b0 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -1,5 +1,6 @@
 #include <linux/compiler.h>
 #include <linux/export.h>
+#include <linux/kasan-checks.h>
 #include <linux/uaccess.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -103,6 +104,7 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
 	if (unlikely(count <= 0))
 		return 0;
 
+	kasan_check_write(dst, count);
 	max_addr = user_addr_max();
 	src_addr = (unsigned long)src;
 	if (likely(src_addr < max_addr)) {
-- 
2.7.3

Re: [PATCH 1/4] kasan/tests: add tests for user memory access functions

[Andrew Morton]

On Fri, 6 May 2016 15:45:19 +0300 Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:

> This patch adds some tests for user memory access API.
> KASAN doesn't pass these tests yet, but follow on patches will fix that.

I'll move this patch from [1/4] to [4/4] to avoid the minor bisection
hole.

Re: [PATCH 4/4] x86/kasan: Instrument user memory access API

[Dmitry Vyukov]

On Fri, May 6, 2016 at 2:45 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> Exchange between user and kernel memory is coded in assembly language.
> Which means that such accesses won't be spotted by KASAN as a compiler
> instruments only C code.
> Add explicit KASAN checks to user memory access API to ensure that
> userspace writes to (or reads from) a valid kernel memory.
>
> Note: Unlike others strncpy_from_user() is written mostly in C and KASAN
> sees memory accesses in it. However, it makes sense to add explicit check
> for all @count bytes that *potentially* could be written to the kernel.


Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

Thanks!


> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: x86@kernel.org
> ---
>  arch/x86/include/asm/uaccess.h    | 5 +++++
>  arch/x86/include/asm/uaccess_64.h | 7 +++++++
>  lib/strncpy_from_user.c           | 2 ++
>  3 files changed, 14 insertions(+)
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 0b17fad..5dd6d18 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -5,6 +5,7 @@
>   */
>  #include <linux/errno.h>
>  #include <linux/compiler.h>
> +#include <linux/kasan-checks.h>
>  #include <linux/thread_info.h>
>  #include <linux/string.h>
>  #include <asm/asm.h>
> @@ -732,6 +733,8 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
>
>         might_fault();
>
> +       kasan_check_write(to, n);
> +
>         /*
>          * While we would like to have the compiler do the checking for us
>          * even in the non-constant size case, any false positives there are
> @@ -765,6 +768,8 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
>         int sz = __compiletime_object_size(from);
>
> +       kasan_check_read(from, n);
> +
>         might_fault();
>
>         /* See the comment in copy_from_user() above. */
> diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> index 3076986..2eac2aa 100644
> --- a/arch/x86/include/asm/uaccess_64.h
> +++ b/arch/x86/include/asm/uaccess_64.h
> @@ -7,6 +7,7 @@
>  #include <linux/compiler.h>
>  #include <linux/errno.h>
>  #include <linux/lockdep.h>
> +#include <linux/kasan-checks.h>
>  #include <asm/alternative.h>
>  #include <asm/cpufeatures.h>
>  #include <asm/page.h>
> @@ -109,6 +110,7 @@ static __always_inline __must_check
>  int __copy_from_user(void *dst, const void __user *src, unsigned size)
>  {
>         might_fault();
> +       kasan_check_write(dst, size);
>         return __copy_from_user_nocheck(dst, src, size);
>  }
>
> @@ -175,6 +177,7 @@ static __always_inline __must_check
>  int __copy_to_user(void __user *dst, const void *src, unsigned size)
>  {
>         might_fault();
> +       kasan_check_read(src, size);
>         return __copy_to_user_nocheck(dst, src, size);
>  }
>
> @@ -242,12 +245,14 @@ int __copy_in_user(void __user *dst, const void __user *src, unsigned size)
>  static __must_check __always_inline int
>  __copy_from_user_inatomic(void *dst, const void __user *src, unsigned size)
>  {
> +       kasan_check_write(dst, size);
>         return __copy_from_user_nocheck(dst, src, size);
>  }
>
>  static __must_check __always_inline int
>  __copy_to_user_inatomic(void __user *dst, const void *src, unsigned size)
>  {
> +       kasan_check_read(src, size);
>         return __copy_to_user_nocheck(dst, src, size);
>  }
>
> @@ -258,6 +263,7 @@ static inline int
>  __copy_from_user_nocache(void *dst, const void __user *src, unsigned size)
>  {
>         might_fault();
> +       kasan_check_write(dst, size);
>         return __copy_user_nocache(dst, src, size, 1);
>  }
>
> @@ -265,6 +271,7 @@ static inline int
>  __copy_from_user_inatomic_nocache(void *dst, const void __user *src,
>                                   unsigned size)
>  {
> +       kasan_check_write(dst, size);
>         return __copy_user_nocache(dst, src, size, 0);
>  }
>
> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index 3384032..e3472b0 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -1,5 +1,6 @@
>  #include <linux/compiler.h>
>  #include <linux/export.h>
> +#include <linux/kasan-checks.h>
>  #include <linux/uaccess.h>
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> @@ -103,6 +104,7 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
>         if (unlikely(count <= 0))
>                 return 0;
>
> +       kasan_check_write(dst, count);
>         max_addr = user_addr_max();
>         src_addr = (unsigned long)src;
>         if (likely(src_addr < max_addr)) {
> --
> 2.7.3
>

Re: [PATCH 4/4] x86/kasan: Instrument user memory access API

[Ingo Molnar]

* Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:

> Exchange between user and kernel memory is coded in assembly language.
> Which means that such accesses won't be spotted by KASAN as a compiler
> instruments only C code.
> Add explicit KASAN checks to user memory access API to ensure that
> userspace writes to (or reads from) a valid kernel memory.
> 
> Note: Unlike others strncpy_from_user() is written mostly in C and KASAN
> sees memory accesses in it. However, it makes sense to add explicit check
> for all @count bytes that *potentially* could be written to the kernel.
> 
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: x86@kernel.org
> ---
>  arch/x86/include/asm/uaccess.h    | 5 +++++
>  arch/x86/include/asm/uaccess_64.h | 7 +++++++
>  lib/strncpy_from_user.c           | 2 ++
>  3 files changed, 14 insertions(+)

[...]

> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index 3384032..e3472b0 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -1,5 +1,6 @@
>  #include <linux/compiler.h>
>  #include <linux/export.h>
> +#include <linux/kasan-checks.h>
>  #include <linux/uaccess.h>
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> @@ -103,6 +104,7 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
>  	if (unlikely(count <= 0))
>  		return 0;
>  
> +	kasan_check_write(dst, count);
>  	max_addr = user_addr_max();
>  	src_addr = (unsigned long)src;
>  	if (likely(src_addr < max_addr)) {

Please do the check inside the condition, before the user_access_begin(), because 
where you've put the check we might still fail and not do a user copy and -EFAULT 
out.

With that fixed:

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[PATCH] x86-kasan-instrument-user-memory-access-api-fix

[Andrey Ryabinin]

Move kasan check under the condition, otherwise we may fail and not
do a user copy.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 lib/strncpy_from_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index e3472b0..33f655e 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -104,13 +104,13 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
 	if (unlikely(count <= 0))
 		return 0;
 
-	kasan_check_write(dst, count);
 	max_addr = user_addr_max();
 	src_addr = (unsigned long)src;
 	if (likely(src_addr < max_addr)) {
 		unsigned long max = max_addr - src_addr;
 		long retval;
 
+		kasan_check_write(dst, count);
 		user_access_begin();
 		retval = do_strncpy_from_user(dst, src, count, max);
 		user_access_end();
-- 
2.7.3

Re: [PATCH 2/4] mm/kasan: print name of mem[set,cpy,move]() caller in report

[Alexander Potapenko]

On Fri, May 6, 2016 at 2:45 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> When bogus memory access happens in mem[set,cpy,move]() it's usually
> caller's fault. So don't blame mem[set,cpy,move]() in bug report, blame
> the caller instead.
>
> Before:
>         BUG: KASAN: out-of-bounds access in memset+0x23/0x40 at <address>
> After:
>         BUG: KASAN: out-of-bounds access in <memset_caller> at <address>
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> ---
>  mm/kasan/kasan.c | 64 ++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 34 insertions(+), 30 deletions(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..6e4072c 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -273,32 +273,36 @@ static __always_inline bool memory_is_poisoned(unsigned long addr, size_t size)
>         return memory_is_poisoned_n(addr, size);
>  }
>
> -
> -static __always_inline void check_memory_region(unsigned long addr,
> -                                               size_t size, bool write)
> +static __always_inline void check_memory_region_inline(unsigned long addr,
> +                                               size_t size, bool write,
> +                                               unsigned long ret_ip)
>  {
>         if (unlikely(size == 0))
>                 return;
>
>         if (unlikely((void *)addr <
>                 kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) {
> -               kasan_report(addr, size, write, _RET_IP_);
> +               kasan_report(addr, size, write, ret_ip);
>                 return;
>         }
>
>         if (likely(!memory_is_poisoned(addr, size)))
>                 return;
>
> -       kasan_report(addr, size, write, _RET_IP_);
> +       kasan_report(addr, size, write, ret_ip);
>  }
>
> -void __asan_loadN(unsigned long addr, size_t size);
> -void __asan_storeN(unsigned long addr, size_t size);
> +static void check_memory_region(unsigned long addr,
> +                               size_t size, bool write,
> +                               unsigned long ret_ip)
> +{
> +       check_memory_region_inline(addr, size, write, ret_ip);
> +}
>
>  #undef memset
>  void *memset(void *addr, int c, size_t len)
>  {
> -       __asan_storeN((unsigned long)addr, len);
> +       check_memory_region((unsigned long)addr, len, true, _RET_IP_);
>
>         return __memset(addr, c, len);
>  }
> @@ -306,8 +310,8 @@ void *memset(void *addr, int c, size_t len)
>  #undef memmove
>  void *memmove(void *dest, const void *src, size_t len)
>  {
> -       __asan_loadN((unsigned long)src, len);
> -       __asan_storeN((unsigned long)dest, 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);
>  }
> @@ -315,8 +319,8 @@ void *memmove(void *dest, const void *src, size_t len)
>  #undef memcpy
>  void *memcpy(void *dest, const void *src, size_t len)
>  {
> -       __asan_loadN((unsigned long)src, len);
> -       __asan_storeN((unsigned long)dest, len);
> +       check_memory_region((unsigned long)src, len, false, _RET_IP_);
> +       check_memory_region((unsigned long)dest, len, true, _RET_IP_);
>
>         return __memcpy(dest, src, len);
>  }
> @@ -698,22 +702,22 @@ void __asan_unregister_globals(struct kasan_global *globals, size_t size)
>  }
>  EXPORT_SYMBOL(__asan_unregister_globals);
>
> -#define DEFINE_ASAN_LOAD_STORE(size)                           \
> -       void __asan_load##size(unsigned long addr)              \
> -       {                                                       \
> -               check_memory_region(addr, size, false);         \
> -       }                                                       \
> -       EXPORT_SYMBOL(__asan_load##size);                       \
> -       __alias(__asan_load##size)                              \
> -       void __asan_load##size##_noabort(unsigned long);        \
> -       EXPORT_SYMBOL(__asan_load##size##_noabort);             \
> -       void __asan_store##size(unsigned long addr)             \
> -       {                                                       \
> -               check_memory_region(addr, size, true);          \
> -       }                                                       \
> -       EXPORT_SYMBOL(__asan_store##size);                      \
> -       __alias(__asan_store##size)                             \
> -       void __asan_store##size##_noabort(unsigned long);       \
> +#define DEFINE_ASAN_LOAD_STORE(size)                                   \
> +       void __asan_load##size(unsigned long addr)                      \
> +       {                                                               \
> +               check_memory_region_inline(addr, size, false, _RET_IP_);\
> +       }                                                               \
> +       EXPORT_SYMBOL(__asan_load##size);                               \
> +       __alias(__asan_load##size)                                      \
> +       void __asan_load##size##_noabort(unsigned long);                \
> +       EXPORT_SYMBOL(__asan_load##size##_noabort);                     \
> +       void __asan_store##size(unsigned long addr)                     \
> +       {                                                               \
> +               check_memory_region_inline(addr, size, true, _RET_IP_); \
> +       }                                                               \
> +       EXPORT_SYMBOL(__asan_store##size);                              \
> +       __alias(__asan_store##size)                                     \
> +       void __asan_store##size##_noabort(unsigned long);               \
>         EXPORT_SYMBOL(__asan_store##size##_noabort)
>
>  DEFINE_ASAN_LOAD_STORE(1);
> @@ -724,7 +728,7 @@ DEFINE_ASAN_LOAD_STORE(16);
>
>  void __asan_loadN(unsigned long addr, size_t size)
>  {
> -       check_memory_region(addr, size, false);
> +       check_memory_region(addr, size, false, _RET_IP_);
>  }
>  EXPORT_SYMBOL(__asan_loadN);
>
> @@ -734,7 +738,7 @@ EXPORT_SYMBOL(__asan_loadN_noabort);
>
>  void __asan_storeN(unsigned long addr, size_t size)
>  {
> -       check_memory_region(addr, size, true);
> +       check_memory_region(addr, size, true, _RET_IP_);
>  }
>  EXPORT_SYMBOL(__asan_storeN);
>
> --Reviewed-by:
> 2.7.3
>
Acked-by: Alexander Potapenko <glider@google.com>



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH 3/4] mm/kasan: add API to check memory regions

[Alexander Potapenko]

On Fri, May 6, 2016 at 2:45 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> Memory access coded in an assembly won't be seen by KASAN as a compiler
> can instrument only C code. Add kasan_check_[read,write]() API
> which is going to be used to check a certain memory range.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> ---
>  MAINTAINERS                  |  2 +-
>  include/linux/kasan-checks.h | 12 ++++++++++++
>  mm/kasan/kasan.c             | 12 ++++++++++++
>  3 files changed, 25 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/kasan-checks.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 43b85c1..3a9471c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6363,7 +6363,7 @@ S:        Maintained
>  F:     arch/*/include/asm/kasan.h
>  F:     arch/*/mm/kasan_init*
>  F:     Documentation/kasan.txt
> -F:     include/linux/kasan.h
> +F:     include/linux/kasan*.h
>  F:     lib/test_kasan.c
>  F:     mm/kasan/
>  F:     scripts/Makefile.kasan
> diff --git a/include/linux/kasan-checks.h b/include/linux/kasan-checks.h
> new file mode 100644
> index 0000000..b7f8ace
> --- /dev/null
> +++ b/include/linux/kasan-checks.h
> @@ -0,0 +1,12 @@
> +#ifndef _LINUX_KASAN_CHECKS_H
> +#define _LINUX_KASAN_CHECKS_H
> +
> +#ifdef CONFIG_KASAN
> +void kasan_check_read(const void *p, unsigned int size);
> +void kasan_check_write(const void *p, unsigned int size);
> +#else
> +static inline void kasan_check_read(const void *p, unsigned int size) { }
> +static inline void kasan_check_write(const void *p, unsigned int size) { }
> +#endif
> +
> +#endif
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 6e4072c..54f0ea7 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -299,6 +299,18 @@ static void check_memory_region(unsigned long addr,
>         check_memory_region_inline(addr, size, write, ret_ip);
>  }
>
> +void kasan_check_read(const void *p, unsigned int size)
> +{
> +       check_memory_region((unsigned long)p, size, false, _RET_IP_);
> +}
> +EXPORT_SYMBOL(kasan_check_read);
> +
> +void kasan_check_write(const void *p, unsigned int size)
> +{
> +       check_memory_region((unsigned long)p, size, true, _RET_IP_);
> +}
> +EXPORT_SYMBOL(kasan_check_write);
> +
>  #undef memset
>  void *memset(void *addr, int c, size_t len)
>  {
> --
> 2.7.3
>
Acked-by: Alexander Potapenko <glider@google.com>


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg