[PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Building ubsan kernels even for compile-testing introduced these
warnings in my randconfig environment:

crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
static void blake2b_compress(struct blake2b_state *S,
crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])

Further testing showed that this is caused by
-fsanitize=unsigned-integer-overflow.

The one in blake2b immediately overflows the 8KB stack area on 32-bit
architectures, so better ensure this never happens by making this
option gcc-only.

Fixes: d0a3ac549f38 ("ubsan: enable for all*config builds")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 lib/Kconfig.ubsan | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index 8b635fd75fe4..e23873282ba7 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
 
 config UBSAN_UNSIGNED_OVERFLOW
 	bool "Perform checking for unsigned arithmetic overflow"
+	# clang hugely expands stack usage with -fsanitize=object-size
+	depends on !CC_IS_CLANG
 	depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
 	help
 	  This option enables -fsanitize=unsigned-integer-overflow which checks
-- 
2.29.2

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Marco Elver]

On Wed, 30 Dec 2020 at 16:47, Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> Building ubsan kernels even for compile-testing introduced these
> warnings in my randconfig environment:
>
> crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> static void blake2b_compress(struct blake2b_state *S,
> crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])
>
> Further testing showed that this is caused by
> -fsanitize=unsigned-integer-overflow.
>
> The one in blake2b immediately overflows the 8KB stack area on 32-bit
> architectures, so better ensure this never happens by making this
> option gcc-only.
>
> Fixes: d0a3ac549f38 ("ubsan: enable for all*config builds")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  lib/Kconfig.ubsan | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 8b635fd75fe4..e23873282ba7 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
>
>  config UBSAN_UNSIGNED_OVERFLOW
>         bool "Perform checking for unsigned arithmetic overflow"
> +       # clang hugely expands stack usage with -fsanitize=object-size

This is the first time -fsanitize=object-size is mentioned. Typo?

> +       depends on !CC_IS_CLANG
>         depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
>         help
>           This option enables -fsanitize=unsigned-integer-overflow which checks
> --
> 2.29.2
>

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Nathan Chancellor]

On Wed, Dec 30, 2020 at 05:13:03PM +0100, Marco Elver wrote:
> On Wed, 30 Dec 2020 at 16:47, Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > Building ubsan kernels even for compile-testing introduced these
> > warnings in my randconfig environment:
> >
> > crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> > static void blake2b_compress(struct blake2b_state *S,
> > crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> > static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> > lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> > static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> > lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> > static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])
> >
> > Further testing showed that this is caused by
> > -fsanitize=unsigned-integer-overflow.
> >
> > The one in blake2b immediately overflows the 8KB stack area on 32-bit
> > architectures, so better ensure this never happens by making this
> > option gcc-only.

This patch also fixes the failed BUILD_BUG issue in mm/mremap.c that you
sent a patch for [1], along with a couple of other issues I see such as:

ld.lld: error: undefined symbol: __bad_mask
>>> referenced by gpi.c
>>>               dma/qcom/gpi.o:(gpi_update_reg) in archive
>>>               drivers/built-in.a
>>> referenced by gpi.c
>>>               dma/qcom/gpi.o:(gpi_update_reg) in archive
>>>               drivers/built-in.a

[1]: https://lore.kernel.org/lkml/20201230154104.522605-1-arnd@kernel.org/

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> > Fixes: d0a3ac549f38 ("ubsan: enable for all*config builds")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  lib/Kconfig.ubsan | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> > index 8b635fd75fe4..e23873282ba7 100644
> > --- a/lib/Kconfig.ubsan
> > +++ b/lib/Kconfig.ubsan
> > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> >
> >  config UBSAN_UNSIGNED_OVERFLOW
> >         bool "Perform checking for unsigned arithmetic overflow"
> > +       # clang hugely expands stack usage with -fsanitize=object-size
> 
> This is the first time -fsanitize=object-size is mentioned. Typo?

Copy and paste issue from CONFIG_UBSAN_OBJECT_SIZE

> > +       depends on !CC_IS_CLANG
> >         depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> >         help
> >           This option enables -fsanitize=unsigned-integer-overflow which checks
> > --
> > 2.29.2
> >

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Andrew Morton]

On Mon, 4 Jan 2021 15:33:36 -0700 Nathan Chancellor <natechancellor@gmail.com> wrote:

> > > +++ b/lib/Kconfig.ubsan
> > > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> > >
> > >  config UBSAN_UNSIGNED_OVERFLOW
> > >         bool "Perform checking for unsigned arithmetic overflow"
> > > +       # clang hugely expands stack usage with -fsanitize=object-size
> > 
> > This is the first time -fsanitize=object-size is mentioned. Typo?
> 
> Copy and paste issue from CONFIG_UBSAN_OBJECT_SIZE

This?

--- a/lib/Kconfig.ubsan~ubsan-disable-unsigned-integer-overflow-sanitizer-with-clang-fix
+++ a/lib/Kconfig.ubsan
@@ -122,7 +122,7 @@ config UBSAN_SIGNED_OVERFLOW
 
 config UBSAN_UNSIGNED_OVERFLOW
 	bool "Perform checking for unsigned arithmetic overflow"
-	# clang hugely expands stack usage with -fsanitize=object-size
+	# clang hugely expands stack usage with -fsanitize=unsigned-integer-overflow
 	depends on !CC_IS_CLANG
 	depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
 	help
_

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Nathan Chancellor]

On Mon, Jan 04, 2021 at 03:33:33PM -0800, Andrew Morton wrote:
> On Mon, 4 Jan 2021 15:33:36 -0700 Nathan Chancellor <natechancellor@gmail.com> wrote:
> 
> > > > +++ b/lib/Kconfig.ubsan
> > > > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> > > >
> > > >  config UBSAN_UNSIGNED_OVERFLOW
> > > >         bool "Perform checking for unsigned arithmetic overflow"
> > > > +       # clang hugely expands stack usage with -fsanitize=object-size
> > > 
> > > This is the first time -fsanitize=object-size is mentioned. Typo?
> > 
> > Copy and paste issue from CONFIG_UBSAN_OBJECT_SIZE
> 
> This?

Correct.

> --- a/lib/Kconfig.ubsan~ubsan-disable-unsigned-integer-overflow-sanitizer-with-clang-fix
> +++ a/lib/Kconfig.ubsan
> @@ -122,7 +122,7 @@ config UBSAN_SIGNED_OVERFLOW
>  
>  config UBSAN_UNSIGNED_OVERFLOW
>  	bool "Perform checking for unsigned arithmetic overflow"
> -	# clang hugely expands stack usage with -fsanitize=object-size
> +	# clang hugely expands stack usage with -fsanitize=unsigned-integer-overflow
>  	depends on !CC_IS_CLANG
>  	depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
>  	help
> _
> 

Cheers,
Nathan

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Arnd Bergmann]

On Mon, Jan 4, 2021 at 11:33 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> On Wed, Dec 30, 2020 at 05:13:03PM +0100, Marco Elver wrote:
> > On Wed, 30 Dec 2020 at 16:47, Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > Building ubsan kernels even for compile-testing introduced these
> > > warnings in my randconfig environment:
> > >
> > > crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> > > static void blake2b_compress(struct blake2b_state *S,
> > > crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> > > static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> > > lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> > > static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> > > lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> > > static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])
> > >
> > > Further testing showed that this is caused by
> > > -fsanitize=unsigned-integer-overflow.
> > >
> > > The one in blake2b immediately overflows the 8KB stack area on 32-bit
> > > architectures, so better ensure this never happens by making this
> > > option gcc-only.
>
> This patch also fixes the failed BUILD_BUG issue in mm/mremap.c that you
> sent a patch for [1], along with a couple of other issues I see such as:

I'm fairly sure I still saw that BUILD_BUG() even after I had applied this
patch, I would guess that one just depends on inlining decisions that
are influenced by all kinds of compiler options including
-fsanitize=unsigned-integer-overflow, so it becomes less likely.

I'll revert my other patch in the randconfig tree to see if it comes back.

      Arnd

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Arnd Bergmann]

On Tue, Jan 5, 2021 at 10:25 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Mon, Jan 4, 2021 at 11:33 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > On Wed, Dec 30, 2020 at 05:13:03PM +0100, Marco Elver wrote:
> > > On Wed, 30 Dec 2020 at 16:47, Arnd Bergmann <arnd@kernel.org> wrote:
> > > >
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > >
> > > > Building ubsan kernels even for compile-testing introduced these
> > > > warnings in my randconfig environment:
> > > >
> > > > crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> > > > static void blake2b_compress(struct blake2b_state *S,
> > > > crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> > > > static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> > > > lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> > > > static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> > > > lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> > > > static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])
> > > >
> > > > Further testing showed that this is caused by
> > > > -fsanitize=unsigned-integer-overflow.
> > > >
> > > > The one in blake2b immediately overflows the 8KB stack area on 32-bit
> > > > architectures, so better ensure this never happens by making this
> > > > option gcc-only.
> >
> > This patch also fixes the failed BUILD_BUG issue in mm/mremap.c that you
> > sent a patch for [1], along with a couple of other issues I see such as:
>
> I'm fairly sure I still saw that BUILD_BUG() even after I had applied this
> patch, I would guess that one just depends on inlining decisions that
> are influenced by all kinds of compiler options including
> -fsanitize=unsigned-integer-overflow, so it becomes less likely.
>
> I'll revert my other patch in the randconfig tree to see if it comes back.

The qcom/gpi.c failure still happens with this patch applied:

In file included from /git/arm-soc/drivers/dma/qcom/gpi.c:8:
In function 'field_multiplier',
    inlined from 'gpi_update_reg' at
/git/arm-soc/include/linux/bitfield.h:124:17:
/git/arm-soc/include/linux/bitfield.h:119:3: error: call to
'__bad_mask' declared with attribute error: bad bitfield mask
  119 |   __bad_mask();
      |   ^~~~~~~~~~~~
In function 'field_multiplier',
    inlined from 'gpi_update_reg' at
/git/arm-soc/include/linux/bitfield.h:154:1:
/git/arm-soc/include/linux/bitfield.h:119:3: error: call to
'__bad_mask' declared with attribute error: bad bitfield mask
  119 |   __bad_mask();
      |   ^~~~~~~~~~~~

See https://pastebin.com/8UH6x4A2 for the .config

       Arnd

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Nathan Chancellor]

On Wed, Jan 06, 2021 at 10:12:51AM +0100, Arnd Bergmann wrote:
> On Tue, Jan 5, 2021 at 10:25 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > On Mon, Jan 4, 2021 at 11:33 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > > On Wed, Dec 30, 2020 at 05:13:03PM +0100, Marco Elver wrote:
> > > > On Wed, 30 Dec 2020 at 16:47, Arnd Bergmann <arnd@kernel.org> wrote:
> > > > >
> > > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > >
> > > > > Building ubsan kernels even for compile-testing introduced these
> > > > > warnings in my randconfig environment:
> > > > >
> > > > > crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> > > > > static void blake2b_compress(struct blake2b_state *S,
> > > > > crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> > > > > static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> > > > > lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> > > > > static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> > > > > lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> > > > > static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])
> > > > >
> > > > > Further testing showed that this is caused by
> > > > > -fsanitize=unsigned-integer-overflow.
> > > > >
> > > > > The one in blake2b immediately overflows the 8KB stack area on 32-bit
> > > > > architectures, so better ensure this never happens by making this
> > > > > option gcc-only.
> > >
> > > This patch also fixes the failed BUILD_BUG issue in mm/mremap.c that you
> > > sent a patch for [1], along with a couple of other issues I see such as:
> >
> > I'm fairly sure I still saw that BUILD_BUG() even after I had applied this
> > patch, I would guess that one just depends on inlining decisions that
> > are influenced by all kinds of compiler options including
> > -fsanitize=unsigned-integer-overflow, so it becomes less likely.
> >
> > I'll revert my other patch in the randconfig tree to see if it comes back.
> 
> The qcom/gpi.c failure still happens with this patch applied:
> 
> In file included from /git/arm-soc/drivers/dma/qcom/gpi.c:8:
> In function 'field_multiplier',
>     inlined from 'gpi_update_reg' at
> /git/arm-soc/include/linux/bitfield.h:124:17:
> /git/arm-soc/include/linux/bitfield.h:119:3: error: call to
> '__bad_mask' declared with attribute error: bad bitfield mask
>   119 |   __bad_mask();
>       |   ^~~~~~~~~~~~
> In function 'field_multiplier',
>     inlined from 'gpi_update_reg' at
> /git/arm-soc/include/linux/bitfield.h:154:1:
> /git/arm-soc/include/linux/bitfield.h:119:3: error: call to
> '__bad_mask' declared with attribute error: bad bitfield mask
>   119 |   __bad_mask();
>       |   ^~~~~~~~~~~~
> 
> See https://pastebin.com/8UH6x4A2 for the .config
> 
>        Arnd

That config does not build for me, am I holding it wrong?

$ curl -LSso .config https://pastebin.com/raw/8UH6x4A2

$ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- LLVM=1 olddefconfig vmlinux
.config:364:warning: override: ARCH_DOVE changes choice state
arch/arm/kernel/sys_oabi-compat.c:257:6: error: implicit declaration of function 'ep_op_has_event' [-Werror,-Wimplicit-function-declaration]
        if (ep_op_has_event(op) &&
            ^
arch/arm/kernel/sys_oabi-compat.c:264:9: error: implicit declaration of function 'do_epoll_ctl' [-Werror,-Wimplicit-function-declaration]
        return do_epoll_ctl(epfd, op, fd, &kernel, false);
               ^
arch/arm/kernel/sys_oabi-compat.c:264:9: note: did you mean 'sys_epoll_ctl'?
./include/linux/syscalls.h:359:17: note: 'sys_epoll_ctl' declared here
asmlinkage long sys_epoll_ctl(int epfd, int op, int fd,
                ^
2 errors generated.
...

Cheers,
Nathan

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Kees Cook]

On Wed, Dec 30, 2020 at 04:47:35PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Building ubsan kernels even for compile-testing introduced these
> warnings in my randconfig environment:
> 
> crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> static void blake2b_compress(struct blake2b_state *S,
> crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])
> 
> Further testing showed that this is caused by
> -fsanitize=unsigned-integer-overflow.
> 
> The one in blake2b immediately overflows the 8KB stack area on 32-bit
> architectures, so better ensure this never happens by making this
> option gcc-only.
> 
> Fixes: d0a3ac549f38 ("ubsan: enable for all*config builds")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  lib/Kconfig.ubsan | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 8b635fd75fe4..e23873282ba7 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
>  
>  config UBSAN_UNSIGNED_OVERFLOW
>  	bool "Perform checking for unsigned arithmetic overflow"
> +	# clang hugely expands stack usage with -fsanitize=object-size
> +	depends on !CC_IS_CLANG
>  	depends on $(cc-option,-fsanitize=unsigned-integer-overflow)

Because of Clang implementation issues (see commit c637693b20da), this is
already "default n" (and not supported under GCC at all). IIUC, setting
this to "depends on !COMPILE_TEST" won't work for randconfigs, yes?

Is there some better way to mark this as "known to have issues, please
don't include in randconfig?"

I'd like to keep it around so people can continue to work out the
problems with it, but not have unexpecting folks trip over it. ;)

-- 
Kees Cook

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Arnd Bergmann]

On Wed, Jan 6, 2021 at 10:38 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> On Wed, Jan 06, 2021 at 10:12:51AM +0100, Arnd Bergmann wrote:
> > On Tue, Jan 5, 2021 at 10:25 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > > On Mon, Jan 4, 2021 at 11:33 PM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > > > On Wed, Dec 30, 2020 at 05:13:03PM +0100, Marco Elver wrote:
> > > > > On Wed, 30 Dec 2020 at 16:47, Arnd Bergmann <arnd@kernel.org> wrote:
> > > > > >
> > > > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > > >
> > > > > > Building ubsan kernels even for compile-testing introduced these
> > > > > > warnings in my randconfig environment:
> > > > > >
> > > > > > crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> > > > > > static void blake2b_compress(struct blake2b_state *S,
> > > > > > crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> > > > > > static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> > > > > > lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> > > > > > static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> > > > > > lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> > > > > > static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])
> > > > > >
> > > > > > Further testing showed that this is caused by
> > > > > > -fsanitize=unsigned-integer-overflow.
> > > > > >
> > > > > > The one in blake2b immediately overflows the 8KB stack area on 32-bit
> > > > > > architectures, so better ensure this never happens by making this
> > > > > > option gcc-only.
> > > >
> > > > This patch also fixes the failed BUILD_BUG issue in mm/mremap.c that you
> > > > sent a patch for [1], along with a couple of other issues I see such as:
> > >
> > > I'm fairly sure I still saw that BUILD_BUG() even after I had applied this
> > > patch, I would guess that one just depends on inlining decisions that
> > > are influenced by all kinds of compiler options including
> > > -fsanitize=unsigned-integer-overflow, so it becomes less likely.
> > >
> > > I'll revert my other patch in the randconfig tree to see if it comes back.
> >
> > The qcom/gpi.c failure still happens with this patch applied:
> >
> > In file included from /git/arm-soc/drivers/dma/qcom/gpi.c:8:
> > In function 'field_multiplier',
> >     inlined from 'gpi_update_reg' at
> > /git/arm-soc/include/linux/bitfield.h:124:17:
> > /git/arm-soc/include/linux/bitfield.h:119:3: error: call to
> > '__bad_mask' declared with attribute error: bad bitfield mask
> >   119 |   __bad_mask();
> >       |   ^~~~~~~~~~~~
> > In function 'field_multiplier',
> >     inlined from 'gpi_update_reg' at
> > /git/arm-soc/include/linux/bitfield.h:154:1:
> > /git/arm-soc/include/linux/bitfield.h:119:3: error: call to
> > '__bad_mask' declared with attribute error: bad bitfield mask
> >   119 |   __bad_mask();
> >       |   ^~~~~~~~~~~~
> >
> > See https://pastebin.com/8UH6x4A2 for the .config
> >
> >        Arnd
>
> That config does not build for me, am I holding it wrong?
>
> $ curl -LSso .config https://pastebin.com/raw/8UH6x4A2

Sorry about that, you ran into a bug that I have applied a
local fix for. You could enable CONFIG_EPOLL, or apply
this patch:

https://lore.kernel.org/linux-arm-kernel/20200429132349.1294904-1-arnd@arndb.de/

      Arnd

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Arnd Bergmann]

On Wed, Jan 6, 2021 at 10:57 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Dec 30, 2020 at 04:47:35PM +0100, Arnd Bergmann wrote:
> > diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> > index 8b635fd75fe4..e23873282ba7 100644
> > --- a/lib/Kconfig.ubsan
> > +++ b/lib/Kconfig.ubsan
> > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> >
> >  config UBSAN_UNSIGNED_OVERFLOW
> >       bool "Perform checking for unsigned arithmetic overflow"
> > +     # clang hugely expands stack usage with -fsanitize=object-size
> > +     depends on !CC_IS_CLANG
> >       depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
>
> Because of Clang implementation issues (see commit c637693b20da), this is
> already "default n" (and not supported under GCC at all). IIUC, setting
> this to "depends on !COMPILE_TEST" won't work for randconfigs, yes?

Ah, I had not realized this is clang specific. Adding the !COMPILE_TEST
dependency would hide it for me, which may be good enough for me.

> Is there some better way to mark this as "known to have issues, please
> don't include in randconfig?"
>
> I'd like to keep it around so people can continue to work out the
> problems with it, but not have unexpecting folks trip over it. ;)

I've reverted that patch locally now and default-enabled for randconfigs.
Now that I have an otherwise clean build, this should provide me
with a full set of files that produce the warning. If the number is
small enough, I could try opening individual github issues.

      Arnd

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Kees Cook]

On Wed, Jan 06, 2021 at 11:12:18PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 6, 2021 at 10:57 PM Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Dec 30, 2020 at 04:47:35PM +0100, Arnd Bergmann wrote:
> > > diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> > > index 8b635fd75fe4..e23873282ba7 100644
> > > --- a/lib/Kconfig.ubsan
> > > +++ b/lib/Kconfig.ubsan
> > > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> > >
> > >  config UBSAN_UNSIGNED_OVERFLOW
> > >       bool "Perform checking for unsigned arithmetic overflow"
> > > +     # clang hugely expands stack usage with -fsanitize=object-size
> > > +     depends on !CC_IS_CLANG
> > >       depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> >
> > Because of Clang implementation issues (see commit c637693b20da), this is
> > already "default n" (and not supported under GCC at all). IIUC, setting
> > this to "depends on !COMPILE_TEST" won't work for randconfigs, yes?
> 
> Ah, I had not realized this is clang specific. Adding the !COMPILE_TEST
> dependency would hide it for me, which may be good enough for me.

I thought COMPILE_TEST does not get set for randconfig?

> > Is there some better way to mark this as "known to have issues, please
> > don't include in randconfig?"
> >
> > I'd like to keep it around so people can continue to work out the
> > problems with it, but not have unexpecting folks trip over it. ;)
> 
> I've reverted that patch locally now and default-enabled for randconfigs.
> Now that I have an otherwise clean build, this should provide me
> with a full set of files that produce the warning. If the number is
> small enough, I could try opening individual github issues.

Okay, let me know if something needs changing. :)

-- 
Kees Cook

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Arnd Bergmann]

On Thu, Jan 7, 2021 at 12:17 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jan 06, 2021 at 11:12:18PM +0100, Arnd Bergmann wrote:
> > On Wed, Jan 6, 2021 at 10:57 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Wed, Dec 30, 2020 at 04:47:35PM +0100, Arnd Bergmann wrote:
> > > > diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> > > > index 8b635fd75fe4..e23873282ba7 100644
> > > > --- a/lib/Kconfig.ubsan
> > > > +++ b/lib/Kconfig.ubsan
> > > > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> > > >
> > > >  config UBSAN_UNSIGNED_OVERFLOW
> > > >       bool "Perform checking for unsigned arithmetic overflow"
> > > > +     # clang hugely expands stack usage with -fsanitize=object-size
> > > > +     depends on !CC_IS_CLANG
> > > >       depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> > >
> > > Because of Clang implementation issues (see commit c637693b20da), this is
> > > already "default n" (and not supported under GCC at all). IIUC, setting
> > > this to "depends on !COMPILE_TEST" won't work for randconfigs, yes?
> >
> > Ah, I had not realized this is clang specific. Adding the !COMPILE_TEST
> > dependency would hide it for me, which may be good enough for me.
>
> I thought COMPILE_TEST does not get set for randconfig?

It does on my kernel, though I never submitted that patch ;-)

        Arnd

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Nathan Chancellor]

On Wed, Jan 06, 2021 at 11:06:39PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 6, 2021 at 10:38 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > On Wed, Jan 06, 2021 at 10:12:51AM +0100, Arnd Bergmann wrote:
> > > On Tue, Jan 5, 2021 at 10:25 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > > >
> > > > On Mon, Jan 4, 2021 at 11:33 PM Nathan Chancellor
> > > > <natechancellor@gmail.com> wrote:
> > > > > On Wed, Dec 30, 2020 at 05:13:03PM +0100, Marco Elver wrote:
> > > > > > On Wed, 30 Dec 2020 at 16:47, Arnd Bergmann <arnd@kernel.org> wrote:
> > > > > > >
> > > > > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > > > >
> > > > > > > Building ubsan kernels even for compile-testing introduced these
> > > > > > > warnings in my randconfig environment:
> > > > > > >
> > > > > > > crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> > > > > > > static void blake2b_compress(struct blake2b_state *S,
> > > > > > > crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> > > > > > > static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> > > > > > > lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180 bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> > > > > > > static noinline void fe_mul_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> > > > > > > lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588 bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> > > > > > > static noinline void fe_sqr_impl(u32 out[10], const u32 in1[10])
> > > > > > >
> > > > > > > Further testing showed that this is caused by
> > > > > > > -fsanitize=unsigned-integer-overflow.
> > > > > > >
> > > > > > > The one in blake2b immediately overflows the 8KB stack area on 32-bit
> > > > > > > architectures, so better ensure this never happens by making this
> > > > > > > option gcc-only.
> > > > >
> > > > > This patch also fixes the failed BUILD_BUG issue in mm/mremap.c that you
> > > > > sent a patch for [1], along with a couple of other issues I see such as:
> > > >
> > > > I'm fairly sure I still saw that BUILD_BUG() even after I had applied this
> > > > patch, I would guess that one just depends on inlining decisions that
> > > > are influenced by all kinds of compiler options including
> > > > -fsanitize=unsigned-integer-overflow, so it becomes less likely.
> > > >
> > > > I'll revert my other patch in the randconfig tree to see if it comes back.
> > >
> > > The qcom/gpi.c failure still happens with this patch applied:
> > >
> > > In file included from /git/arm-soc/drivers/dma/qcom/gpi.c:8:
> > > In function 'field_multiplier',
> > >     inlined from 'gpi_update_reg' at
> > > /git/arm-soc/include/linux/bitfield.h:124:17:
> > > /git/arm-soc/include/linux/bitfield.h:119:3: error: call to
> > > '__bad_mask' declared with attribute error: bad bitfield mask
> > >   119 |   __bad_mask();
> > >       |   ^~~~~~~~~~~~
> > > In function 'field_multiplier',
> > >     inlined from 'gpi_update_reg' at
> > > /git/arm-soc/include/linux/bitfield.h:154:1:
> > > /git/arm-soc/include/linux/bitfield.h:119:3: error: call to
> > > '__bad_mask' declared with attribute error: bad bitfield mask
> > >   119 |   __bad_mask();
> > >       |   ^~~~~~~~~~~~
> > >
> > > See https://pastebin.com/8UH6x4A2 for the .config
> > >
> > >        Arnd
> >
> > That config does not build for me, am I holding it wrong?
> >
> > $ curl -LSso .config https://pastebin.com/raw/8UH6x4A2
>
> Sorry about that, you ran into a bug that I have applied a
> local fix for. You could enable CONFIG_EPOLL, or apply
> this patch:
>
> https://lore.kernel.org/linux-arm-kernel/20200429132349.1294904-1-arnd@arndb.de/
>
>       Arnd

I decided to just try to build the file directly. As it turns out,
ARCH=arm allyesconfig does not show any error in get_extent or
gpi_update_reg but ARCH=arm64 allyesconfig does. Looks like this is
because of the lack of CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL for ARCH=arm.

$ rg UBSAN out/{arm,arm64}/.config
out/arm64/.config
13853:CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y
13854:CONFIG_UBSAN=y
13855:CONFIG_CC_HAS_UBSAN_BOUNDS=y
13856:CONFIG_CC_HAS_UBSAN_ARRAY_BOUNDS=y
13857:CONFIG_UBSAN_BOUNDS=y
13858:CONFIG_UBSAN_ARRAY_BOUNDS=y
13859:CONFIG_UBSAN_SHIFT=y
13860:CONFIG_UBSAN_DIV_ZERO=y
13861:CONFIG_UBSAN_UNREACHABLE=y
13862:CONFIG_UBSAN_SIGNED_OVERFLOW=y
13863:CONFIG_UBSAN_UNSIGNED_OVERFLOW=y
13864:CONFIG_UBSAN_OBJECT_SIZE=y
13865:CONFIG_UBSAN_BOOL=y
13866:CONFIG_UBSAN_ENUM=y
13867:CONFIG_UBSAN_SANITIZE_ALL=y
13868:CONFIG_TEST_UBSAN=m

out/arm/.config
14133:CONFIG_UBSAN=y
14134:CONFIG_CC_HAS_UBSAN_BOUNDS=y
14135:CONFIG_CC_HAS_UBSAN_ARRAY_BOUNDS=y
14136:CONFIG_UBSAN_BOUNDS=y
14137:CONFIG_UBSAN_ARRAY_BOUNDS=y
14138:CONFIG_UBSAN_SHIFT=y
14139:CONFIG_UBSAN_DIV_ZERO=y
14140:CONFIG_UBSAN_UNREACHABLE=y
14141:CONFIG_UBSAN_SIGNED_OVERFLOW=y
14142:CONFIG_UBSAN_UNSIGNED_OVERFLOW=y
14143:CONFIG_UBSAN_OBJECT_SIZE=y
14144:CONFIG_UBSAN_BOOL=y
14145:CONFIG_UBSAN_ENUM=y
14146:CONFIG_TEST_UBSAN=m

$ llvm-nm -S out/arm64/drivers/dma/qcom/gpi.o |& rg __bad_mask
                                  U __bad_mask

$ llvm-nm -S out/arm/drivers/dma/qcom/gpi.o |& rg __bad_mask

Applying this diff causes __bad_mask to show up:

diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile
index 50f1e7014693..c261adb47960 100644
--- a/drivers/dma/qcom/Makefile
+++ b/drivers/dma/qcom/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_QCOM_ADM) += qcom_adm.o
 obj-$(CONFIG_QCOM_BAM_DMA) += bam_dma.o
 obj-$(CONFIG_QCOM_GPI_DMA) += gpi.o
+UBSAN_SANITIZE_gpi.o := y
 obj-$(CONFIG_QCOM_HIDMA_MGMT) += hdma_mgmt.o
 hdma_mgmt-objs  := hidma_mgmt.o hidma_mgmt_sys.o
 obj-$(CONFIG_QCOM_HIDMA) +=  hdma.o

$ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- LLVM=1 \
       O=out/arm distclean allyesconfig drivers/dma/qcom/gpi.o

$ llvm-nm -S out/arm/drivers/dma/qcom/gpi.o |& rg __bad_mask
                  U __bad_mask

So I guess I am curious how you saw this pop up. Not to mention it seems
like that error is with GCC rather than clang?

For what it's worth, when I run that .config through olddefconfig,
CONFIG_ARCH_QCOM and CONFIG_QCOM_GPI_DMA get disabled... Am I doing
something wrong?

$ curl -LSso /tmp/out/arm/.config https://pastebin.com/raw/8UH6x4A2

$ rg "CONFIG_ARCH_QCOM|CONFIG_QCOM_GPI_DMA" /tmp/out/arm/.config
377:CONFIG_ARCH_QCOM=y
4437:CONFIG_QCOM_GPI_DMA=y

$ make.sh ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- LLVM=1
O=/tmp/out/arm olddefconfig
.config:364:warning: override: ARCH_DOVE changes choice state

$ rg "CONFIG_ARCH_QCOM|CONFIG_QCOM_GPI_DMA" /tmp/out/arm/.config

Cheers,
Nathan

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Arnd Bergmann]

On Wed, Jan 6, 2021 at 11:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Wed, Jan 6, 2021 at 10:57 PM Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Dec 30, 2020 at 04:47:35PM +0100, Arnd Bergmann wrote:
> > > diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> > > index 8b635fd75fe4..e23873282ba7 100644
> > > --- a/lib/Kconfig.ubsan
> > > +++ b/lib/Kconfig.ubsan
> > > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> > >
> > >  config UBSAN_UNSIGNED_OVERFLOW
> > >       bool "Perform checking for unsigned arithmetic overflow"
> > > +     # clang hugely expands stack usage with -fsanitize=object-size
> > > +     depends on !CC_IS_CLANG
> > >       depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> >
> > Because of Clang implementation issues (see commit c637693b20da), this is
> > already "default n" (and not supported under GCC at all). IIUC, setting
> > this to "depends on !COMPILE_TEST" won't work for randconfigs, yes?
>
> Ah, I had not realized this is clang specific. Adding the !COMPILE_TEST
> dependency would hide it for me, which may be good enough for me.
>
> > Is there some better way to mark this as "known to have issues, please
> > don't include in randconfig?"
> >
> > I'd like to keep it around so people can continue to work out the
> > problems with it, but not have unexpecting folks trip over it. ;)
>
> I've reverted that patch locally now and default-enabled for randconfigs.
> Now that I have an otherwise clean build, this should provide me
> with a full set of files that produce the warning. If the number is
> small enough, I could try opening individual github issues.

A day's worth of randconfig builds with clang-11 or clang-12 shows these
instances that exceeded the warning limit:

crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes
in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes
in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180
bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588
bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
fs/btrfs/scrub.c:3028:31: error: stack frame size of 1132 bytes in
function 'scrub_stripe' [-Werror,-Wframe-larger-than=]
drivers/net/ethernet/intel/e1000/e1000_main.c:3590:6: warning: stack
frame size of 1100 bytes in function 'e1000_update_stats'
[-Wframe-larger-than=]
drivers/net/ethernet/broadcom/tg3.c:11829:13: warning: stack frame
size of 1188 bytes in function 'tg3_get_estats' [-Wframe-larger-than=]
drivers/net/ethernet/intel/igb/igb_main.c:6551:6: warning: stack frame
size of 1348 bytes in function 'igb_update_stats'
[-Wframe-larger-than=]
drivers/net/ethernet/intel/igc/igc_main.c:3608:6: warning: stack frame
size of 1124 bytes in function 'igc_update_stats'
[-Wframe-larger-than=]
drivers/net/ethernet/qlogic/qed/qed_l2.c:1759:1: warning: stack frame
size of 1300 bytes in function '__qed_get_vport_port_stats'
[-Wframe-larger-than=]
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:7022:6: warning: stack
frame size of 1564 bytes in function 'ixgbe_update_stats'
[-Wframe-larger-than=]
drivers/net/ethernet/intel/ixgb/ixgb_main.c:1590:1: warning: stack
frame size of 1140 bytes in function 'ixgb_update_stats'
[-Wframe-larger-than=]
drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c:145:6: warning:
stack frame size of 1068 bytes in function 'mlx5i_get_stats'
[-Wframe-larger-than=]
drivers/staging/media/atomisp/pci/atomisp_cmd.c:5600:5: warning: stack
frame size of 1052 bytes in function 'atomisp_set_fmt'
[-Wframe-larger-than=]

All of these *only* happen on 32-bit x86, and can be reproduced in a
i386_defconfig, with the corresponding drivers (btrfs, sha512, blake2b,
curve25519, and the ethernet ones) and UBSAN_UNSIGNED_OVERFLOW
manually enabled. Given that few people still care about i386, maybe
we could just make the option depend on !CONFIG_X86_32

That config also runs into two more BUILD_BUG_ON() that I had not
seen in randconfig tests:

(i386 defconfig plus ubsan)
ld.lld: error: undefined symbol: __compiletime_assert_207
>>> referenced by cpu_entry_area.c
>>>               mm/cpu_entry_area.o:(setup_cpu_entry_area_ptes) in archive arch/x86/built-in.a

(x86-64 defconfig plus ubsan)
ld.lld: error: undefined symbol: __compiletime_assert_362
>>> referenced by efi_64.c
>>>               platform/efi/efi_64.o:(efi_sync_low_kernel_mappings) in archive arch/x86/built-in.a

      Arnd

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Kees Cook]

On Thu, Jan 07, 2021 at 05:09:59PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 6, 2021 at 11:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > On Wed, Jan 6, 2021 at 10:57 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Wed, Dec 30, 2020 at 04:47:35PM +0100, Arnd Bergmann wrote:
> > > > diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> > > > index 8b635fd75fe4..e23873282ba7 100644
> > > > --- a/lib/Kconfig.ubsan
> > > > +++ b/lib/Kconfig.ubsan
> > > > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> > > >
> > > >  config UBSAN_UNSIGNED_OVERFLOW
> > > >       bool "Perform checking for unsigned arithmetic overflow"
> > > > +     # clang hugely expands stack usage with -fsanitize=object-size
> > > > +     depends on !CC_IS_CLANG
> > > >       depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> > >
> > > Because of Clang implementation issues (see commit c637693b20da), this is
> > > already "default n" (and not supported under GCC at all). IIUC, setting
> > > this to "depends on !COMPILE_TEST" won't work for randconfigs, yes?
> >
> > Ah, I had not realized this is clang specific. Adding the !COMPILE_TEST
> > dependency would hide it for me, which may be good enough for me.
> >
> > > Is there some better way to mark this as "known to have issues, please
> > > don't include in randconfig?"
> > >
> > > I'd like to keep it around so people can continue to work out the
> > > problems with it, but not have unexpecting folks trip over it. ;)
> >
> > I've reverted that patch locally now and default-enabled for randconfigs.
> > Now that I have an otherwise clean build, this should provide me
> > with a full set of files that produce the warning. If the number is
> > small enough, I could try opening individual github issues.
> 
> A day's worth of randconfig builds with clang-11 or clang-12 shows these
> instances that exceeded the warning limit:
> 
> crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes
> in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes
> in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180
> bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588
> bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> fs/btrfs/scrub.c:3028:31: error: stack frame size of 1132 bytes in
> function 'scrub_stripe' [-Werror,-Wframe-larger-than=]
> drivers/net/ethernet/intel/e1000/e1000_main.c:3590:6: warning: stack
> frame size of 1100 bytes in function 'e1000_update_stats'
> [-Wframe-larger-than=]
> drivers/net/ethernet/broadcom/tg3.c:11829:13: warning: stack frame
> size of 1188 bytes in function 'tg3_get_estats' [-Wframe-larger-than=]
> drivers/net/ethernet/intel/igb/igb_main.c:6551:6: warning: stack frame
> size of 1348 bytes in function 'igb_update_stats'
> [-Wframe-larger-than=]
> drivers/net/ethernet/intel/igc/igc_main.c:3608:6: warning: stack frame
> size of 1124 bytes in function 'igc_update_stats'
> [-Wframe-larger-than=]
> drivers/net/ethernet/qlogic/qed/qed_l2.c:1759:1: warning: stack frame
> size of 1300 bytes in function '__qed_get_vport_port_stats'
> [-Wframe-larger-than=]
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:7022:6: warning: stack
> frame size of 1564 bytes in function 'ixgbe_update_stats'
> [-Wframe-larger-than=]
> drivers/net/ethernet/intel/ixgb/ixgb_main.c:1590:1: warning: stack
> frame size of 1140 bytes in function 'ixgb_update_stats'
> [-Wframe-larger-than=]
> drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c:145:6: warning:
> stack frame size of 1068 bytes in function 'mlx5i_get_stats'
> [-Wframe-larger-than=]
> drivers/staging/media/atomisp/pci/atomisp_cmd.c:5600:5: warning: stack
> frame size of 1052 bytes in function 'atomisp_set_fmt'
> [-Wframe-larger-than=]
> 
> All of these *only* happen on 32-bit x86, and can be reproduced in a
> i386_defconfig, with the corresponding drivers (btrfs, sha512, blake2b,
> curve25519, and the ethernet ones) and UBSAN_UNSIGNED_OVERFLOW
> manually enabled. Given that few people still care about i386, maybe
> we could just make the option depend on !CONFIG_X86_32

I'm fine with that -- or maybe any 32-bit architecture, if the problem
is poor stack space optimization on 32-bit archs?

> 
> That config also runs into two more BUILD_BUG_ON() that I had not
> seen in randconfig tests:
> 
> (i386 defconfig plus ubsan)
> ld.lld: error: undefined symbol: __compiletime_assert_207
> >>> referenced by cpu_entry_area.c
> >>>               mm/cpu_entry_area.o:(setup_cpu_entry_area_ptes) in archive arch/x86/built-in.a

That one I don't think I've seen before.

> 
> (x86-64 defconfig plus ubsan)
> ld.lld: error: undefined symbol: __compiletime_assert_362
> >>> referenced by efi_64.c
> >>>               platform/efi/efi_64.o:(efi_sync_low_kernel_mappings) in archive arch/x86/built-in.a

I think this is:
https://github.com/ClangBuiltLinux/linux/issues/256
and that bug needs re-opening? Or maybe there's a new bug I can't find?

-- 
Kees Cook

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Nathan Chancellor]

On Thu, Jan 07, 2021 at 09:22:00AM -0800, Kees Cook wrote:
> On Thu, Jan 07, 2021 at 05:09:59PM +0100, Arnd Bergmann wrote:
> > On Wed, Jan 6, 2021 at 11:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > > On Wed, Jan 6, 2021 at 10:57 PM Kees Cook <keescook@chromium.org> wrote:
> > > > On Wed, Dec 30, 2020 at 04:47:35PM +0100, Arnd Bergmann wrote:
> > > > > diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> > > > > index 8b635fd75fe4..e23873282ba7 100644
> > > > > --- a/lib/Kconfig.ubsan
> > > > > +++ b/lib/Kconfig.ubsan
> > > > > @@ -122,6 +122,8 @@ config UBSAN_SIGNED_OVERFLOW
> > > > >
> > > > >  config UBSAN_UNSIGNED_OVERFLOW
> > > > >       bool "Perform checking for unsigned arithmetic overflow"
> > > > > +     # clang hugely expands stack usage with -fsanitize=object-size
> > > > > +     depends on !CC_IS_CLANG
> > > > >       depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> > > >
> > > > Because of Clang implementation issues (see commit c637693b20da), this is
> > > > already "default n" (and not supported under GCC at all). IIUC, setting
> > > > this to "depends on !COMPILE_TEST" won't work for randconfigs, yes?
> > >
> > > Ah, I had not realized this is clang specific. Adding the !COMPILE_TEST
> > > dependency would hide it for me, which may be good enough for me.
> > >
> > > > Is there some better way to mark this as "known to have issues, please
> > > > don't include in randconfig?"
> > > >
> > > > I'd like to keep it around so people can continue to work out the
> > > > problems with it, but not have unexpecting folks trip over it. ;)
> > >
> > > I've reverted that patch locally now and default-enabled for randconfigs.
> > > Now that I have an otherwise clean build, this should provide me
> > > with a full set of files that produce the warning. If the number is
> > > small enough, I could try opening individual github issues.
> > 
> > A day's worth of randconfig builds with clang-11 or clang-12 shows these
> > instances that exceeded the warning limit:
> > 
> > crypto/blake2b_generic.c:98:13: error: stack frame size of 9636 bytes
> > in function 'blake2b_compress' [-Werror,-Wframe-larger-than=]
> > crypto/sha512_generic.c:151:13: error: stack frame size of 1292 bytes
> > in function 'sha512_generic_block_fn' [-Werror,-Wframe-larger-than=]
> > lib/crypto/curve25519-fiat32.c:312:22: error: stack frame size of 2180
> > bytes in function 'fe_mul_impl' [-Werror,-Wframe-larger-than=]
> > lib/crypto/curve25519-fiat32.c:444:22: error: stack frame size of 1588
> > bytes in function 'fe_sqr_impl' [-Werror,-Wframe-larger-than=]
> > fs/btrfs/scrub.c:3028:31: error: stack frame size of 1132 bytes in
> > function 'scrub_stripe' [-Werror,-Wframe-larger-than=]
> > drivers/net/ethernet/intel/e1000/e1000_main.c:3590:6: warning: stack
> > frame size of 1100 bytes in function 'e1000_update_stats'
> > [-Wframe-larger-than=]
> > drivers/net/ethernet/broadcom/tg3.c:11829:13: warning: stack frame
> > size of 1188 bytes in function 'tg3_get_estats' [-Wframe-larger-than=]
> > drivers/net/ethernet/intel/igb/igb_main.c:6551:6: warning: stack frame
> > size of 1348 bytes in function 'igb_update_stats'
> > [-Wframe-larger-than=]
> > drivers/net/ethernet/intel/igc/igc_main.c:3608:6: warning: stack frame
> > size of 1124 bytes in function 'igc_update_stats'
> > [-Wframe-larger-than=]
> > drivers/net/ethernet/qlogic/qed/qed_l2.c:1759:1: warning: stack frame
> > size of 1300 bytes in function '__qed_get_vport_port_stats'
> > [-Wframe-larger-than=]
> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:7022:6: warning: stack
> > frame size of 1564 bytes in function 'ixgbe_update_stats'
> > [-Wframe-larger-than=]
> > drivers/net/ethernet/intel/ixgb/ixgb_main.c:1590:1: warning: stack
> > frame size of 1140 bytes in function 'ixgb_update_stats'
> > [-Wframe-larger-than=]
> > drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c:145:6: warning:
> > stack frame size of 1068 bytes in function 'mlx5i_get_stats'
> > [-Wframe-larger-than=]
> > drivers/staging/media/atomisp/pci/atomisp_cmd.c:5600:5: warning: stack
> > frame size of 1052 bytes in function 'atomisp_set_fmt'
> > [-Wframe-larger-than=]
> > 
> > All of these *only* happen on 32-bit x86, and can be reproduced in a
> > i386_defconfig, with the corresponding drivers (btrfs, sha512, blake2b,
> > curve25519, and the ethernet ones) and UBSAN_UNSIGNED_OVERFLOW
> > manually enabled. Given that few people still care about i386, maybe
> > we could just make the option depend on !CONFIG_X86_32
> 
> I'm fine with that -- or maybe any 32-bit architecture, if the problem
> is poor stack space optimization on 32-bit archs?
> 
> > 
> > That config also runs into two more BUILD_BUG_ON() that I had not
> > seen in randconfig tests:
> > 
> > (i386 defconfig plus ubsan)
> > ld.lld: error: undefined symbol: __compiletime_assert_207
> > >>> referenced by cpu_entry_area.c
> > >>>               mm/cpu_entry_area.o:(setup_cpu_entry_area_ptes) in archive arch/x86/built-in.a
> 
> That one I don't think I've seen before.
> 
> > 
> > (x86-64 defconfig plus ubsan)
> > ld.lld: error: undefined symbol: __compiletime_assert_362
> > >>> referenced by efi_64.c
> > >>>               platform/efi/efi_64.o:(efi_sync_low_kernel_mappings) in archive arch/x86/built-in.a
> 
> I think this is:
> https://github.com/ClangBuiltLinux/linux/issues/256
> and that bug needs re-opening? Or maybe there's a new bug I can't find?

The problem is that applying the fix for that issue does not work, nor
does converting p4d_index to a macro like mips and s390. I am not sure
what exactly is going on there, it appears that clang cannot reason
about ptrs_per_p4d because it is an extern variable with no assigned
value in its translation unit?

Cheers,
Nathan

Re: [PATCH] ubsan: disable unsigned-integer-overflow sanitizer with clang

[Arnd Bergmann]

On Thu, Jan 7, 2021 at 7:15 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> On Thu, Jan 07, 2021 at 09:22:00AM -0800, Kees Cook wrote:

> > I think this is:
> > https://github.com/ClangBuiltLinux/linux/issues/256
> > and that bug needs re-opening? Or maybe there's a new bug I can't find?
>
> The problem is that applying the fix for that issue does not work, nor
> does converting p4d_index to a macro like mips and s390. I am not sure
> what exactly is going on there, it appears that clang cannot reason
> about ptrs_per_p4d because it is an extern variable with no assigned
> value in its translation unit?

Right, I tried the __always_inline trick already and concluded the same.

       Arnd