linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] kasan: test: use underlying string helpers
@ 2021-10-13 15:00 Arnd Bergmann
  2021-10-13 15:00 ` [PATCH 2/2] kasan: use fortified strings for hwaddress sanitizer Arnd Bergmann
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Arnd Bergmann @ 2021-10-13 15:00 UTC (permalink / raw)
  To: linux-hardening, Kees Cook, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, kasan-dev
  Cc: Arnd Bergmann, Andrew Morton, Marco Elver, Catalin Marinas,
	Peter Collingbourne, Patricia Alfonso, Vincenzo Frascino,
	linux-kernel

From: Arnd Bergmann <arnd@arndb.de>

Calling memcmp() and memchr() with an intentional buffer overflow
is now caught at compile time:

In function 'memcmp',
    inlined from 'kasan_memcmp' at lib/test_kasan.c:897:2:
include/linux/fortify-string.h:263:25: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
  263 |                         __read_overflow();
      |                         ^~~~~~~~~~~~~~~~~
In function 'memchr',
    inlined from 'kasan_memchr' at lib/test_kasan.c:872:2:
include/linux/fortify-string.h:277:17: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
  277 |                 __read_overflow();
      |                 ^~~~~~~~~~~~~~~~~

Change the kasan tests to wrap those inside of a noinline function
to prevent the compiler from noticing the bug and let kasan find
it at runtime.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 lib/test_kasan.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 67ed689a0b1b..903215e944f1 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -852,6 +852,21 @@ static void kmem_cache_invalid_free(struct kunit *test)
 	kmem_cache_destroy(cache);
 }
 
+/*
+ * noinline wrappers to prevent the compiler from noticing the overflow
+ * at compile time rather than having kasan catch it.
+ * */
+static noinline void *__kasan_memchr(const void *s, int c, size_t n)
+{
+	return memchr(s, c, n);
+}
+
+static noinline int __kasan_memcmp(const void *s1, const void *s2, size_t n)
+{
+	return memcmp(s1, s2, n);
+}
+
+
 static void kasan_memchr(struct kunit *test)
 {
 	char *ptr;
@@ -870,7 +885,7 @@ static void kasan_memchr(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
 
 	KUNIT_EXPECT_KASAN_FAIL(test,
-		kasan_ptr_result = memchr(ptr, '1', size + 1));
+		kasan_ptr_result = __kasan_memchr(ptr, '1', size + 1));
 
 	kfree(ptr);
 }
@@ -895,7 +910,7 @@ static void kasan_memcmp(struct kunit *test)
 	memset(arr, 0, sizeof(arr));
 
 	KUNIT_EXPECT_KASAN_FAIL(test,
-		kasan_int_result = memcmp(ptr, arr, size+1));
+		kasan_int_result = __kasan_memcmp(ptr, arr, size+1));
 	kfree(ptr);
 }
 
-- 
2.29.2


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

* [PATCH 2/2] kasan: use fortified strings for hwaddress sanitizer
  2021-10-13 15:00 [PATCH 1/2] kasan: test: use underlying string helpers Arnd Bergmann
@ 2021-10-13 15:00 ` Arnd Bergmann
  2021-10-18 19:57   ` Kees Cook
  2021-10-14  8:12 ` [PATCH 1/2] kasan: test: use underlying string helpers Vincenzo Frascino
  2021-10-18 19:47 ` Kees Cook
  2 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2021-10-13 15:00 UTC (permalink / raw)
  To: linux-hardening, Kees Cook, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, kasan-dev
  Cc: Arnd Bergmann, Nathan Chancellor, Nick Desaulniers, Kees Cook,
	Miguel Ojeda, Sami Tolvanen, Marco Elver, Masahiro Yamada,
	Ard Biesheuvel, linux-kernel, llvm

From: Arnd Bergmann <arnd@arndb.de>

GCC has separate macros for -fsanitize=kernel-address and
-fsanitize=kernel-hwaddress, and the check in the arm64 string.h
gets this wrong, which leads to string functions not getting
fortified with gcc. The newly added tests find this:

warning: unsafe memchr() usage lacked '__read_overflow' warning in /git/arm-soc/lib/test_fortify/read_overflow-memchr.c
warning: unsafe memchr_inv() usage lacked '__read_overflow' symbol in /git/arm-soc/lib/test_fortify/read_overflow-memchr_inv.c
warning: unsafe memcmp() usage lacked '__read_overflow' warning in /git/arm-soc/lib/test_fortify/read_overflow-memcmp.c
warning: unsafe memscan() usage lacked '__read_overflow' symbol in /git/arm-soc/lib/test_fortify/read_overflow-memscan.c
warning: unsafe memcmp() usage lacked '__read_overflow2' warning in /git/arm-soc/lib/test_fortify/read_overflow2-memcmp.c
warning: unsafe memcpy() usage lacked '__read_overflow2' symbol in /git/arm-soc/lib/test_fortify/read_overflow2-memcpy.c
warning: unsafe memmove() usage lacked '__read_overflow2' symbol in /git/arm-soc/lib/test_fortify/read_overflow2-memmove.c
warning: unsafe memcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memcpy.c
warning: unsafe memmove() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memmove.c
warning: unsafe memset() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memset.c
warning: unsafe strcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strcpy-lit.c
warning: unsafe strcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strcpy.c
warning: unsafe strlcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strlcpy-src.c
warning: unsafe strlcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strlcpy.c
warning: unsafe strncpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strncpy-src.c
warning: unsafe strncpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strncpy.c
warning: unsafe strscpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strscpy.c

Add a workaround to include/linux/compiler_types.h so we always
define __SANITIZE_ADDRESS__ for either mode, as we already do
for clang.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/compiler_types.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index aad6f6408bfa..2f2776fffefe 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -178,6 +178,13 @@ struct ftrace_likely_data {
  */
 #define noinline_for_stack noinline
 
+/*
+ * Treat __SANITIZE_HWADDRESS__ the same as __SANITIZE_ADDRESS__ in the kernel
+ */
+#ifdef __SANITIZE_HWADDRESS__
+#define __SANITIZE_ADDRESS__
+#endif
+
 /*
  * Sanitizer helper attributes: Because using __always_inline and
  * __no_sanitize_* conflict, provide helper attributes that will either expand
-- 
2.29.2


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

* Re: [PATCH 1/2] kasan: test: use underlying string helpers
  2021-10-13 15:00 [PATCH 1/2] kasan: test: use underlying string helpers Arnd Bergmann
  2021-10-13 15:00 ` [PATCH 2/2] kasan: use fortified strings for hwaddress sanitizer Arnd Bergmann
@ 2021-10-14  8:12 ` Vincenzo Frascino
  2021-10-15  2:40   ` Kees Cook
  2021-10-18 19:47 ` Kees Cook
  2 siblings, 1 reply; 10+ messages in thread
From: Vincenzo Frascino @ 2021-10-14  8:12 UTC (permalink / raw)
  To: Arnd Bergmann, linux-hardening, Kees Cook, Andrey Ryabinin,
	Alexander Potapenko, Andrey Konovalov, Dmitry Vyukov, kasan-dev
  Cc: Arnd Bergmann, Andrew Morton, Marco Elver, Catalin Marinas,
	Peter Collingbourne, Patricia Alfonso, linux-kernel



On 10/13/21 5:00 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Calling memcmp() and memchr() with an intentional buffer overflow
> is now caught at compile time:
> 
> In function 'memcmp',
>     inlined from 'kasan_memcmp' at lib/test_kasan.c:897:2:
> include/linux/fortify-string.h:263:25: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
>   263 |                         __read_overflow();
>       |                         ^~~~~~~~~~~~~~~~~
> In function 'memchr',
>     inlined from 'kasan_memchr' at lib/test_kasan.c:872:2:
> include/linux/fortify-string.h:277:17: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
>   277 |                 __read_overflow();
>       |                 ^~~~~~~~~~~~~~~~~
> 
> Change the kasan tests to wrap those inside of a noinline function
> to prevent the compiler from noticing the bug and let kasan find
> it at runtime.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

> ---
>  lib/test_kasan.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 67ed689a0b1b..903215e944f1 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -852,6 +852,21 @@ static void kmem_cache_invalid_free(struct kunit *test)
>  	kmem_cache_destroy(cache);
>  }
>  
> +/*
> + * noinline wrappers to prevent the compiler from noticing the overflow
> + * at compile time rather than having kasan catch it.
> + * */
> +static noinline void *__kasan_memchr(const void *s, int c, size_t n)
> +{
> +	return memchr(s, c, n);
> +}
> +
> +static noinline int __kasan_memcmp(const void *s1, const void *s2, size_t n)
> +{
> +	return memcmp(s1, s2, n);
> +}
> +
> +
>  static void kasan_memchr(struct kunit *test)
>  {
>  	char *ptr;
> @@ -870,7 +885,7 @@ static void kasan_memchr(struct kunit *test)
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>  
>  	KUNIT_EXPECT_KASAN_FAIL(test,
> -		kasan_ptr_result = memchr(ptr, '1', size + 1));
> +		kasan_ptr_result = __kasan_memchr(ptr, '1', size + 1));
>  
>  	kfree(ptr);
>  }
> @@ -895,7 +910,7 @@ static void kasan_memcmp(struct kunit *test)
>  	memset(arr, 0, sizeof(arr));
>  
>  	KUNIT_EXPECT_KASAN_FAIL(test,
> -		kasan_int_result = memcmp(ptr, arr, size+1));
> +		kasan_int_result = __kasan_memcmp(ptr, arr, size+1));
>  	kfree(ptr);
>  }
>  
> 

-- 
Regards,
Vincenzo

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

* Re: [PATCH 1/2] kasan: test: use underlying string helpers
  2021-10-14  8:12 ` [PATCH 1/2] kasan: test: use underlying string helpers Vincenzo Frascino
@ 2021-10-15  2:40   ` Kees Cook
  2021-10-28 20:15     ` Andrew Morton
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-10-15  2:40 UTC (permalink / raw)
  To: Vincenzo Frascino, Arnd Bergmann, linux-hardening, Kees Cook,
	Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
	Dmitry Vyukov, kasan-dev
  Cc: Arnd Bergmann, Andrew Morton, Marco Elver, Catalin Marinas,
	Peter Collingbourne, Patricia Alfonso, linux-kernel



On October 14, 2021 1:12:54 AM PDT, Vincenzo Frascino <vincenzo.frascino@arm.com> wrote:
>
>
>On 10/13/21 5:00 PM, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> Calling memcmp() and memchr() with an intentional buffer overflow
>> is now caught at compile time:
>> 
>> In function 'memcmp',
>>     inlined from 'kasan_memcmp' at lib/test_kasan.c:897:2:
>> include/linux/fortify-string.h:263:25: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
>>   263 |                         __read_overflow();
>>       |                         ^~~~~~~~~~~~~~~~~
>> In function 'memchr',
>>     inlined from 'kasan_memchr' at lib/test_kasan.c:872:2:
>> include/linux/fortify-string.h:277:17: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
>>   277 |                 __read_overflow();
>>       |                 ^~~~~~~~~~~~~~~~~
>> 
>> Change the kasan tests to wrap those inside of a noinline function
>> to prevent the compiler from noticing the bug and let kasan find
>> it at runtime.
>> 
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
>Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

How about just explicitly making the size invisible to the compiler?

I did this for similar issues in the same source:

https://lore.kernel.org/linux-hardening/20211006181544.1670992-1-keescook@chromium.org/T/#u


-Kees

>
>> ---
>>  lib/test_kasan.c | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>> 
>> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
>> index 67ed689a0b1b..903215e944f1 100644
>> --- a/lib/test_kasan.c
>> +++ b/lib/test_kasan.c
>> @@ -852,6 +852,21 @@ static void kmem_cache_invalid_free(struct kunit *test)
>>  	kmem_cache_destroy(cache);
>>  }
>>  
>> +/*
>> + * noinline wrappers to prevent the compiler from noticing the overflow
>> + * at compile time rather than having kasan catch it.
>> + * */
>> +static noinline void *__kasan_memchr(const void *s, int c, size_t n)
>> +{
>> +	return memchr(s, c, n);
>> +}
>> +
>> +static noinline int __kasan_memcmp(const void *s1, const void *s2, size_t n)
>> +{
>> +	return memcmp(s1, s2, n);
>> +}
>> +
>> +
>>  static void kasan_memchr(struct kunit *test)
>>  {
>>  	char *ptr;
>> @@ -870,7 +885,7 @@ static void kasan_memchr(struct kunit *test)
>>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>>  
>>  	KUNIT_EXPECT_KASAN_FAIL(test,
>> -		kasan_ptr_result = memchr(ptr, '1', size + 1));
>> +		kasan_ptr_result = __kasan_memchr(ptr, '1', size + 1));
>>  
>>  	kfree(ptr);
>>  }
>> @@ -895,7 +910,7 @@ static void kasan_memcmp(struct kunit *test)
>>  	memset(arr, 0, sizeof(arr));
>>  
>>  	KUNIT_EXPECT_KASAN_FAIL(test,
>> -		kasan_int_result = memcmp(ptr, arr, size+1));
>> +		kasan_int_result = __kasan_memcmp(ptr, arr, size+1));
>>  	kfree(ptr);
>>  }
>>  
>> 
>

-- 
Kees Cook

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

* Re: [PATCH 1/2] kasan: test: use underlying string helpers
  2021-10-13 15:00 [PATCH 1/2] kasan: test: use underlying string helpers Arnd Bergmann
  2021-10-13 15:00 ` [PATCH 2/2] kasan: use fortified strings for hwaddress sanitizer Arnd Bergmann
  2021-10-14  8:12 ` [PATCH 1/2] kasan: test: use underlying string helpers Vincenzo Frascino
@ 2021-10-18 19:47 ` Kees Cook
  2021-10-18 19:55   ` Arnd Bergmann
  2 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-10-18 19:47 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-hardening, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, kasan-dev, Arnd Bergmann,
	Andrew Morton, Marco Elver, Catalin Marinas, Peter Collingbourne,
	Patricia Alfonso, Vincenzo Frascino, linux-kernel

On Wed, Oct 13, 2021 at 05:00:05PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Calling memcmp() and memchr() with an intentional buffer overflow
> is now caught at compile time:
> 
> In function 'memcmp',
>     inlined from 'kasan_memcmp' at lib/test_kasan.c:897:2:
> include/linux/fortify-string.h:263:25: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
>   263 |                         __read_overflow();
>       |                         ^~~~~~~~~~~~~~~~~
> In function 'memchr',
>     inlined from 'kasan_memchr' at lib/test_kasan.c:872:2:
> include/linux/fortify-string.h:277:17: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
>   277 |                 __read_overflow();
>       |                 ^~~~~~~~~~~~~~~~~
> 
> Change the kasan tests to wrap those inside of a noinline function
> to prevent the compiler from noticing the bug and let kasan find
> it at runtime.

Is this with W=1 ? I had explicitly disabled the read overflows for
"phase 1" of the overflow restriction tightening...

(And what do you think of using OPTIMIZER_HIDE_VAR() instead[1]?

-Kees

[1] https://lore.kernel.org/linux-hardening/20211006181544.1670992-1-keescook@chromium.org/T/#u

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  lib/test_kasan.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 67ed689a0b1b..903215e944f1 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -852,6 +852,21 @@ static void kmem_cache_invalid_free(struct kunit *test)
>  	kmem_cache_destroy(cache);
>  }
>  
> +/*
> + * noinline wrappers to prevent the compiler from noticing the overflow
> + * at compile time rather than having kasan catch it.
> + * */
> +static noinline void *__kasan_memchr(const void *s, int c, size_t n)
> +{
> +	return memchr(s, c, n);
> +}
> +
> +static noinline int __kasan_memcmp(const void *s1, const void *s2, size_t n)
> +{
> +	return memcmp(s1, s2, n);
> +}
> +
> +
>  static void kasan_memchr(struct kunit *test)
>  {
>  	char *ptr;
> @@ -870,7 +885,7 @@ static void kasan_memchr(struct kunit *test)
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>  
>  	KUNIT_EXPECT_KASAN_FAIL(test,
> -		kasan_ptr_result = memchr(ptr, '1', size + 1));
> +		kasan_ptr_result = __kasan_memchr(ptr, '1', size + 1));
>  
>  	kfree(ptr);
>  }
> @@ -895,7 +910,7 @@ static void kasan_memcmp(struct kunit *test)
>  	memset(arr, 0, sizeof(arr));
>  
>  	KUNIT_EXPECT_KASAN_FAIL(test,
> -		kasan_int_result = memcmp(ptr, arr, size+1));
> +		kasan_int_result = __kasan_memcmp(ptr, arr, size+1));
>  	kfree(ptr);
>  }
>  
> -- 
> 2.29.2
> 

-- 
Kees Cook

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

* Re: [PATCH 1/2] kasan: test: use underlying string helpers
  2021-10-18 19:47 ` Kees Cook
@ 2021-10-18 19:55   ` Arnd Bergmann
  0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2021-10-18 19:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-hardening, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, kasan-dev, Arnd Bergmann,
	Andrew Morton, Marco Elver, Catalin Marinas, Peter Collingbourne,
	Patricia Alfonso, Vincenzo Frascino, Linux Kernel Mailing List

On Mon, Oct 18, 2021 at 9:47 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Oct 13, 2021 at 05:00:05PM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > Calling memcmp() and memchr() with an intentional buffer overflow
> > is now caught at compile time:
> >
> > In function 'memcmp',
> >     inlined from 'kasan_memcmp' at lib/test_kasan.c:897:2:
> > include/linux/fortify-string.h:263:25: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
> >   263 |                         __read_overflow();
> >       |                         ^~~~~~~~~~~~~~~~~
> > In function 'memchr',
> >     inlined from 'kasan_memchr' at lib/test_kasan.c:872:2:
> > include/linux/fortify-string.h:277:17: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
> >   277 |                 __read_overflow();
> >       |                 ^~~~~~~~~~~~~~~~~
> >
> > Change the kasan tests to wrap those inside of a noinline function
> > to prevent the compiler from noticing the bug and let kasan find
> > it at runtime.
>
> Is this with W=1 ? I had explicitly disabled the read overflows for
> "phase 1" of the overflow restriction tightening...

I have a somewhat modified source tree that builds cleanly with W=1 after
disabling all the noisy ones, so this is probably one that I would not have
seen without it.

> (And what do you think of using OPTIMIZER_HIDE_VAR() instead[1]?
>
> [1] https://lore.kernel.org/linux-hardening/20211006181544.1670992-1-keescook@chromium.org/T/#u

Yes, that is probably better. I can try updating the patch tomorrow,
unless you do it first.

       Arnd

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

* Re: [PATCH 2/2] kasan: use fortified strings for hwaddress sanitizer
  2021-10-13 15:00 ` [PATCH 2/2] kasan: use fortified strings for hwaddress sanitizer Arnd Bergmann
@ 2021-10-18 19:57   ` Kees Cook
  2021-10-18 20:09     ` Arnd Bergmann
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-10-18 19:57 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-hardening, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, kasan-dev, Arnd Bergmann,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Sami Tolvanen,
	Marco Elver, Masahiro Yamada, Ard Biesheuvel, linux-kernel, llvm

On Wed, Oct 13, 2021 at 05:00:06PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> GCC has separate macros for -fsanitize=kernel-address and
> -fsanitize=kernel-hwaddress, and the check in the arm64 string.h
> gets this wrong, which leads to string functions not getting
> fortified with gcc. The newly added tests find this:
> 
> warning: unsafe memchr() usage lacked '__read_overflow' warning in /git/arm-soc/lib/test_fortify/read_overflow-memchr.c
> warning: unsafe memchr_inv() usage lacked '__read_overflow' symbol in /git/arm-soc/lib/test_fortify/read_overflow-memchr_inv.c
> warning: unsafe memcmp() usage lacked '__read_overflow' warning in /git/arm-soc/lib/test_fortify/read_overflow-memcmp.c
> warning: unsafe memscan() usage lacked '__read_overflow' symbol in /git/arm-soc/lib/test_fortify/read_overflow-memscan.c
> warning: unsafe memcmp() usage lacked '__read_overflow2' warning in /git/arm-soc/lib/test_fortify/read_overflow2-memcmp.c
> warning: unsafe memcpy() usage lacked '__read_overflow2' symbol in /git/arm-soc/lib/test_fortify/read_overflow2-memcpy.c
> warning: unsafe memmove() usage lacked '__read_overflow2' symbol in /git/arm-soc/lib/test_fortify/read_overflow2-memmove.c
> warning: unsafe memcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memcpy.c
> warning: unsafe memmove() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memmove.c
> warning: unsafe memset() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memset.c
> warning: unsafe strcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strcpy-lit.c
> warning: unsafe strcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strcpy.c
> warning: unsafe strlcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strlcpy-src.c
> warning: unsafe strlcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strlcpy.c
> warning: unsafe strncpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strncpy-src.c
> warning: unsafe strncpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strncpy.c
> warning: unsafe strscpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strscpy.c
> 

What is the build config that trips these warnings?

In trying to understand this, I see in arch/arm64/include/asm/string.h:

#if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
        !defined(__SANITIZE_ADDRESS__)

other architectures (like arm32) do:

#if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)

so it's okay because it's not getting touched by the hwaddress sanitizer?
e.g. I see:

config CC_HAS_KASAN_GENERIC
        def_bool $(cc-option, -fsanitize=kernel-address)

config CC_HAS_KASAN_SW_TAGS
        def_bool $(cc-option, -fsanitize=kernel-hwaddress)

> Add a workaround to include/linux/compiler_types.h so we always
> define __SANITIZE_ADDRESS__ for either mode, as we already do
> for clang.

Where is the clang work-around? (Or is this a statement that clang,
under -fsanitize=kernel-hwaddress, already sets __SANITIZE_ADDRESS__ by
default?

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/linux/compiler_types.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index aad6f6408bfa..2f2776fffefe 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -178,6 +178,13 @@ struct ftrace_likely_data {
>   */
>  #define noinline_for_stack noinline
>  
> +/*
> + * Treat __SANITIZE_HWADDRESS__ the same as __SANITIZE_ADDRESS__ in the kernel
> + */
> +#ifdef __SANITIZE_HWADDRESS__
> +#define __SANITIZE_ADDRESS__
> +#endif

Should this go into compiler-gcc.h instead?

> +
>  /*
>   * Sanitizer helper attributes: Because using __always_inline and
>   * __no_sanitize_* conflict, provide helper attributes that will either expand
> -- 
> 2.29.2
> 

-- 
Kees Cook

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

* Re: [PATCH 2/2] kasan: use fortified strings for hwaddress sanitizer
  2021-10-18 19:57   ` Kees Cook
@ 2021-10-18 20:09     ` Arnd Bergmann
  0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2021-10-18 20:09 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-hardening, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, kasan-dev, Arnd Bergmann,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Sami Tolvanen,
	Marco Elver, Masahiro Yamada, Ard Biesheuvel,
	Linux Kernel Mailing List, llvm

On Mon, Oct 18, 2021 at 9:57 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Oct 13, 2021 at 05:00:06PM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > GCC has separate macros for -fsanitize=kernel-address and
> > -fsanitize=kernel-hwaddress, and the check in the arm64 string.h
> > gets this wrong, which leads to string functions not getting
> > fortified with gcc. The newly added tests find this:
> >
> > warning: unsafe memchr() usage lacked '__read_overflow' warning in /git/arm-soc/lib/test_fortify/read_overflow-memchr.c
> > warning: unsafe memchr_inv() usage lacked '__read_overflow' symbol in /git/arm-soc/lib/test_fortify/read_overflow-memchr_inv.c
> > warning: unsafe memcmp() usage lacked '__read_overflow' warning in /git/arm-soc/lib/test_fortify/read_overflow-memcmp.c
> > warning: unsafe memscan() usage lacked '__read_overflow' symbol in /git/arm-soc/lib/test_fortify/read_overflow-memscan.c
> > warning: unsafe memcmp() usage lacked '__read_overflow2' warning in /git/arm-soc/lib/test_fortify/read_overflow2-memcmp.c
> > warning: unsafe memcpy() usage lacked '__read_overflow2' symbol in /git/arm-soc/lib/test_fortify/read_overflow2-memcpy.c
> > warning: unsafe memmove() usage lacked '__read_overflow2' symbol in /git/arm-soc/lib/test_fortify/read_overflow2-memmove.c
> > warning: unsafe memcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memcpy.c
> > warning: unsafe memmove() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memmove.c
> > warning: unsafe memset() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-memset.c
> > warning: unsafe strcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strcpy-lit.c
> > warning: unsafe strcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strcpy.c
> > warning: unsafe strlcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strlcpy-src.c
> > warning: unsafe strlcpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strlcpy.c
> > warning: unsafe strncpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strncpy-src.c
> > warning: unsafe strncpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strncpy.c
> > warning: unsafe strscpy() usage lacked '__write_overflow' symbol in /git/arm-soc/lib/test_fortify/write_overflow-strscpy.c
> >
>
> What is the build config that trips these warnings?

It's a randconfig build, I've uploaded one .config to
https://pastebin.com/raw/4TKB9mhs,
but I have other ones if you can't reproduce with that one.

> In trying to understand this, I see in arch/arm64/include/asm/string.h:
>
> #if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
>         !defined(__SANITIZE_ADDRESS__)
>
> other architectures (like arm32) do:
>
> #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)

Yes, that is exactly the thing that goes wrong. With clang, __SANITIZE_ADDRESS__
gets set here, but gcc sets __SANITIZE_HWADDRESS__ instead
for CONFIG_KASAN_SW_TAGS, so the condition is always true.

> > Add a workaround to include/linux/compiler_types.h so we always
> > define __SANITIZE_ADDRESS__ for either mode, as we already do
> > for clang.
>
> Where is the clang work-around? (Or is this a statement that clang,
> under -fsanitize=kernel-hwaddress, already sets __SANITIZE_ADDRESS__ by
> default?

I mean this snippet:

#if __has_feature(address_sanitizer) || __has_feature(hwaddress_sanitizer)
/* Emulate GCC's __SANITIZE_ADDRESS__ flag */
#define __SANITIZE_ADDRESS__
#endif

Without that, clang sets neither __SANITIZE_ADDRESS__ nor
__SANITIZE_HWADDRESS__

> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > index aad6f6408bfa..2f2776fffefe 100644
> > --- a/include/linux/compiler_types.h
> > +++ b/include/linux/compiler_types.h
> > @@ -178,6 +178,13 @@ struct ftrace_likely_data {
> >   */
> >  #define noinline_for_stack noinline
> >
> > +/*
> > + * Treat __SANITIZE_HWADDRESS__ the same as __SANITIZE_ADDRESS__ in the kernel
> > + */
> > +#ifdef __SANITIZE_HWADDRESS__
> > +#define __SANITIZE_ADDRESS__
> > +#endif
>
> Should this go into compiler-gcc.h instead?

Yes, that might be clearer, but the effect is the same, as no other
compiler defines
those macros.

       Arnd

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

* Re: [PATCH 1/2] kasan: test: use underlying string helpers
  2021-10-15  2:40   ` Kees Cook
@ 2021-10-28 20:15     ` Andrew Morton
  2021-10-28 20:42       ` Kees Cook
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2021-10-28 20:15 UTC (permalink / raw)
  To: Kees Cook
  Cc: Vincenzo Frascino, Arnd Bergmann, linux-hardening, Kees Cook,
	Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
	Dmitry Vyukov, kasan-dev, Arnd Bergmann, Marco Elver,
	Catalin Marinas, Peter Collingbourne, Patricia Alfonso,
	linux-kernel

On Thu, 14 Oct 2021 19:40:45 -0700 Kees Cook <keescook@chromium.org> wrote:

> 
> 
> On October 14, 2021 1:12:54 AM PDT, Vincenzo Frascino <vincenzo.frascino@arm.com> wrote:
> >
> >
> >On 10/13/21 5:00 PM, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> 
> >> Calling memcmp() and memchr() with an intentional buffer overflow
> >> is now caught at compile time:
> >> 
> >> In function 'memcmp',
> >>     inlined from 'kasan_memcmp' at lib/test_kasan.c:897:2:
> >> include/linux/fortify-string.h:263:25: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
> >>   263 |                         __read_overflow();
> >>       |                         ^~~~~~~~~~~~~~~~~
> >> In function 'memchr',
> >>     inlined from 'kasan_memchr' at lib/test_kasan.c:872:2:
> >> include/linux/fortify-string.h:277:17: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
> >>   277 |                 __read_overflow();
> >>       |                 ^~~~~~~~~~~~~~~~~
> >> 
> >> Change the kasan tests to wrap those inside of a noinline function
> >> to prevent the compiler from noticing the bug and let kasan find
> >> it at runtime.
> >> 
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> >Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> 
> How about just explicitly making the size invisible to the compiler?
> 
> I did this for similar issues in the same source:
> 
> https://lore.kernel.org/linux-hardening/20211006181544.1670992-1-keescook@chromium.org/T/#u
> 

Arnd?

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

* Re: [PATCH 1/2] kasan: test: use underlying string helpers
  2021-10-28 20:15     ` Andrew Morton
@ 2021-10-28 20:42       ` Kees Cook
  0 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2021-10-28 20:42 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Vincenzo Frascino, Arnd Bergmann, linux-hardening, Kees Cook,
	Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
	Dmitry Vyukov, kasan-dev, Arnd Bergmann, Marco Elver,
	Catalin Marinas, Peter Collingbourne, Patricia Alfonso,
	linux-kernel

On Thu, Oct 28, 2021 at 01:15:26PM -0700, Andrew Morton wrote:
> On Thu, 14 Oct 2021 19:40:45 -0700 Kees Cook <keescook@chromium.org> wrote:
> 
> > 
> > 
> > On October 14, 2021 1:12:54 AM PDT, Vincenzo Frascino <vincenzo.frascino@arm.com> wrote:
> > >
> > >
> > >On 10/13/21 5:00 PM, Arnd Bergmann wrote:
> > >> From: Arnd Bergmann <arnd@arndb.de>
> > >> 
> > >> Calling memcmp() and memchr() with an intentional buffer overflow
> > >> is now caught at compile time:
> > >> 
> > >> In function 'memcmp',
> > >>     inlined from 'kasan_memcmp' at lib/test_kasan.c:897:2:
> > >> include/linux/fortify-string.h:263:25: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
> > >>   263 |                         __read_overflow();
> > >>       |                         ^~~~~~~~~~~~~~~~~
> > >> In function 'memchr',
> > >>     inlined from 'kasan_memchr' at lib/test_kasan.c:872:2:
> > >> include/linux/fortify-string.h:277:17: error: call to '__read_overflow' declared with attribute error: detected read beyond size of object (1st parameter)
> > >>   277 |                 __read_overflow();
> > >>       |                 ^~~~~~~~~~~~~~~~~
> > >> 
> > >> Change the kasan tests to wrap those inside of a noinline function
> > >> to prevent the compiler from noticing the bug and let kasan find
> > >> it at runtime.
> > >> 
> > >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > >
> > >Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > 
> > How about just explicitly making the size invisible to the compiler?
> > 
> > I did this for similar issues in the same source:
> > 
> > https://lore.kernel.org/linux-hardening/20211006181544.1670992-1-keescook@chromium.org/T/#u

This is already fixed in your tree with:

"kasan: test: consolidate workarounds for unwanted __alloc_size() protection"

which was based on this original patch (and my comments).

-- 
Kees Cook

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

end of thread, other threads:[~2021-10-28 20:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13 15:00 [PATCH 1/2] kasan: test: use underlying string helpers Arnd Bergmann
2021-10-13 15:00 ` [PATCH 2/2] kasan: use fortified strings for hwaddress sanitizer Arnd Bergmann
2021-10-18 19:57   ` Kees Cook
2021-10-18 20:09     ` Arnd Bergmann
2021-10-14  8:12 ` [PATCH 1/2] kasan: test: use underlying string helpers Vincenzo Frascino
2021-10-15  2:40   ` Kees Cook
2021-10-28 20:15     ` Andrew Morton
2021-10-28 20:42       ` Kees Cook
2021-10-18 19:47 ` Kees Cook
2021-10-18 19:55   ` Arnd Bergmann

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