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 <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Miguel Ojeda <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Arvind Sankar <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
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 <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Miguel Ojeda <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Arvind Sankar <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> 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
>
On Wed, Oct 20, 2021 at 10:00 PM Kees Cook <[email protected]> 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 <[email protected]>
Cheers,
Miguel
On Wed, 20 Oct 2021 at 22:00, Kees Cook <[email protected]> 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 <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Miguel Ojeda <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Arvind Sankar <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Other than that,
Reviewed-by: Marco Elver <[email protected]>
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
>
On Thu, Oct 21, 2021 at 12:45:02AM +0200, Miguel Ojeda wrote:
> On Wed, Oct 20, 2021 at 10:00 PM Kees Cook <[email protected]> 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 <[email protected]>
Thanks!
--
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 <[email protected]> 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 <[email protected]>
> > Cc: Nathan Chancellor <[email protected]>
> > Cc: Nick Desaulniers <[email protected]>
> > Cc: Miguel Ojeda <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Marco Elver <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Arvind Sankar <[email protected]>
> > Cc: Masahiro Yamada <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
>
> Other than that,
>
> Reviewed-by: Marco Elver <[email protected]>
Thanks! (Oh, BTW, it seems "b4" won't include your Reviewed-by: tag if
it is indented like this.)
-Kees
--
Kees Cook
On Thu, 21 Oct 2021 at 10:43, Kees Cook <[email protected]> wrote:
> > Other than that,
> >
> > Reviewed-by: Marco Elver <[email protected]>
>
> 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
On Thu, Oct 21, 2021 at 10:46:39AM +0200, Marco Elver wrote:
> > > Reviewed-by: Marco Elver <[email protected]>
> >
> > 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