[PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[Kees Cook]

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

Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[Nathan Chancellor]

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
> 

Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[Miguel Ojeda]

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

Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[Kees Cook]

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

Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[Marco Elver]

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
>

Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[Kees Cook]

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

Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[Marco Elver]

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

Re: [PATCH] compiler-gcc.h: Define __SANITIZE_ADDRESS__ under hwaddress sanitizer

[Konstantin Ryabitsev]

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