linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: lib: use C string functions with KASAN enabled.
@ 2018-09-06 17:05 Andrey Ryabinin
  2018-09-06 17:05 ` [PATCH] lib/test_kasan: Add tests for several string/memory API functions Andrey Ryabinin
  2018-09-07 14:56 ` [PATCH] arm64: lib: use C string functions with KASAN enabled Will Deacon
  0 siblings, 2 replies; 16+ messages in thread
From: Andrey Ryabinin @ 2018-09-06 17:05 UTC (permalink / raw)
  To: Will Deacon, Catalin Marinas
  Cc: Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Andrey Ryabinin

ARM64 has asm implementations of memchr(), memcmp(), str[r]chr(),
str[n]cmp(), str[n]len(). KASAN don't see memory accesses in asm
code, thus it can potentially miss many bugs.

Ifdef out __HAVE_ARCH_* defines of these functions when KASAN is
enabled, so the generic implementations from lib/string.c will be used.

Declare asm functions as weak instead of removing them because they
still can be used by efistub.

Reported-by: Kyeongdon Kim <kyeongdon.kim@lge.com>
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 arch/arm64/include/asm/string.h | 14 ++++++++------
 arch/arm64/kernel/arm64ksyms.c  |  7 +++++--
 arch/arm64/lib/memchr.S         |  1 +
 arch/arm64/lib/memcmp.S         |  1 +
 arch/arm64/lib/strchr.S         |  1 +
 arch/arm64/lib/strcmp.S         |  1 +
 arch/arm64/lib/strlen.S         |  1 +
 arch/arm64/lib/strncmp.S        |  1 +
 arch/arm64/lib/strnlen.S        |  1 +
 arch/arm64/lib/strrchr.S        |  1 +
 10 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/string.h b/arch/arm64/include/asm/string.h
index dd95d33a5bd5..03a6c256b7ec 100644
--- a/arch/arm64/include/asm/string.h
+++ b/arch/arm64/include/asm/string.h
@@ -16,6 +16,7 @@
 #ifndef __ASM_STRING_H
 #define __ASM_STRING_H
 
+#ifndef CONFIG_KASAN
 #define __HAVE_ARCH_STRRCHR
 extern char *strrchr(const char *, int c);
 
@@ -34,6 +35,13 @@ extern __kernel_size_t strlen(const char *);
 #define __HAVE_ARCH_STRNLEN
 extern __kernel_size_t strnlen(const char *, __kernel_size_t);
 
+#define __HAVE_ARCH_MEMCMP
+extern int memcmp(const void *, const void *, size_t);
+
+#define __HAVE_ARCH_MEMCHR
+extern void *memchr(const void *, int, __kernel_size_t);
+#endif
+
 #define __HAVE_ARCH_MEMCPY
 extern void *memcpy(void *, const void *, __kernel_size_t);
 extern void *__memcpy(void *, const void *, __kernel_size_t);
@@ -42,16 +50,10 @@ extern void *__memcpy(void *, const void *, __kernel_size_t);
 extern void *memmove(void *, const void *, __kernel_size_t);
 extern void *__memmove(void *, const void *, __kernel_size_t);
 
-#define __HAVE_ARCH_MEMCHR
-extern void *memchr(const void *, int, __kernel_size_t);
-
 #define __HAVE_ARCH_MEMSET
 extern void *memset(void *, int, __kernel_size_t);
 extern void *__memset(void *, int, __kernel_size_t);
 
-#define __HAVE_ARCH_MEMCMP
-extern int memcmp(const void *, const void *, size_t);
-
 #ifdef CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE
 #define __HAVE_ARCH_MEMCPY_FLUSHCACHE
 void memcpy_flushcache(void *dst, const void *src, size_t cnt);
diff --git a/arch/arm64/kernel/arm64ksyms.c b/arch/arm64/kernel/arm64ksyms.c
index d894a20b70b2..72f63a59b008 100644
--- a/arch/arm64/kernel/arm64ksyms.c
+++ b/arch/arm64/kernel/arm64ksyms.c
@@ -44,20 +44,23 @@ EXPORT_SYMBOL(__arch_copy_in_user);
 EXPORT_SYMBOL(memstart_addr);
 
 	/* string / mem functions */
+#ifndef CONFIG_KASAN
 EXPORT_SYMBOL(strchr);
 EXPORT_SYMBOL(strrchr);
 EXPORT_SYMBOL(strcmp);
 EXPORT_SYMBOL(strncmp);
 EXPORT_SYMBOL(strlen);
 EXPORT_SYMBOL(strnlen);
+EXPORT_SYMBOL(memcmp);
+EXPORT_SYMBOL(memchr);
+#endif
+
 EXPORT_SYMBOL(memset);
 EXPORT_SYMBOL(memcpy);
 EXPORT_SYMBOL(memmove);
 EXPORT_SYMBOL(__memset);
 EXPORT_SYMBOL(__memcpy);
 EXPORT_SYMBOL(__memmove);
-EXPORT_SYMBOL(memchr);
-EXPORT_SYMBOL(memcmp);
 
 	/* atomic bitops */
 EXPORT_SYMBOL(set_bit);
diff --git a/arch/arm64/lib/memchr.S b/arch/arm64/lib/memchr.S
index 4444c1d25f4b..b790ec0228f6 100644
--- a/arch/arm64/lib/memchr.S
+++ b/arch/arm64/lib/memchr.S
@@ -30,6 +30,7 @@
  * Returns:
  *	x0 - address of first occurrence of 'c' or 0
  */
+.weak memchr
 ENTRY(memchr)
 	and	w1, w1, #0xff
 1:	subs	x2, x2, #1
diff --git a/arch/arm64/lib/memcmp.S b/arch/arm64/lib/memcmp.S
index 2a4e239bd17a..de9975b0afda 100644
--- a/arch/arm64/lib/memcmp.S
+++ b/arch/arm64/lib/memcmp.S
@@ -58,6 +58,7 @@ pos		.req	x11
 limit_wd	.req	x12
 mask		.req	x13
 
+.weak memcmp
 ENTRY(memcmp)
 	cbz	limit, .Lret0
 	eor	tmp1, src1, src2
diff --git a/arch/arm64/lib/strchr.S b/arch/arm64/lib/strchr.S
index dae0cf5591f9..10799adb8d5f 100644
--- a/arch/arm64/lib/strchr.S
+++ b/arch/arm64/lib/strchr.S
@@ -29,6 +29,7 @@
  * Returns:
  *	x0 - address of first occurrence of 'c' or 0
  */
+.weak strchr
 ENTRY(strchr)
 	and	w1, w1, #0xff
 1:	ldrb	w2, [x0], #1
diff --git a/arch/arm64/lib/strcmp.S b/arch/arm64/lib/strcmp.S
index 471fe61760ef..5629b4fa5431 100644
--- a/arch/arm64/lib/strcmp.S
+++ b/arch/arm64/lib/strcmp.S
@@ -60,6 +60,7 @@ tmp3		.req	x9
 zeroones	.req	x10
 pos		.req	x11
 
+.weak strcmp
 ENTRY(strcmp)
 	eor	tmp1, src1, src2
 	mov	zeroones, #REP8_01
diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
index 55ccc8e24c08..f00df4b1b8d9 100644
--- a/arch/arm64/lib/strlen.S
+++ b/arch/arm64/lib/strlen.S
@@ -56,6 +56,7 @@ pos		.req	x12
 #define REP8_7f 0x7f7f7f7f7f7f7f7f
 #define REP8_80 0x8080808080808080
 
+.weak strlen
 ENTRY(strlen)
 	mov	zeroones, #REP8_01
 	bic	src, srcin, #15
diff --git a/arch/arm64/lib/strncmp.S b/arch/arm64/lib/strncmp.S
index e267044761c6..28563ac1c19f 100644
--- a/arch/arm64/lib/strncmp.S
+++ b/arch/arm64/lib/strncmp.S
@@ -64,6 +64,7 @@ limit_wd	.req	x13
 mask		.req	x14
 endloop		.req	x15
 
+.weak strncmp
 ENTRY(strncmp)
 	cbz	limit, .Lret0
 	eor	tmp1, src1, src2
diff --git a/arch/arm64/lib/strnlen.S b/arch/arm64/lib/strnlen.S
index eae38da6e0bb..bdbfd41164f4 100644
--- a/arch/arm64/lib/strnlen.S
+++ b/arch/arm64/lib/strnlen.S
@@ -59,6 +59,7 @@ limit_wd	.req	x14
 #define REP8_7f 0x7f7f7f7f7f7f7f7f
 #define REP8_80 0x8080808080808080
 
+.weak strnlen
 ENTRY(strnlen)
 	cbz	limit, .Lhit_limit
 	mov	zeroones, #REP8_01
diff --git a/arch/arm64/lib/strrchr.S b/arch/arm64/lib/strrchr.S
index f8e2784d5752..31c77f605014 100644
--- a/arch/arm64/lib/strrchr.S
+++ b/arch/arm64/lib/strrchr.S
@@ -29,6 +29,7 @@
  * Returns:
  *	x0 - address of last occurrence of 'c' or 0
  */
+.weak strrchr
 ENTRY(strrchr)
 	mov	x3, #0
 	and	w1, w1, #0xff
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH] lib/test_kasan: Add tests for several string/memory API functions
  2018-09-06 17:05 [PATCH] arm64: lib: use C string functions with KASAN enabled Andrey Ryabinin
@ 2018-09-06 17:05 ` Andrey Ryabinin
  2018-09-07 14:56 ` [PATCH] arm64: lib: use C string functions with KASAN enabled Will Deacon
  1 sibling, 0 replies; 16+ messages in thread
From: Andrey Ryabinin @ 2018-09-06 17:05 UTC (permalink / raw)
  To: Will Deacon, Catalin Marinas
  Cc: Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Andrey Ryabinin

Arch code may have asm implementation of string/memory API functions
instead of using generic one from lib/string.c. KASAN don't see
memory accesses in asm code, thus can miss many bugs.

E.g. on ARM64 KASAN don't see bugs in memchr(), memcmp(), str[r]chr(),
str[n]cmp(), str[n]len(). Add tests for these functions to be sure
that we notice the problem on other architectures.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 lib/test_kasan.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index ec657105edbf..51b78405bf24 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -579,6 +579,73 @@ static noinline void __init kmem_cache_invalid_free(void)
 	kmem_cache_destroy(cache);
 }
 
+static noinline void __init kasan_memchr(void)
+{
+	char *ptr;
+	size_t size = 24;
+
+	pr_info("out-of-bounds in memchr\n");
+	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
+	if (!ptr)
+		return;
+
+	memchr(ptr, '1', size + 1);
+	kfree(ptr);
+}
+
+static noinline void __init kasan_memcmp(void)
+{
+	char *ptr;
+	size_t size = 24;
+	int arr[9];
+
+	pr_info("out-of-bounds in memcmp\n");
+	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
+	if (!ptr)
+		return;
+
+	memset(arr, 0, sizeof(arr));
+	memcmp(ptr, arr, size+1);
+	kfree(ptr);
+}
+
+static noinline void __init kasan_strings(void)
+{
+	char *ptr;
+	size_t size = 24;
+
+	pr_info("use-after-free in strchr\n");
+	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
+	if (!ptr)
+		return;
+
+	kfree(ptr);
+
+	/*
+	 * Try to cause only 1 invalid access (less spam in dmesg).
+	 * For that we need ptr to point to zeroed byte.
+	 * Skip metadata that could be stored in freed object so ptr
+	 * will likely point to zeroed byte.
+	 */
+	ptr += 16;
+	strchr(ptr, '1');
+
+	pr_info("use-after-free in strrchr\n");
+	strrchr(ptr, '1');
+
+	pr_info("use-after-free in strcmp\n");
+	strcmp(ptr, "2");
+
+	pr_info("use-after-free in strncmp\n");
+	strncmp(ptr, "2", 1);
+
+	pr_info("use-after-free in strlen\n");
+	strlen(ptr);
+
+	pr_info("use-after-free in strnlen\n");
+	strnlen(ptr, 1);
+}
+
 static int __init kmalloc_tests_init(void)
 {
 	/*
@@ -618,6 +685,9 @@ static int __init kmalloc_tests_init(void)
 	use_after_scope_test();
 	kmem_cache_double_free();
 	kmem_cache_invalid_free();
+	kasan_memchr();
+	kasan_memcmp();
+	kasan_strings();
 
 	kasan_restore_multi_shot(multishot);
 
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] arm64: lib: use C string functions with KASAN enabled.
  2018-09-06 17:05 [PATCH] arm64: lib: use C string functions with KASAN enabled Andrey Ryabinin
  2018-09-06 17:05 ` [PATCH] lib/test_kasan: Add tests for several string/memory API functions Andrey Ryabinin
@ 2018-09-07 14:56 ` Will Deacon
  2018-09-07 15:48   ` Andrey Ryabinin
  1 sibling, 1 reply; 16+ messages in thread
From: Will Deacon @ 2018-09-07 14:56 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Catalin Marinas, Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel

On Thu, Sep 06, 2018 at 08:05:33PM +0300, Andrey Ryabinin wrote:
> ARM64 has asm implementations of memchr(), memcmp(), str[r]chr(),
> str[n]cmp(), str[n]len(). KASAN don't see memory accesses in asm
> code, thus it can potentially miss many bugs.
> 
> Ifdef out __HAVE_ARCH_* defines of these functions when KASAN is
> enabled, so the generic implementations from lib/string.c will be used.
> 
> Declare asm functions as weak instead of removing them because they
> still can be used by efistub.

I don't understand this bit: efistub uses the __pi_ prefixed versions of the
routines, so why do we need to declare them as weak?

Will

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] arm64: lib: use C string functions with KASAN enabled.
  2018-09-07 14:56 ` [PATCH] arm64: lib: use C string functions with KASAN enabled Will Deacon
@ 2018-09-07 15:48   ` Andrey Ryabinin
  2018-09-10 11:33     ` Mark Rutland
  0 siblings, 1 reply; 16+ messages in thread
From: Andrey Ryabinin @ 2018-09-07 15:48 UTC (permalink / raw)
  To: Will Deacon
  Cc: Catalin Marinas, Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel



On 09/07/2018 05:56 PM, Will Deacon wrote:
> On Thu, Sep 06, 2018 at 08:05:33PM +0300, Andrey Ryabinin wrote:
>> ARM64 has asm implementations of memchr(), memcmp(), str[r]chr(),
>> str[n]cmp(), str[n]len(). KASAN don't see memory accesses in asm
>> code, thus it can potentially miss many bugs.
>>
>> Ifdef out __HAVE_ARCH_* defines of these functions when KASAN is
>> enabled, so the generic implementations from lib/string.c will be used.
>>
>> Declare asm functions as weak instead of removing them because they
>> still can be used by efistub.
> 
> I don't understand this bit: efistub uses the __pi_ prefixed versions of the
> routines, so why do we need to declare them as weak?

Weak needed because we can't have two non-weak functions with the same name.

Alternative approach would be to never use e.g. "strlen" name for asm implementation of strlen() under CONFIG_KASAN=y.
But that would require adding some special ENDPIPROC_KASAN() macro since we want __pi_strlen() to point
to the asm_strlen().

Using weak seems like a way better solution to me.

> 
> Will
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] arm64: lib: use C string functions with KASAN enabled.
  2018-09-07 15:48   ` Andrey Ryabinin
@ 2018-09-10 11:33     ` Mark Rutland
  2018-09-10 12:53       ` Mark Rutland
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Rutland @ 2018-09-10 11:33 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Will Deacon, Catalin Marinas, Andrew Morton, Kyeongdon Kim,
	Ard Biesheuvel, Alexander Potapenko, Dmitry Vyukov, kasan-dev,
	linux-arm-kernel, linux-kernel

On Fri, Sep 07, 2018 at 06:48:10PM +0300, Andrey Ryabinin wrote:
> On 09/07/2018 05:56 PM, Will Deacon wrote:
> > On Thu, Sep 06, 2018 at 08:05:33PM +0300, Andrey Ryabinin wrote:
> >> ARM64 has asm implementations of memchr(), memcmp(), str[r]chr(),
> >> str[n]cmp(), str[n]len(). KASAN don't see memory accesses in asm
> >> code, thus it can potentially miss many bugs.
> >>
> >> Ifdef out __HAVE_ARCH_* defines of these functions when KASAN is
> >> enabled, so the generic implementations from lib/string.c will be
> >> used.
> >>
> >> Declare asm functions as weak instead of removing them because they
> >> still can be used by efistub.
> > 
> > I don't understand this bit: efistub uses the __pi_ prefixed
> > versions of the routines, so why do we need to declare them as weak?
> 
> Weak needed because we can't have two non-weak functions with the same
> name.
> 
> Alternative approach would be to never use e.g. "strlen" name for asm
> implementation of strlen() under CONFIG_KASAN=y.  But that would
> require adding some special ENDPIPROC_KASAN() macro since we want
> __pi_strlen() to point to the asm_strlen().

Somehow, what we have today works with CONFIG_FORTIFY_SOURCE, which
AFAICT would suffer from texactly the same problem with things like
memcpy.

So either we're getting away with that by chance already (and should fix
that regardless of this patch), or this is not actually a problem.

Conditionally aliasing <foo> to pi_<foo> in a linker script (or header,
for functions which aren't special per the c spec) seems sane to me.

> Using weak seems like a way better solution to me.

I would strongly prefer fixing this without weak, even if we need a
ENDPRPROC_KASAN, and/or wrappers in some header file somewhere, since if
something goes wrong that will fail deterministically at build time
rather than silently falling back to the wrong piece of code.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] arm64: lib: use C string functions with KASAN enabled.
  2018-09-10 11:33     ` Mark Rutland
@ 2018-09-10 12:53       ` Mark Rutland
  2018-09-10 13:06         ` Will Deacon
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Rutland @ 2018-09-10 12:53 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Will Deacon, Catalin Marinas, Andrew Morton, Kyeongdon Kim,
	Ard Biesheuvel, Alexander Potapenko, Dmitry Vyukov, kasan-dev,
	linux-arm-kernel, linux-kernel

On Mon, Sep 10, 2018 at 12:33:22PM +0100, Mark Rutland wrote:
> On Fri, Sep 07, 2018 at 06:48:10PM +0300, Andrey Ryabinin wrote:
> > On 09/07/2018 05:56 PM, Will Deacon wrote:
> > > I don't understand this bit: efistub uses the __pi_ prefixed
> > > versions of the routines, so why do we need to declare them as weak?
> > 
> > Weak needed because we can't have two non-weak functions with the same
> > name.
> > 
> > Alternative approach would be to never use e.g. "strlen" name for asm
> > implementation of strlen() under CONFIG_KASAN=y.  But that would
> > require adding some special ENDPIPROC_KASAN() macro since we want
> > __pi_strlen() to point to the asm_strlen().
> 
> Somehow, what we have today works with CONFIG_FORTIFY_SOURCE, which
> AFAICT would suffer from texactly the same problem with things like
> memcpy.
> 
> So either we're getting away with that by chance already (and should fix
> that regardless of this patch), or this is not actually a problem.

I now see those functions are marked weak in the assembly
implementation; sorry for the noise.

Regardless, I still think it's preferable to avoid weak wherever
possible.

I have a couple of local patches to do that for KASAN, though it's not
clear to me how that should interact with FORTIFY_SOURCE.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] arm64: lib: use C string functions with KASAN enabled.
  2018-09-10 12:53       ` Mark Rutland
@ 2018-09-10 13:06         ` Will Deacon
  2018-09-11 13:01           ` Andrey Ryabinin
  0 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2018-09-10 13:06 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Andrey Ryabinin, Catalin Marinas, Andrew Morton, Kyeongdon Kim,
	Ard Biesheuvel, Alexander Potapenko, Dmitry Vyukov, kasan-dev,
	linux-arm-kernel, linux-kernel

On Mon, Sep 10, 2018 at 01:53:03PM +0100, Mark Rutland wrote:
> On Mon, Sep 10, 2018 at 12:33:22PM +0100, Mark Rutland wrote:
> > On Fri, Sep 07, 2018 at 06:48:10PM +0300, Andrey Ryabinin wrote:
> > > On 09/07/2018 05:56 PM, Will Deacon wrote:
> > > > I don't understand this bit: efistub uses the __pi_ prefixed
> > > > versions of the routines, so why do we need to declare them as weak?
> > > 
> > > Weak needed because we can't have two non-weak functions with the same
> > > name.
> > > 
> > > Alternative approach would be to never use e.g. "strlen" name for asm
> > > implementation of strlen() under CONFIG_KASAN=y.  But that would
> > > require adding some special ENDPIPROC_KASAN() macro since we want
> > > __pi_strlen() to point to the asm_strlen().
> > 
> > Somehow, what we have today works with CONFIG_FORTIFY_SOURCE, which
> > AFAICT would suffer from texactly the same problem with things like
> > memcpy.
> > 
> > So either we're getting away with that by chance already (and should fix
> > that regardless of this patch), or this is not actually a problem.
> 
> I now see those functions are marked weak in the assembly
> implementation; sorry for the noise.
> 
> Regardless, I still think it's preferable to avoid weak wherever
> possible.

I was thinking along the same lines, but having played around with the code,
I agree with Andrey that this appears to be the cleanest solution.

Andrey -- could you respin using WEAK instead of .weak, removing any
redundant uses of ENTRY in the process? We might also need to throw an
ALIGN directive into the WEAK definition.

Will

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] arm64: lib: use C string functions with KASAN enabled.
  2018-09-10 13:06         ` Will Deacon
@ 2018-09-11 13:01           ` Andrey Ryabinin
  2018-09-14 15:28             ` Will Deacon
  0 siblings, 1 reply; 16+ messages in thread
From: Andrey Ryabinin @ 2018-09-11 13:01 UTC (permalink / raw)
  To: Will Deacon, Mark Rutland
  Cc: Catalin Marinas, Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel



On 09/10/2018 04:06 PM, Will Deacon wrote:
> On Mon, Sep 10, 2018 at 01:53:03PM +0100, Mark Rutland wrote:
>> On Mon, Sep 10, 2018 at 12:33:22PM +0100, Mark Rutland wrote:
>>> On Fri, Sep 07, 2018 at 06:48:10PM +0300, Andrey Ryabinin wrote:
>>>> On 09/07/2018 05:56 PM, Will Deacon wrote:
>>>>> I don't understand this bit: efistub uses the __pi_ prefixed
>>>>> versions of the routines, so why do we need to declare them as weak?
>>>>
>>>> Weak needed because we can't have two non-weak functions with the same
>>>> name.
>>>>
>>>> Alternative approach would be to never use e.g. "strlen" name for asm
>>>> implementation of strlen() under CONFIG_KASAN=y.  But that would
>>>> require adding some special ENDPIPROC_KASAN() macro since we want
>>>> __pi_strlen() to point to the asm_strlen().
>>>
>>> Somehow, what we have today works with CONFIG_FORTIFY_SOURCE, which
>>> AFAICT would suffer from texactly the same problem with things like
>>> memcpy.
>>>

FORTIFY_SOURCE seems uses "extern inline" to redefine functions.
I obviously cannot make the whole lib/string.c 'extern inline'.


>>> So either we're getting away with that by chance already (and should fix
>>> that regardless of this patch), or this is not actually a problem.
>>
>> I now see those functions are marked weak in the assembly
>> implementation; sorry for the noise.
>>
>> Regardless, I still think it's preferable to avoid weak wherever
>> possible.
> 
> I was thinking along the same lines, but having played around with the code,
> I agree with Andrey that this appears to be the cleanest solution.
> 
> Andrey -- could you respin using WEAK instead of .weak, removing any
> redundant uses of ENTRY in the process? We might also need to throw an
> ALIGN directive into the WEAK definition.
> 

Actually I come up with something that looks decent, without using weak symbols, see below.
"#ifndef CONFIG_KASAN" could be moved to the header. In that ALIAS probably should be renamed to
something like NOKASAN_ALIAS().

---
 arch/arm64/include/asm/assembler.h |  7 +++++++
 arch/arm64/include/asm/string.h    | 14 ++++++++------
 arch/arm64/kernel/arm64ksyms.c     |  7 +++++--
 arch/arm64/lib/memchr.S            |  8 ++++++--
 arch/arm64/lib/memcmp.S            |  8 ++++++--
 arch/arm64/lib/strchr.S            |  8 ++++++--
 arch/arm64/lib/strcmp.S            |  8 ++++++--
 arch/arm64/lib/strlen.S            |  8 ++++++--
 arch/arm64/lib/strncmp.S           |  8 ++++++--
 arch/arm64/lib/strnlen.S           |  8 ++++++--
 arch/arm64/lib/strrchr.S           |  8 ++++++--
 11 files changed, 68 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 0bcc98dbba56..9779c6e03337 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -467,6 +467,13 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
 	.size	__pi_##x, . - x;	\
 	ENDPROC(x)
 
+#define ALIAS(x, y)			\
+	.globl	y;		\
+	.type 	y, %function;	\
+	.set	y, x;		\
+	.size	y, . - x;	\
+	ENDPROC(y)
+
 /*
  * Annotate a function as being unsuitable for kprobes.
  */
diff --git a/arch/arm64/include/asm/string.h b/arch/arm64/include/asm/string.h
index dd95d33a5bd5..8ddc7bd1f03e 100644
--- a/arch/arm64/include/asm/string.h
+++ b/arch/arm64/include/asm/string.h
@@ -16,6 +16,7 @@
 #ifndef __ASM_STRING_H
 #define __ASM_STRING_H
 
+#ifndef CONFIG_KASAN
 #define __HAVE_ARCH_STRRCHR
 extern char *strrchr(const char *, int c);
 
@@ -34,6 +35,13 @@ extern __kernel_size_t strlen(const char *);
 #define __HAVE_ARCH_STRNLEN
 extern __kernel_size_t strnlen(const char *, __kernel_size_t);
 
+#define __HAVE_ARCH_MEMCHR
+extern void *memchr(const void *, int, __kernel_size_t);
+
+#define __HAVE_ARCH_MEMCMP
+extern int memcmp(const void *, const void *, size_t);
+#endif
+
 #define __HAVE_ARCH_MEMCPY
 extern void *memcpy(void *, const void *, __kernel_size_t);
 extern void *__memcpy(void *, const void *, __kernel_size_t);
@@ -42,16 +50,10 @@ extern void *__memcpy(void *, const void *, __kernel_size_t);
 extern void *memmove(void *, const void *, __kernel_size_t);
 extern void *__memmove(void *, const void *, __kernel_size_t);
 
-#define __HAVE_ARCH_MEMCHR
-extern void *memchr(const void *, int, __kernel_size_t);
-
 #define __HAVE_ARCH_MEMSET
 extern void *memset(void *, int, __kernel_size_t);
 extern void *__memset(void *, int, __kernel_size_t);
 
-#define __HAVE_ARCH_MEMCMP
-extern int memcmp(const void *, const void *, size_t);
-
 #ifdef CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE
 #define __HAVE_ARCH_MEMCPY_FLUSHCACHE
 void memcpy_flushcache(void *dst, const void *src, size_t cnt);
diff --git a/arch/arm64/kernel/arm64ksyms.c b/arch/arm64/kernel/arm64ksyms.c
index d894a20b70b2..d72a32ea5335 100644
--- a/arch/arm64/kernel/arm64ksyms.c
+++ b/arch/arm64/kernel/arm64ksyms.c
@@ -43,6 +43,7 @@ EXPORT_SYMBOL(__arch_copy_in_user);
 	/* physical memory */
 EXPORT_SYMBOL(memstart_addr);
 
+#ifndef CONFIG_KASAN
 	/* string / mem functions */
 EXPORT_SYMBOL(strchr);
 EXPORT_SYMBOL(strrchr);
@@ -50,14 +51,16 @@ EXPORT_SYMBOL(strcmp);
 EXPORT_SYMBOL(strncmp);
 EXPORT_SYMBOL(strlen);
 EXPORT_SYMBOL(strnlen);
+EXPORT_SYMBOL(memchr);
+EXPORT_SYMBOL(memcmp);
+#endif
+
 EXPORT_SYMBOL(memset);
 EXPORT_SYMBOL(memcpy);
 EXPORT_SYMBOL(memmove);
 EXPORT_SYMBOL(__memset);
 EXPORT_SYMBOL(__memcpy);
 EXPORT_SYMBOL(__memmove);
-EXPORT_SYMBOL(memchr);
-EXPORT_SYMBOL(memcmp);
 
 	/* atomic bitops */
 EXPORT_SYMBOL(set_bit);
diff --git a/arch/arm64/lib/memchr.S b/arch/arm64/lib/memchr.S
index 4444c1d25f4b..a2f711baaaec 100644
--- a/arch/arm64/lib/memchr.S
+++ b/arch/arm64/lib/memchr.S
@@ -30,7 +30,7 @@
  * Returns:
  *	x0 - address of first occurrence of 'c' or 0
  */
-ENTRY(memchr)
+ENTRY(__pi_memchr)
 	and	w1, w1, #0xff
 1:	subs	x2, x2, #1
 	b.mi	2f
@@ -41,4 +41,8 @@ ENTRY(memchr)
 	ret
 2:	mov	x0, #0
 	ret
-ENDPIPROC(memchr)
+ENDPROC(__pi_memchr)
+
+#ifndef CONFIG_KASAN
+ALIAS(__pi_memchr, memchr)
+#endif
diff --git a/arch/arm64/lib/memcmp.S b/arch/arm64/lib/memcmp.S
index 2a4e239bd17a..d2d6b76d1a44 100644
--- a/arch/arm64/lib/memcmp.S
+++ b/arch/arm64/lib/memcmp.S
@@ -58,7 +58,7 @@ pos		.req	x11
 limit_wd	.req	x12
 mask		.req	x13
 
-ENTRY(memcmp)
+ENTRY(__pi_memcmp)
 	cbz	limit, .Lret0
 	eor	tmp1, src1, src2
 	tst	tmp1, #7
@@ -255,4 +255,8 @@ CPU_LE( rev	data2, data2 )
 .Lret0:
 	mov	result, #0
 	ret
-ENDPIPROC(memcmp)
+ENDPROC(__pi_memcmp)
+
+#ifndef CONFIG_KASAN
+ALIAS(__pi_memcmp, memcmp)
+#endif
diff --git a/arch/arm64/lib/strchr.S b/arch/arm64/lib/strchr.S
index dae0cf5591f9..5bcfcf66042e 100644
--- a/arch/arm64/lib/strchr.S
+++ b/arch/arm64/lib/strchr.S
@@ -29,7 +29,7 @@
  * Returns:
  *	x0 - address of first occurrence of 'c' or 0
  */
-ENTRY(strchr)
+ENTRY(__pi_strchr)
 	and	w1, w1, #0xff
 1:	ldrb	w2, [x0], #1
 	cmp	w2, w1
@@ -39,4 +39,8 @@ ENTRY(strchr)
 	cmp	w2, w1
 	csel	x0, x0, xzr, eq
 	ret
-ENDPROC(strchr)
+ENDPROC(__pi_strchr)
+
+#ifndef CONFIG_KASAN
+ALIAS(__pi_strchr, strchr)
+#endif
diff --git a/arch/arm64/lib/strcmp.S b/arch/arm64/lib/strcmp.S
index 471fe61760ef..e0dd23f36be9 100644
--- a/arch/arm64/lib/strcmp.S
+++ b/arch/arm64/lib/strcmp.S
@@ -60,7 +60,7 @@ tmp3		.req	x9
 zeroones	.req	x10
 pos		.req	x11
 
-ENTRY(strcmp)
+ENTRY(__pi_strcmp)
 	eor	tmp1, src1, src2
 	mov	zeroones, #REP8_01
 	tst	tmp1, #7
@@ -231,4 +231,8 @@ CPU_BE(	orr	syndrome, diff, has_nul )
 	lsr	data1, data1, #56
 	sub	result, data1, data2, lsr #56
 	ret
-ENDPIPROC(strcmp)
+ENDPROC(__pi_strcmp)
+
+#ifndef CONFIG_KASAN
+ALIAS(__pi_strcmp, strcmp)
+#endif
diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
index 55ccc8e24c08..f73e6a6c2fc0 100644
--- a/arch/arm64/lib/strlen.S
+++ b/arch/arm64/lib/strlen.S
@@ -56,7 +56,7 @@ pos		.req	x12
 #define REP8_7f 0x7f7f7f7f7f7f7f7f
 #define REP8_80 0x8080808080808080
 
-ENTRY(strlen)
+ENTRY(__pi_strlen)
 	mov	zeroones, #REP8_01
 	bic	src, srcin, #15
 	ands	tmp1, srcin, #15
@@ -123,4 +123,8 @@ CPU_LE( lsr	tmp2, tmp2, tmp1 )	/* Shift (tmp1 & 63).  */
 	csinv	data1, data1, xzr, le
 	csel	data2, data2, data2a, le
 	b	.Lrealigned
-ENDPIPROC(strlen)
+ENDPROC(__pi_strlen)
+
+#ifndef CONFIG_KASAN
+ALIAS(__pi_strlen, strlen)
+#endif
diff --git a/arch/arm64/lib/strncmp.S b/arch/arm64/lib/strncmp.S
index e267044761c6..640dc77d4a2c 100644
--- a/arch/arm64/lib/strncmp.S
+++ b/arch/arm64/lib/strncmp.S
@@ -64,7 +64,7 @@ limit_wd	.req	x13
 mask		.req	x14
 endloop		.req	x15
 
-ENTRY(strncmp)
+ENTRY(__pi_strncmp)
 	cbz	limit, .Lret0
 	eor	tmp1, src1, src2
 	mov	zeroones, #REP8_01
@@ -307,4 +307,8 @@ CPU_BE( orr	syndrome, diff, has_nul )
 .Lret0:
 	mov	result, #0
 	ret
-ENDPIPROC(strncmp)
+ENDPROC(__pi_strncmp)
+
+#ifndef CONFIG_KASAN
+ALIAS(__pi_strncmp, strncmp)
+#endif
diff --git a/arch/arm64/lib/strnlen.S b/arch/arm64/lib/strnlen.S
index eae38da6e0bb..c9749b807f84 100644
--- a/arch/arm64/lib/strnlen.S
+++ b/arch/arm64/lib/strnlen.S
@@ -59,7 +59,7 @@ limit_wd	.req	x14
 #define REP8_7f 0x7f7f7f7f7f7f7f7f
 #define REP8_80 0x8080808080808080
 
-ENTRY(strnlen)
+ENTRY(__pi_strnlen)
 	cbz	limit, .Lhit_limit
 	mov	zeroones, #REP8_01
 	bic	src, srcin, #15
@@ -168,4 +168,8 @@ CPU_LE( lsr	tmp2, tmp2, tmp4 )	/* Shift (tmp1 & 63).  */
 .Lhit_limit:
 	mov	len, limit
 	ret
-ENDPIPROC(strnlen)
+ENDPROC(__pi_strnlen)
+
+#ifndef CONFIG_KASAN
+ALIAS(__pi_strnlen, strnlen)
+#endif
diff --git a/arch/arm64/lib/strrchr.S b/arch/arm64/lib/strrchr.S
index f8e2784d5752..27bb369de8d9 100644
--- a/arch/arm64/lib/strrchr.S
+++ b/arch/arm64/lib/strrchr.S
@@ -29,7 +29,7 @@
  * Returns:
  *	x0 - address of last occurrence of 'c' or 0
  */
-ENTRY(strrchr)
+ENTRY(__pi_strrchr)
 	mov	x3, #0
 	and	w1, w1, #0xff
 1:	ldrb	w2, [x0], #1
@@ -40,4 +40,8 @@ ENTRY(strrchr)
 	b	1b
 2:	mov	x0, x3
 	ret
-ENDPIPROC(strrchr)
+ENDPROC(__pi_strrchr)
+
+#ifndef CONFIG_KASAN
+ALIAS(__pi_strrchr, strrchr)
+#endif
-- 
2.16.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] arm64: lib: use C string functions with KASAN enabled.
  2018-09-11 13:01           ` Andrey Ryabinin
@ 2018-09-14 15:28             ` Will Deacon
  2018-09-20 13:56               ` [PATCH v2 1/3] linkage.h: Align weak symbols Andrey Ryabinin
  0 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2018-09-14 15:28 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Mark Rutland, Catalin Marinas, Andrew Morton, Kyeongdon Kim,
	Ard Biesheuvel, Alexander Potapenko, Dmitry Vyukov, kasan-dev,
	linux-arm-kernel, linux-kernel

On Tue, Sep 11, 2018 at 04:01:28PM +0300, Andrey Ryabinin wrote:
> 
> 
> On 09/10/2018 04:06 PM, Will Deacon wrote:
> > On Mon, Sep 10, 2018 at 01:53:03PM +0100, Mark Rutland wrote:
> >> On Mon, Sep 10, 2018 at 12:33:22PM +0100, Mark Rutland wrote:
> >>> On Fri, Sep 07, 2018 at 06:48:10PM +0300, Andrey Ryabinin wrote:
> >>>> On 09/07/2018 05:56 PM, Will Deacon wrote:
> >>>>> I don't understand this bit: efistub uses the __pi_ prefixed
> >>>>> versions of the routines, so why do we need to declare them as weak?
> >>>>
> >>>> Weak needed because we can't have two non-weak functions with the same
> >>>> name.
> >>>>
> >>>> Alternative approach would be to never use e.g. "strlen" name for asm
> >>>> implementation of strlen() under CONFIG_KASAN=y.  But that would
> >>>> require adding some special ENDPIPROC_KASAN() macro since we want
> >>>> __pi_strlen() to point to the asm_strlen().
> >>>
> >>> Somehow, what we have today works with CONFIG_FORTIFY_SOURCE, which
> >>> AFAICT would suffer from texactly the same problem with things like
> >>> memcpy.
> >>>
> 
> FORTIFY_SOURCE seems uses "extern inline" to redefine functions.
> I obviously cannot make the whole lib/string.c 'extern inline'.
> 
> 
> >>> So either we're getting away with that by chance already (and should fix
> >>> that regardless of this patch), or this is not actually a problem.
> >>
> >> I now see those functions are marked weak in the assembly
> >> implementation; sorry for the noise.
> >>
> >> Regardless, I still think it's preferable to avoid weak wherever
> >> possible.
> > 
> > I was thinking along the same lines, but having played around with the code,
> > I agree with Andrey that this appears to be the cleanest solution.
> > 
> > Andrey -- could you respin using WEAK instead of .weak, removing any
> > redundant uses of ENTRY in the process? We might also need to throw an
> > ALIGN directive into the WEAK definition.
> > 
> 
> Actually I come up with something that looks decent, without using weak symbols, see below.
> "#ifndef CONFIG_KASAN" could be moved to the header. In that ALIAS probably should be renamed to
> something like NOKASAN_ALIAS().

Hmm, to be honest, I'd kinda got used to the version using weak symbols
and I reckon it'd be cleaner still if you respin it using WEAK.

Will

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/3] linkage.h: Align weak symbols.
  2018-09-14 15:28             ` Will Deacon
@ 2018-09-20 13:56               ` Andrey Ryabinin
  2018-09-20 13:56                 ` [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled Andrey Ryabinin
                                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Andrey Ryabinin @ 2018-09-20 13:56 UTC (permalink / raw)
  To: Will Deacon, Catalin Marinas
  Cc: Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Mark Rutland, Andrey Ryabinin

Since WEAK() supposed to be used instead of ENTRY() to define weak
symbols, but unlike ENTRY() it doesn't have ALIGN directive.
It seems there is no actual reason to not have, so let's add
ALIGN to WEAK() too.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 include/linux/linkage.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index d7618c41f74c..7c47b1a471d4 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -90,6 +90,7 @@
 #ifndef WEAK
 #define WEAK(name)	   \
 	.weak name ASM_NL   \
+	ALIGN ASM_NL \
 	name:
 #endif
 
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled.
  2018-09-20 13:56               ` [PATCH v2 1/3] linkage.h: Align weak symbols Andrey Ryabinin
@ 2018-09-20 13:56                 ` Andrey Ryabinin
  2018-10-29 10:29                   ` Will Deacon
  2018-09-20 13:56                 ` [PATCH v2 3/3] lib/test_kasan: Add tests for several string/memory API functions Andrey Ryabinin
  2018-10-29 10:29                 ` [PATCH v2 1/3] linkage.h: Align weak symbols Will Deacon
  2 siblings, 1 reply; 16+ messages in thread
From: Andrey Ryabinin @ 2018-09-20 13:56 UTC (permalink / raw)
  To: Will Deacon, Catalin Marinas
  Cc: Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Mark Rutland, Andrey Ryabinin

ARM64 has asm implementation of memchr(), memcmp(), str[r]chr(),
str[n]cmp(), str[n]len(). KASAN don't see memory accesses in asm
code, thus it can potentially miss many bugs.

Ifdef out __HAVE_ARCH_* defines of these functions when KASAN is
enabled, so the generic implementations from lib/string.c will be used.

We can't just remove the asm functions because efistub uses them.
And we can't have two non-weak functions either, so declare the asm
functions as weak.

Reported-by: Kyeongdon Kim <kyeongdon.kim@lge.com>
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
Changes since v1:
 - Use WEAK() instead of .weak

 arch/arm64/include/asm/string.h | 14 ++++++++------
 arch/arm64/kernel/arm64ksyms.c  |  7 +++++--
 arch/arm64/lib/memchr.S         |  2 +-
 arch/arm64/lib/memcmp.S         |  2 +-
 arch/arm64/lib/strchr.S         |  2 +-
 arch/arm64/lib/strcmp.S         |  2 +-
 arch/arm64/lib/strlen.S         |  2 +-
 arch/arm64/lib/strncmp.S        |  2 +-
 arch/arm64/lib/strnlen.S        |  2 +-
 arch/arm64/lib/strrchr.S        |  2 +-
 10 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/string.h b/arch/arm64/include/asm/string.h
index dd95d33a5bd5..03a6c256b7ec 100644
--- a/arch/arm64/include/asm/string.h
+++ b/arch/arm64/include/asm/string.h
@@ -16,6 +16,7 @@
 #ifndef __ASM_STRING_H
 #define __ASM_STRING_H
 
+#ifndef CONFIG_KASAN
 #define __HAVE_ARCH_STRRCHR
 extern char *strrchr(const char *, int c);
 
@@ -34,6 +35,13 @@ extern __kernel_size_t strlen(const char *);
 #define __HAVE_ARCH_STRNLEN
 extern __kernel_size_t strnlen(const char *, __kernel_size_t);
 
+#define __HAVE_ARCH_MEMCMP
+extern int memcmp(const void *, const void *, size_t);
+
+#define __HAVE_ARCH_MEMCHR
+extern void *memchr(const void *, int, __kernel_size_t);
+#endif
+
 #define __HAVE_ARCH_MEMCPY
 extern void *memcpy(void *, const void *, __kernel_size_t);
 extern void *__memcpy(void *, const void *, __kernel_size_t);
@@ -42,16 +50,10 @@ extern void *__memcpy(void *, const void *, __kernel_size_t);
 extern void *memmove(void *, const void *, __kernel_size_t);
 extern void *__memmove(void *, const void *, __kernel_size_t);
 
-#define __HAVE_ARCH_MEMCHR
-extern void *memchr(const void *, int, __kernel_size_t);
-
 #define __HAVE_ARCH_MEMSET
 extern void *memset(void *, int, __kernel_size_t);
 extern void *__memset(void *, int, __kernel_size_t);
 
-#define __HAVE_ARCH_MEMCMP
-extern int memcmp(const void *, const void *, size_t);
-
 #ifdef CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE
 #define __HAVE_ARCH_MEMCPY_FLUSHCACHE
 void memcpy_flushcache(void *dst, const void *src, size_t cnt);
diff --git a/arch/arm64/kernel/arm64ksyms.c b/arch/arm64/kernel/arm64ksyms.c
index d894a20b70b2..72f63a59b008 100644
--- a/arch/arm64/kernel/arm64ksyms.c
+++ b/arch/arm64/kernel/arm64ksyms.c
@@ -44,20 +44,23 @@ EXPORT_SYMBOL(__arch_copy_in_user);
 EXPORT_SYMBOL(memstart_addr);
 
 	/* string / mem functions */
+#ifndef CONFIG_KASAN
 EXPORT_SYMBOL(strchr);
 EXPORT_SYMBOL(strrchr);
 EXPORT_SYMBOL(strcmp);
 EXPORT_SYMBOL(strncmp);
 EXPORT_SYMBOL(strlen);
 EXPORT_SYMBOL(strnlen);
+EXPORT_SYMBOL(memcmp);
+EXPORT_SYMBOL(memchr);
+#endif
+
 EXPORT_SYMBOL(memset);
 EXPORT_SYMBOL(memcpy);
 EXPORT_SYMBOL(memmove);
 EXPORT_SYMBOL(__memset);
 EXPORT_SYMBOL(__memcpy);
 EXPORT_SYMBOL(__memmove);
-EXPORT_SYMBOL(memchr);
-EXPORT_SYMBOL(memcmp);
 
 	/* atomic bitops */
 EXPORT_SYMBOL(set_bit);
diff --git a/arch/arm64/lib/memchr.S b/arch/arm64/lib/memchr.S
index 4444c1d25f4b..0f164a4baf52 100644
--- a/arch/arm64/lib/memchr.S
+++ b/arch/arm64/lib/memchr.S
@@ -30,7 +30,7 @@
  * Returns:
  *	x0 - address of first occurrence of 'c' or 0
  */
-ENTRY(memchr)
+WEAK(memchr)
 	and	w1, w1, #0xff
 1:	subs	x2, x2, #1
 	b.mi	2f
diff --git a/arch/arm64/lib/memcmp.S b/arch/arm64/lib/memcmp.S
index 2a4e239bd17a..fb295f52e9f8 100644
--- a/arch/arm64/lib/memcmp.S
+++ b/arch/arm64/lib/memcmp.S
@@ -58,7 +58,7 @@ pos		.req	x11
 limit_wd	.req	x12
 mask		.req	x13
 
-ENTRY(memcmp)
+WEAK(memcmp)
 	cbz	limit, .Lret0
 	eor	tmp1, src1, src2
 	tst	tmp1, #7
diff --git a/arch/arm64/lib/strchr.S b/arch/arm64/lib/strchr.S
index dae0cf5591f9..7c83091d1bcd 100644
--- a/arch/arm64/lib/strchr.S
+++ b/arch/arm64/lib/strchr.S
@@ -29,7 +29,7 @@
  * Returns:
  *	x0 - address of first occurrence of 'c' or 0
  */
-ENTRY(strchr)
+WEAK(strchr)
 	and	w1, w1, #0xff
 1:	ldrb	w2, [x0], #1
 	cmp	w2, w1
diff --git a/arch/arm64/lib/strcmp.S b/arch/arm64/lib/strcmp.S
index 471fe61760ef..7d5d15398bfb 100644
--- a/arch/arm64/lib/strcmp.S
+++ b/arch/arm64/lib/strcmp.S
@@ -60,7 +60,7 @@ tmp3		.req	x9
 zeroones	.req	x10
 pos		.req	x11
 
-ENTRY(strcmp)
+WEAK(strcmp)
 	eor	tmp1, src1, src2
 	mov	zeroones, #REP8_01
 	tst	tmp1, #7
diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
index 55ccc8e24c08..8e0b14205dcb 100644
--- a/arch/arm64/lib/strlen.S
+++ b/arch/arm64/lib/strlen.S
@@ -56,7 +56,7 @@ pos		.req	x12
 #define REP8_7f 0x7f7f7f7f7f7f7f7f
 #define REP8_80 0x8080808080808080
 
-ENTRY(strlen)
+WEAK(strlen)
 	mov	zeroones, #REP8_01
 	bic	src, srcin, #15
 	ands	tmp1, srcin, #15
diff --git a/arch/arm64/lib/strncmp.S b/arch/arm64/lib/strncmp.S
index e267044761c6..66bd145935d9 100644
--- a/arch/arm64/lib/strncmp.S
+++ b/arch/arm64/lib/strncmp.S
@@ -64,7 +64,7 @@ limit_wd	.req	x13
 mask		.req	x14
 endloop		.req	x15
 
-ENTRY(strncmp)
+WEAK(strncmp)
 	cbz	limit, .Lret0
 	eor	tmp1, src1, src2
 	mov	zeroones, #REP8_01
diff --git a/arch/arm64/lib/strnlen.S b/arch/arm64/lib/strnlen.S
index eae38da6e0bb..355be04441fe 100644
--- a/arch/arm64/lib/strnlen.S
+++ b/arch/arm64/lib/strnlen.S
@@ -59,7 +59,7 @@ limit_wd	.req	x14
 #define REP8_7f 0x7f7f7f7f7f7f7f7f
 #define REP8_80 0x8080808080808080
 
-ENTRY(strnlen)
+WEAK(strnlen)
 	cbz	limit, .Lhit_limit
 	mov	zeroones, #REP8_01
 	bic	src, srcin, #15
diff --git a/arch/arm64/lib/strrchr.S b/arch/arm64/lib/strrchr.S
index f8e2784d5752..ea84924d5990 100644
--- a/arch/arm64/lib/strrchr.S
+++ b/arch/arm64/lib/strrchr.S
@@ -29,7 +29,7 @@
  * Returns:
  *	x0 - address of last occurrence of 'c' or 0
  */
-ENTRY(strrchr)
+WEAK(strrchr)
 	mov	x3, #0
 	and	w1, w1, #0xff
 1:	ldrb	w2, [x0], #1
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 3/3] lib/test_kasan: Add tests for several string/memory API functions
  2018-09-20 13:56               ` [PATCH v2 1/3] linkage.h: Align weak symbols Andrey Ryabinin
  2018-09-20 13:56                 ` [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled Andrey Ryabinin
@ 2018-09-20 13:56                 ` Andrey Ryabinin
  2018-10-29 10:29                 ` [PATCH v2 1/3] linkage.h: Align weak symbols Will Deacon
  2 siblings, 0 replies; 16+ messages in thread
From: Andrey Ryabinin @ 2018-09-20 13:56 UTC (permalink / raw)
  To: Will Deacon, Catalin Marinas
  Cc: Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Mark Rutland, Andrey Ryabinin

Arch code may have asm implementation of string/memory API functions
instead of using generic one from lib/string.c. KASAN don't see
memory accesses in asm code, thus can miss many bugs.

E.g. on ARM64 KASAN don't see bugs in memchr(), memcmp(), str[r]chr(),
str[n]cmp(), str[n]len(). Add tests for these functions to be sure
that we notice the problem on other architectures.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---

 No changes since v1.

 lib/test_kasan.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index ec657105edbf..51b78405bf24 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -579,6 +579,73 @@ static noinline void __init kmem_cache_invalid_free(void)
 	kmem_cache_destroy(cache);
 }
 
+static noinline void __init kasan_memchr(void)
+{
+	char *ptr;
+	size_t size = 24;
+
+	pr_info("out-of-bounds in memchr\n");
+	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
+	if (!ptr)
+		return;
+
+	memchr(ptr, '1', size + 1);
+	kfree(ptr);
+}
+
+static noinline void __init kasan_memcmp(void)
+{
+	char *ptr;
+	size_t size = 24;
+	int arr[9];
+
+	pr_info("out-of-bounds in memcmp\n");
+	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
+	if (!ptr)
+		return;
+
+	memset(arr, 0, sizeof(arr));
+	memcmp(ptr, arr, size+1);
+	kfree(ptr);
+}
+
+static noinline void __init kasan_strings(void)
+{
+	char *ptr;
+	size_t size = 24;
+
+	pr_info("use-after-free in strchr\n");
+	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
+	if (!ptr)
+		return;
+
+	kfree(ptr);
+
+	/*
+	 * Try to cause only 1 invalid access (less spam in dmesg).
+	 * For that we need ptr to point to zeroed byte.
+	 * Skip metadata that could be stored in freed object so ptr
+	 * will likely point to zeroed byte.
+	 */
+	ptr += 16;
+	strchr(ptr, '1');
+
+	pr_info("use-after-free in strrchr\n");
+	strrchr(ptr, '1');
+
+	pr_info("use-after-free in strcmp\n");
+	strcmp(ptr, "2");
+
+	pr_info("use-after-free in strncmp\n");
+	strncmp(ptr, "2", 1);
+
+	pr_info("use-after-free in strlen\n");
+	strlen(ptr);
+
+	pr_info("use-after-free in strnlen\n");
+	strnlen(ptr, 1);
+}
+
 static int __init kmalloc_tests_init(void)
 {
 	/*
@@ -618,6 +685,9 @@ static int __init kmalloc_tests_init(void)
 	use_after_scope_test();
 	kmem_cache_double_free();
 	kmem_cache_invalid_free();
+	kasan_memchr();
+	kasan_memcmp();
+	kasan_strings();
 
 	kasan_restore_multi_shot(multishot);
 
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/3] linkage.h: Align weak symbols.
  2018-09-20 13:56               ` [PATCH v2 1/3] linkage.h: Align weak symbols Andrey Ryabinin
  2018-09-20 13:56                 ` [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled Andrey Ryabinin
  2018-09-20 13:56                 ` [PATCH v2 3/3] lib/test_kasan: Add tests for several string/memory API functions Andrey Ryabinin
@ 2018-10-29 10:29                 ` Will Deacon
  2 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2018-10-29 10:29 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Catalin Marinas, Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Mark Rutland

On Thu, Sep 20, 2018 at 04:56:29PM +0300, Andrey Ryabinin wrote:
> Since WEAK() supposed to be used instead of ENTRY() to define weak
> symbols, but unlike ENTRY() it doesn't have ALIGN directive.
> It seems there is no actual reason to not have, so let's add
> ALIGN to WEAK() too.
> 
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
>  include/linux/linkage.h | 1 +
>  1 file changed, 1 insertion(+)

Looks sensible to me:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

> diff --git a/include/linux/linkage.h b/include/linux/linkage.h
> index d7618c41f74c..7c47b1a471d4 100644
> --- a/include/linux/linkage.h
> +++ b/include/linux/linkage.h
> @@ -90,6 +90,7 @@
>  #ifndef WEAK
>  #define WEAK(name)	   \
>  	.weak name ASM_NL   \
> +	ALIGN ASM_NL \
>  	name:
>  #endif
>  
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled.
  2018-09-20 13:56                 ` [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled Andrey Ryabinin
@ 2018-10-29 10:29                   ` Will Deacon
  2018-10-29 11:16                     ` Andrey Ryabinin
  0 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2018-10-29 10:29 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Catalin Marinas, Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Mark Rutland

On Thu, Sep 20, 2018 at 04:56:30PM +0300, Andrey Ryabinin wrote:
> ARM64 has asm implementation of memchr(), memcmp(), str[r]chr(),
> str[n]cmp(), str[n]len(). KASAN don't see memory accesses in asm
> code, thus it can potentially miss many bugs.
> 
> Ifdef out __HAVE_ARCH_* defines of these functions when KASAN is
> enabled, so the generic implementations from lib/string.c will be used.
> 
> We can't just remove the asm functions because efistub uses them.
> And we can't have two non-weak functions either, so declare the asm
> functions as weak.
> 
> Reported-by: Kyeongdon Kim <kyeongdon.kim@lge.com>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
> Changes since v1:
>  - Use WEAK() instead of .weak
> 
>  arch/arm64/include/asm/string.h | 14 ++++++++------
>  arch/arm64/kernel/arm64ksyms.c  |  7 +++++--
>  arch/arm64/lib/memchr.S         |  2 +-
>  arch/arm64/lib/memcmp.S         |  2 +-
>  arch/arm64/lib/strchr.S         |  2 +-
>  arch/arm64/lib/strcmp.S         |  2 +-
>  arch/arm64/lib/strlen.S         |  2 +-
>  arch/arm64/lib/strncmp.S        |  2 +-
>  arch/arm64/lib/strnlen.S        |  2 +-
>  arch/arm64/lib/strrchr.S        |  2 +-
>  10 files changed, 21 insertions(+), 16 deletions(-)

Acked-by: Will Deacon <will.deacon@arm.com>

Please post these again after the merge window and we'll figure out how to
get them queued.

Will

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled.
  2018-10-29 10:29                   ` Will Deacon
@ 2018-10-29 11:16                     ` Andrey Ryabinin
  2018-10-29 11:20                       ` Will Deacon
  0 siblings, 1 reply; 16+ messages in thread
From: Andrey Ryabinin @ 2018-10-29 11:16 UTC (permalink / raw)
  To: Will Deacon
  Cc: Catalin Marinas, Andrew Morton, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Mark Rutland



On 10/29/2018 01:29 PM, Will Deacon wrote:
> On Thu, Sep 20, 2018 at 04:56:30PM +0300, Andrey Ryabinin wrote:
>> ARM64 has asm implementation of memchr(), memcmp(), str[r]chr(),
>> str[n]cmp(), str[n]len(). KASAN don't see memory accesses in asm
>> code, thus it can potentially miss many bugs.
>>
>> Ifdef out __HAVE_ARCH_* defines of these functions when KASAN is
>> enabled, so the generic implementations from lib/string.c will be used.
>>
>> We can't just remove the asm functions because efistub uses them.
>> And we can't have two non-weak functions either, so declare the asm
>> functions as weak.
>>
>> Reported-by: Kyeongdon Kim <kyeongdon.kim@lge.com>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>> Changes since v1:
>>  - Use WEAK() instead of .weak
>>
>>  arch/arm64/include/asm/string.h | 14 ++++++++------
>>  arch/arm64/kernel/arm64ksyms.c  |  7 +++++--
>>  arch/arm64/lib/memchr.S         |  2 +-
>>  arch/arm64/lib/memcmp.S         |  2 +-
>>  arch/arm64/lib/strchr.S         |  2 +-
>>  arch/arm64/lib/strcmp.S         |  2 +-
>>  arch/arm64/lib/strlen.S         |  2 +-
>>  arch/arm64/lib/strncmp.S        |  2 +-
>>  arch/arm64/lib/strnlen.S        |  2 +-
>>  arch/arm64/lib/strrchr.S        |  2 +-
>>  10 files changed, 21 insertions(+), 16 deletions(-)
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> 
> Please post these again after the merge window and we'll figure out how to
> get them queued.
> 


Andrew sent these patches to Linus couple days ago, so they are in tree already.

Something went wrong with mail notification though. I didn't even realize that they
were in -mm tree, because I didn't receive the usual 'the patch has been added to -mm tree' email.
But I did receive email that was sent to Linus.

Also there was no you or Catalin in Cc tags in 2,3 patches, and in the first patch, the Cc tags were
corrupted:

From: Andrey Ryabinin <aryabinin@virtuozzo.com>
Subject: include/linux/linkage.h: align weak symbols

Since WEAK() supposed to be used instead of ENTRY() to define weak
symbols, but unlike ENTRY() it doesn't have ALIGN directive.  It seems
there is no actual reason to not have, so let's add ALIGN to WEAK() too.

Link: http://lkml.kernel.org/r/20180920135631.23833-1-aryabinin@virtuozzo.com
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Will Deacon <will.deacon@arm.com>, Catalin Marinas <catalin.marinas@arm.com>
Cc: Kyeongdon Kim <kyeongdon.kim@lge.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled.
  2018-10-29 11:16                     ` Andrey Ryabinin
@ 2018-10-29 11:20                       ` Will Deacon
  0 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2018-10-29 11:20 UTC (permalink / raw)
  To: Andrey Ryabinin, akpm
  Cc: Catalin Marinas, Kyeongdon Kim, Ard Biesheuvel,
	Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-arm-kernel,
	linux-kernel, Mark Rutland

Hi Andrey, Andrew,

On Mon, Oct 29, 2018 at 11:16:15AM +0000, Andrey Ryabinin wrote:
> On 10/29/2018 01:29 PM, Will Deacon wrote:
> > On Thu, Sep 20, 2018 at 04:56:30PM +0300, Andrey Ryabinin wrote:
> >> ARM64 has asm implementation of memchr(), memcmp(), str[r]chr(),
> >> str[n]cmp(), str[n]len(). KASAN don't see memory accesses in asm
> >> code, thus it can potentially miss many bugs.
> >>
> >> Ifdef out __HAVE_ARCH_* defines of these functions when KASAN is
> >> enabled, so the generic implementations from lib/string.c will be used.
> >>
> >> We can't just remove the asm functions because efistub uses them.
> >> And we can't have two non-weak functions either, so declare the asm
> >> functions as weak.
> >>
> >> Reported-by: Kyeongdon Kim <kyeongdon.kim@lge.com>
> >> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> >> ---
> >> Changes since v1:
> >>  - Use WEAK() instead of .weak
> >>
> >>  arch/arm64/include/asm/string.h | 14 ++++++++------
> >>  arch/arm64/kernel/arm64ksyms.c  |  7 +++++--
> >>  arch/arm64/lib/memchr.S         |  2 +-
> >>  arch/arm64/lib/memcmp.S         |  2 +-
> >>  arch/arm64/lib/strchr.S         |  2 +-
> >>  arch/arm64/lib/strcmp.S         |  2 +-
> >>  arch/arm64/lib/strlen.S         |  2 +-
> >>  arch/arm64/lib/strncmp.S        |  2 +-
> >>  arch/arm64/lib/strnlen.S        |  2 +-
> >>  arch/arm64/lib/strrchr.S        |  2 +-
> >>  10 files changed, 21 insertions(+), 16 deletions(-)
> > 
> > Acked-by: Will Deacon <will.deacon@arm.com>
> > 
> > Please post these again after the merge window and we'll figure out how to
> > get them queued.
> 
> Andrew sent these patches to Linus couple days ago, so they are in tree
> already.

Oh, good thing I was happy with them in the end, then!

> Something went wrong with mail notification though. I didn't even realize
> that they were in -mm tree, because I didn't receive the usual 'the patch
> has been added to -mm tree' email.  But I did receive email that was sent
> to Linus.

Yeah, strange. I usually see the notifications from Andrew.

> Also there was no you or Catalin in Cc tags in 2,3 patches, and in the
> first patch, the Cc tags were corrupted:

:/

Andrew -- have we broken your scripts somehow, or is this just a one-off
for these patches?

Thanks,

Will

> From: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Subject: include/linux/linkage.h: align weak symbols
> 
> Since WEAK() supposed to be used instead of ENTRY() to define weak
> symbols, but unlike ENTRY() it doesn't have ALIGN directive.  It seems
> there is no actual reason to not have, so let's add ALIGN to WEAK() too.
> 
> Link: http://lkml.kernel.org/r/20180920135631.23833-1-aryabinin@virtuozzo.com
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Will Deacon <will.deacon@arm.com>, Catalin Marinas <catalin.marinas@arm.com>
> Cc: Kyeongdon Kim <kyeongdon.kim@lge.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-10-29 11:30 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-06 17:05 [PATCH] arm64: lib: use C string functions with KASAN enabled Andrey Ryabinin
2018-09-06 17:05 ` [PATCH] lib/test_kasan: Add tests for several string/memory API functions Andrey Ryabinin
2018-09-07 14:56 ` [PATCH] arm64: lib: use C string functions with KASAN enabled Will Deacon
2018-09-07 15:48   ` Andrey Ryabinin
2018-09-10 11:33     ` Mark Rutland
2018-09-10 12:53       ` Mark Rutland
2018-09-10 13:06         ` Will Deacon
2018-09-11 13:01           ` Andrey Ryabinin
2018-09-14 15:28             ` Will Deacon
2018-09-20 13:56               ` [PATCH v2 1/3] linkage.h: Align weak symbols Andrey Ryabinin
2018-09-20 13:56                 ` [PATCH v2 2/3] arm64: lib: use C string functions with KASAN enabled Andrey Ryabinin
2018-10-29 10:29                   ` Will Deacon
2018-10-29 11:16                     ` Andrey Ryabinin
2018-10-29 11:20                       ` Will Deacon
2018-09-20 13:56                 ` [PATCH v2 3/3] lib/test_kasan: Add tests for several string/memory API functions Andrey Ryabinin
2018-10-29 10:29                 ` [PATCH v2 1/3] linkage.h: Align weak symbols Will Deacon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).