[PATCH 1/2] kasan: test: use underlying string helpers

[Arnd Bergmann]

From: Arnd Bergmann <ar...@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 <ar...@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

[Arnd Bergmann]

From: Arnd Bergmann <ar...@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 <ar...@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

[Vincenzo Frascino]

On 10/13/21 5:00 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <ar...@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 <ar...@arndb.de>

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

Regards,
Vincenzo

[Kees Cook]

On October 14, 2021 1:12:54 AM PDT, Vincenzo Frascino <vincenzo...@arm.com> wrote:
>
>
>On 10/13/21 5:00 PM, Arnd Bergmann wrote:
>> From: Arnd Bergmann <ar...@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 <ar...@arndb.de>
>
>Reviewed-by: Vincenzo Frascino <vincenzo...@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.1...@chromium.org/T/#u


-Kees

Kees Cook

[Kees Cook]

On Wed, Oct 13, 2021 at 05:00:05PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <ar...@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.1...@chromium.org/T/#u

--
Kees Cook

[Arnd Bergmann]

On Mon, Oct 18, 2021 at 9:47 PM Kees Cook <kees...@chromium.org> wrote:
> On Wed, Oct 13, 2021 at 05:00:05PM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <ar...@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.1...@chromium.org/T/#u

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

Arnd

[Kees Cook]

On Wed, Oct 13, 2021 at 05:00:06PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <ar...@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 <ar...@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

[Arnd Bergmann]

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

[Andrew Morton]

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

>
>
> On October 14, 2021 1:12:54 AM PDT, Vincenzo Frascino <vincenzo...@arm.com> wrote:
> >
> >
> >On 10/13/21 5:00 PM, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <ar...@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 <ar...@arndb.de>
> >
> >Reviewed-by: Vincenzo Frascino <vincenzo...@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.1...@chromium.org/T/#u
>

Arnd?

[Kees Cook]

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