* [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer
@ 2021-10-20 20:00 Kees Cook
2021-10-20 21:59 ` Nathan Chancellor
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Kees Cook @ 2021-10-20 20:00 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Kees Cook, Arnd Bergmann, Nathan Chancellor, Nick Desaulniers,
Andrew Morton, Marco Elver, Will Deacon, Arvind Sankar,
Masahiro Yamada, llvm, Ard Biesheuvel, Luc Van Oostenryck,
linux-kernel, linux-hardening
When Clang is using the hwaddress sanitizer, it sets __SANITIZE_ADDRESS__
explicitly:
#if __has_feature(address_sanitizer) || __has_feature(hwaddress_sanitizer)
/* Emulate GCC's __SANITIZE_ADDRESS__ flag */
#define __SANITIZE_ADDRESS__
#endif
Once hwaddress sanitizer was added to GCC, however, a separate define
was created, __SANITIZE_HWADDRESS__. The kernel is expecting to find
__SANITIZE_ADDRESS__ in either case, though, and the existing string
macros break on supported architectures:
#if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
!defined(__SANITIZE_ADDRESS__)
where as other architectures (like arm32) have no idea about hwaddress
sanitizer and just check for __SANITIZE_ADDRESS__:
#if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
This would lead to compiler foritfy self-test warnings when building
with CONFIG_KASAN_SW_TAGS=y:
warning: unsafe memmove() usage lacked '__read_overflow2' symbol in lib/test_fortify/read_overflow2-memmove.c
warning: unsafe memcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memcpy.c
...
Sort this out by also defining __SANITIZE_ADDRESS__ in GCC under the
hwaddress sanitizer.
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Marco Elver <elver@google.com>
Cc: Will Deacon <will@kernel.org>
Cc: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
I'm intending to take this via my overflow series, since that is what introduces
the compile-test regression tests (which found this legitimate bug). :)
-Kees
---
include/linux/compiler-gcc.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 6f24eb8c5dda..ccbbd31b3aae 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -121,6 +121,14 @@
#define __no_sanitize_coverage
#endif
+/*
+ * Treat __SANITIZE_HWADDRESS__ the same as __SANITIZE_ADDRESS__ in the kernel,
+ * matching the defines used by Clang.
+ */
+#ifdef __SANITIZE_HWADDRESS__
+#define __SANITIZE_ADDRESS__
+#endif
+
/*
* Turn individual warnings and errors on and off locally, depending
* on version.
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer
2021-10-20 20:00 [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer Kees Cook
@ 2021-10-20 21:59 ` Nathan Chancellor
2021-10-20 22:45 ` Miguel Ojeda
2021-10-21 6:00 ` Marco Elver
2 siblings, 0 replies; 8+ messages in thread
From: Nathan Chancellor @ 2021-10-20 21:59 UTC (permalink / raw)
To: Kees Cook
Cc: Miguel Ojeda, Arnd Bergmann, Nick Desaulniers, Andrew Morton,
Marco Elver, Will Deacon, Arvind Sankar, Masahiro Yamada, llvm,
Ard Biesheuvel, Luc Van Oostenryck, linux-kernel,
linux-hardening
On Wed, Oct 20, 2021 at 01:00:39PM -0700, Kees Cook wrote:
> When Clang is using the hwaddress sanitizer, it sets __SANITIZE_ADDRESS__
> explicitly:
>
> #if __has_feature(address_sanitizer) || __has_feature(hwaddress_sanitizer)
> /* Emulate GCC's __SANITIZE_ADDRESS__ flag */
> #define __SANITIZE_ADDRESS__
> #endif
>
> Once hwaddress sanitizer was added to GCC, however, a separate define
> was created, __SANITIZE_HWADDRESS__. The kernel is expecting to find
> __SANITIZE_ADDRESS__ in either case, though, and the existing string
> macros break on supported architectures:
>
> #if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
> !defined(__SANITIZE_ADDRESS__)
>
> where as other architectures (like arm32) have no idea about hwaddress
> sanitizer and just check for __SANITIZE_ADDRESS__:
>
> #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
>
> This would lead to compiler foritfy self-test warnings when building
> with CONFIG_KASAN_SW_TAGS=y:
>
> warning: unsafe memmove() usage lacked '__read_overflow2' symbol in lib/test_fortify/read_overflow2-memmove.c
> warning: unsafe memcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memcpy.c
> ...
>
> Sort this out by also defining __SANITIZE_ADDRESS__ in GCC under the
> hwaddress sanitizer.
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Marco Elver <elver@google.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Arvind Sankar <nivedita@alum.mit.edu>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: llvm@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> ---
> I'm intending to take this via my overflow series, since that is what introduces
> the compile-test regression tests (which found this legitimate bug). :)
>
> -Kees
> ---
> include/linux/compiler-gcc.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 6f24eb8c5dda..ccbbd31b3aae 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -121,6 +121,14 @@
> #define __no_sanitize_coverage
> #endif
>
> +/*
> + * Treat __SANITIZE_HWADDRESS__ the same as __SANITIZE_ADDRESS__ in the kernel,
> + * matching the defines used by Clang.
> + */
> +#ifdef __SANITIZE_HWADDRESS__
> +#define __SANITIZE_ADDRESS__
> +#endif
> +
> /*
> * Turn individual warnings and errors on and off locally, depending
> * on version.
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer
2021-10-20 20:00 [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer Kees Cook
2021-10-20 21:59 ` Nathan Chancellor
@ 2021-10-20 22:45 ` Miguel Ojeda
2021-10-21 8:41 ` Kees Cook
2021-10-21 6:00 ` Marco Elver
2 siblings, 1 reply; 8+ messages in thread
From: Miguel Ojeda @ 2021-10-20 22:45 UTC (permalink / raw)
To: Kees Cook
Cc: Miguel Ojeda, Arnd Bergmann, Nathan Chancellor, Nick Desaulniers,
Andrew Morton, Marco Elver, Will Deacon, Arvind Sankar,
Masahiro Yamada, llvm, Ard Biesheuvel, Luc Van Oostenryck,
linux-kernel, linux-hardening
On Wed, Oct 20, 2021 at 10:00 PM Kees Cook <keescook@chromium.org> wrote:
>
> I'm intending to take this via my overflow series, since that is what introduces
> the compile-test regression tests (which found this legitimate bug). :)
Not sure if there is a particular reason I was in the `To` field
(please let me know if so), but the patch sounds good to me!
Acked-by: Miguel Ojeda <ojeda@kernel.org>
Cheers,
Miguel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer
2021-10-20 20:00 [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer Kees Cook
2021-10-20 21:59 ` Nathan Chancellor
2021-10-20 22:45 ` Miguel Ojeda
@ 2021-10-21 6:00 ` Marco Elver
2021-10-21 8:43 ` Kees Cook
2 siblings, 1 reply; 8+ messages in thread
From: Marco Elver @ 2021-10-21 6:00 UTC (permalink / raw)
To: Kees Cook
Cc: Miguel Ojeda, Arnd Bergmann, Nathan Chancellor, Nick Desaulniers,
Andrew Morton, Will Deacon, Arvind Sankar, Masahiro Yamada, llvm,
Ard Biesheuvel, Luc Van Oostenryck, linux-kernel,
linux-hardening, Andrey Konovalov, kasan-dev
On Wed, 20 Oct 2021 at 22:00, Kees Cook <keescook@chromium.org> wrote:
> When Clang is using the hwaddress sanitizer, it sets __SANITIZE_ADDRESS__
> explicitly:
>
> #if __has_feature(address_sanitizer) || __has_feature(hwaddress_sanitizer)
> /* Emulate GCC's __SANITIZE_ADDRESS__ flag */
> #define __SANITIZE_ADDRESS__
> #endif
Hmm, the comment is a little inaccurate if hwaddress sanitizer is on,
but I certainly wouldn't want compiler-clang.h to start emulating gcc
here and start defining __SANITIZE_HWADDRESS__ if the places where we
check it are the same as __SANITIZE_ADDRESS__. So this patch is the
right approach.
> Once hwaddress sanitizer was added to GCC, however, a separate define
> was created, __SANITIZE_HWADDRESS__. The kernel is expecting to find
> __SANITIZE_ADDRESS__ in either case, though, and the existing string
> macros break on supported architectures:
>
> #if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
> !defined(__SANITIZE_ADDRESS__)
>
> where as other architectures (like arm32) have no idea about hwaddress
> sanitizer and just check for __SANITIZE_ADDRESS__:
>
> #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
arm32 doesn't support KASAN_SW_TAGS, so I think the bit about arm32 is
irrelevant.
Only arm64 can, and the reason that arm64 doesn't check against
"defined(CONFIG_KASAN)" is because we also have KASAN_HW_TAGS (no
compiler instrumentation).
> This would lead to compiler foritfy self-test warnings when building
> with CONFIG_KASAN_SW_TAGS=y:
>
> warning: unsafe memmove() usage lacked '__read_overflow2' symbol in lib/test_fortify/read_overflow2-memmove.c
> warning: unsafe memcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memcpy.c
> ...
>
> Sort this out by also defining __SANITIZE_ADDRESS__ in GCC under the
> hwaddress sanitizer.
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Marco Elver <elver@google.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Arvind Sankar <nivedita@alum.mit.edu>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: llvm@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>
Other than that,
Reviewed-by: Marco Elver <elver@google.com>
Thanks!
> ---
> I'm intending to take this via my overflow series, since that is what introduces
> the compile-test regression tests (which found this legitimate bug). :)
>
> -Kees
> ---
> include/linux/compiler-gcc.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 6f24eb8c5dda..ccbbd31b3aae 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -121,6 +121,14 @@
> #define __no_sanitize_coverage
> #endif
>
> +/*
> + * Treat __SANITIZE_HWADDRESS__ the same as __SANITIZE_ADDRESS__ in the kernel,
> + * matching the defines used by Clang.
> + */
> +#ifdef __SANITIZE_HWADDRESS__
> +#define __SANITIZE_ADDRESS__
> +#endif
> +
> /*
> * Turn individual warnings and errors on and off locally, depending
> * on version.
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer
2021-10-20 22:45 ` Miguel Ojeda
@ 2021-10-21 8:41 ` Kees Cook
0 siblings, 0 replies; 8+ messages in thread
From: Kees Cook @ 2021-10-21 8:41 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Miguel Ojeda, Arnd Bergmann, Nathan Chancellor, Nick Desaulniers,
Andrew Morton, Marco Elver, Will Deacon, Arvind Sankar,
Masahiro Yamada, llvm, Ard Biesheuvel, Luc Van Oostenryck,
linux-kernel, linux-hardening
On Thu, Oct 21, 2021 at 12:45:02AM +0200, Miguel Ojeda wrote:
> On Wed, Oct 20, 2021 at 10:00 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > I'm intending to take this via my overflow series, since that is what introduces
> > the compile-test regression tests (which found this legitimate bug). :)
>
> Not sure if there is a particular reason I was in the `To` field
> (please let me know if so), but the patch sounds good to me!
Well, I didn't want the "To" to be me, and IIRC, you had sent a review
for Arnd's prior version.
> Acked-by: Miguel Ojeda <ojeda@kernel.org>
Thanks!
--
Kees Cook
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer
2021-10-21 6:00 ` Marco Elver
@ 2021-10-21 8:43 ` Kees Cook
2021-10-21 8:46 ` Marco Elver
0 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2021-10-21 8:43 UTC (permalink / raw)
To: Marco Elver
Cc: Miguel Ojeda, Arnd Bergmann, Nathan Chancellor, Nick Desaulniers,
Andrew Morton, Will Deacon, Arvind Sankar, Masahiro Yamada, llvm,
Ard Biesheuvel, Luc Van Oostenryck, linux-kernel,
linux-hardening, Andrey Konovalov, kasan-dev
On Thu, Oct 21, 2021 at 08:00:00AM +0200, Marco Elver wrote:
> On Wed, 20 Oct 2021 at 22:00, Kees Cook <keescook@chromium.org> wrote:
> > When Clang is using the hwaddress sanitizer, it sets __SANITIZE_ADDRESS__
> > explicitly:
> >
> > #if __has_feature(address_sanitizer) || __has_feature(hwaddress_sanitizer)
> > /* Emulate GCC's __SANITIZE_ADDRESS__ flag */
> > #define __SANITIZE_ADDRESS__
> > #endif
>
> Hmm, the comment is a little inaccurate if hwaddress sanitizer is on,
> but I certainly wouldn't want compiler-clang.h to start emulating gcc
> here and start defining __SANITIZE_HWADDRESS__ if the places where we
> check it are the same as __SANITIZE_ADDRESS__. So this patch is the
> right approach.
Yeah, I agree. I think that was Arnd's thinking as well.
>
> > Once hwaddress sanitizer was added to GCC, however, a separate define
> > was created, __SANITIZE_HWADDRESS__. The kernel is expecting to find
> > __SANITIZE_ADDRESS__ in either case, though, and the existing string
> > macros break on supported architectures:
> >
> > #if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
> > !defined(__SANITIZE_ADDRESS__)
> >
> > where as other architectures (like arm32) have no idea about hwaddress
> > sanitizer and just check for __SANITIZE_ADDRESS__:
> >
> > #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
>
> arm32 doesn't support KASAN_SW_TAGS, so I think the bit about arm32 is
> irrelevant.
Right -- I had just picked an example.
> Only arm64 can, and the reason that arm64 doesn't check against
> "defined(CONFIG_KASAN)" is because we also have KASAN_HW_TAGS (no
> compiler instrumentation).
>
> > This would lead to compiler foritfy self-test warnings when building
> > with CONFIG_KASAN_SW_TAGS=y:
> >
> > warning: unsafe memmove() usage lacked '__read_overflow2' symbol in lib/test_fortify/read_overflow2-memmove.c
> > warning: unsafe memcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memcpy.c
> > ...
> >
> > Sort this out by also defining __SANITIZE_ADDRESS__ in GCC under the
> > hwaddress sanitizer.
> >
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Cc: Nathan Chancellor <nathan@kernel.org>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Miguel Ojeda <ojeda@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Marco Elver <elver@google.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Arvind Sankar <nivedita@alum.mit.edu>
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Cc: llvm@lists.linux.dev
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Other than that,
>
> Reviewed-by: Marco Elver <elver@google.com>
Thanks! (Oh, BTW, it seems "b4" won't include your Reviewed-by: tag if
it is indented like this.)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer
2021-10-21 8:43 ` Kees Cook
@ 2021-10-21 8:46 ` Marco Elver
2021-10-21 13:50 ` Konstantin Ryabitsev
0 siblings, 1 reply; 8+ messages in thread
From: Marco Elver @ 2021-10-21 8:46 UTC (permalink / raw)
To: Kees Cook
Cc: Miguel Ojeda, Arnd Bergmann, Nathan Chancellor, Nick Desaulniers,
Andrew Morton, Will Deacon, Arvind Sankar, Masahiro Yamada, llvm,
Ard Biesheuvel, Luc Van Oostenryck, linux-kernel,
linux-hardening, Andrey Konovalov, kasan-dev,
Konstantin Ryabitsev
On Thu, 21 Oct 2021 at 10:43, Kees Cook <keescook@chromium.org> wrote:
> > Other than that,
> >
> > Reviewed-by: Marco Elver <elver@google.com>
>
> Thanks! (Oh, BTW, it seems "b4" won't include your Reviewed-by: tag if
> it is indented like this.)
Ah, I'll stop doing that then -- or can we make b4 play along?
Thanks,
-- Marco
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer
2021-10-21 8:46 ` Marco Elver
@ 2021-10-21 13:50 ` Konstantin Ryabitsev
0 siblings, 0 replies; 8+ messages in thread
From: Konstantin Ryabitsev @ 2021-10-21 13:50 UTC (permalink / raw)
To: Marco Elver
Cc: Kees Cook, Miguel Ojeda, Arnd Bergmann, Nathan Chancellor,
Nick Desaulniers, Andrew Morton, Will Deacon, Arvind Sankar,
Masahiro Yamada, llvm, Ard Biesheuvel, Luc Van Oostenryck,
linux-kernel, linux-hardening, Andrey Konovalov, kasan-dev
On Thu, Oct 21, 2021 at 10:46:39AM +0200, Marco Elver wrote:
> > > Reviewed-by: Marco Elver <elver@google.com>
> >
> > Thanks! (Oh, BTW, it seems "b4" won't include your Reviewed-by: tag if
> > it is indented like this.)
>
> Ah, I'll stop doing that then -- or can we make b4 play along?
I'd rather not allow for that, as this can lead to increased false-positive
rates. It's already a bit too much of a cross-your-fingers kind of thing.
-K
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-21 13:51 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-20 20:00 [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer Kees Cook
2021-10-20 21:59 ` Nathan Chancellor
2021-10-20 22:45 ` Miguel Ojeda
2021-10-21 8:41 ` Kees Cook
2021-10-21 6:00 ` Marco Elver
2021-10-21 8:43 ` Kees Cook
2021-10-21 8:46 ` Marco Elver
2021-10-21 13:50 ` Konstantin Ryabitsev
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).