Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Herbert Xu]

On Thu, Aug 27, 2020 at 11:52:50AM +0800, kernel test robot wrote:
> 
> First bad commit (maybe != root cause):
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   15bc20c6af4ceee97a1f90b43c0e386643c071b4
> commit: 5fb8ef25803ef33e2eb60b626435828b937bed75 crypto: chacha - move existing library code into lib/crypto
> date:   9 months ago
> config: i386-randconfig-r015-20200827 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> reproduce (this is a W=1 build):
>         git checkout 5fb8ef25803ef33e2eb60b626435828b937bed75
>         # save the attached .config to linux build tree
>         make W=1 ARCH=i386 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>    lib/crypto/chacha.c: In function 'chacha_permute':
> >> lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>       65 | }
>          | ^

This doesn't happen with a normal configuration.  To recreate
this warning, you need to enable both GCOV_KERNEL and UBSAN.

This is the minimal gcc command-line to recreate it:

gcc -Wframe-larger-than=1024 -fprofile-arcs -fsanitize=object-size -c -O2 chacha.c

If you take away either profile-arcs or sanitize=object-size then
the problem goes away.

Any suggestions?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Ard Biesheuvel]

(+ Arnd)

On Thu, 27 Aug 2020 at 10:06, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Aug 27, 2020 at 11:52:50AM +0800, kernel test robot wrote:
> >
> > First bad commit (maybe != root cause):
> >
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   15bc20c6af4ceee97a1f90b43c0e386643c071b4
> > commit: 5fb8ef25803ef33e2eb60b626435828b937bed75 crypto: chacha - move existing library code into lib/crypto
> > date:   9 months ago
> > config: i386-randconfig-r015-20200827 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > reproduce (this is a W=1 build):
> >         git checkout 5fb8ef25803ef33e2eb60b626435828b937bed75
> >         # save the attached .config to linux build tree
> >         make W=1 ARCH=i386
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> >    lib/crypto/chacha.c: In function 'chacha_permute':
> > >> lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> >       65 | }
> >          | ^
>
> This doesn't happen with a normal configuration.  To recreate
> this warning, you need to enable both GCOV_KERNEL and UBSAN.
>
> This is the minimal gcc command-line to recreate it:
>
> gcc -Wframe-larger-than=1024 -fprofile-arcs -fsanitize=object-size -c -O2 chacha.c
>
> If you take away either profile-arcs or sanitize=object-size then
> the problem goes away.
>
> Any suggestions?
>

Is it really worth it to obsess about this? Special compiler
instrumentation simply leads to a larger stack footprint in many
cases, which is why we use a larger stack to begin with (at least we
do so for Kasan, so if we don't for Ubsan, we should consider it)

Past experience also shows that this is highly dependent on the exact
compiler version, so issues like these are often moving targets.

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Herbert Xu]

On Thu, Aug 27, 2020 at 10:10:19AM +0200, Ard Biesheuvel wrote:
>
> Is it really worth it to obsess about this? Special compiler
> instrumentation simply leads to a larger stack footprint in many
> cases, which is why we use a larger stack to begin with (at least we
> do so for Kasan, so if we don't for Ubsan, we should consider it)

Perhaps the stack frame warning should be disabled if both GCOV and
UBSAN are on.

> Past experience also shows that this is highly dependent on the exact
> compiler version, so issues like these are often moving targets.

Interestingly this particular file fails with those options on
gcc 8, 9 and 10.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Linus Torvalds]

On Thu, Aug 27, 2020 at 1:25 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> Interestingly this particular file fails with those options on
> gcc 8, 9 and 10.

How are you guys testing? I have UBSAN and GCOV on, and don't see
crazy frames on either i386 or x86-64.

I see 72 bytes and 64 bytes respectively for chacha_permute() (plus
the register pushes, which is about the same size)

                  Linus

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Linus Torvalds]

On Thu, Aug 27, 2020 at 10:34 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> How are you guys testing? I have UBSAN and GCOV on, and don't see
> crazy frames on either i386 or x86-64.

Oh, never mind. I also have COMPILE_TEST on, so it ends up disabling
GCOV_PROFILE_ALL and UBSAN_SANITIZE_ALL.

And yeah, this seems to be a gcc bug. It generates a ton of stack
slots for temporaries. It's -fsanitize=object-size that seems to do
it.

And "-fstack-reuse=all" doesn't seem to make any difference.

So I think

 (a) our stack size check is good to catch this

 (b) gcc and -fsanitize=object-size is basically an unusable combination

and it's not a bug in the kernel.

                Linus

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Kees Cook]

On Thu, Aug 27, 2020 at 10:55:32AM -0700, Linus Torvalds wrote:
> On Thu, Aug 27, 2020 at 10:34 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > How are you guys testing? I have UBSAN and GCOV on, and don't see
> > crazy frames on either i386 or x86-64.
> 
> Oh, never mind. I also have COMPILE_TEST on, so it ends up disabling
> GCOV_PROFILE_ALL and UBSAN_SANITIZE_ALL.
> 
> And yeah, this seems to be a gcc bug. It generates a ton of stack
> slots for temporaries. It's -fsanitize=object-size that seems to do
> it.
> 
> And "-fstack-reuse=all" doesn't seem to make any difference.
> 
> So I think
> 
>  (a) our stack size check is good to catch this
> 
>  (b) gcc and -fsanitize=object-size is basically an unusable combination
> 
> and it's not a bug in the kernel.

Do you mean you checked both gcc and clang and it was only a problem with gcc?
(If so, I can tweak the "depends" below...)

This should let us avoid it, I'm currently testing:

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index 774315de555a..24091315c251 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -47,6 +47,19 @@ config UBSAN_BOUNDS
 	  to the {str,mem}*cpy() family of functions (that is addressed
 	  by CONFIG_FORTIFY_SOURCE).
 
+config UBSAN_OBJECT_SIZE
+	bool "Check for accesses beyond known object sizes"
+	default UBSAN
+	depends on !COMPILE_TEST
+	help
+	  This option enables detection of cases where accesses may
+	  happen beyond the end of an object's size, which happens in
+	  places like invalid downcasts, or calling function pointers
+	  through invalid pointers.
+
+	  This uses much more stack space, and isn't recommended for
+	  cases were stack utilization depth is a concern.
+
 config UBSAN_MISC
 	bool "Enable all other Undefined Behavior sanity checks"
 	default UBSAN
diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
index 27348029b2b8..3ff67e9b17fd 100644
--- a/scripts/Makefile.ubsan
+++ b/scripts/Makefile.ubsan
@@ -7,12 +7,15 @@ ifdef CONFIG_UBSAN_BOUNDS
       CFLAGS_UBSAN += $(call cc-option, -fsanitize=bounds)
 endif
 
+ifdef CONFIG_UBSAN_OBJECT_SIZE
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=object-size)
+endif
+
 ifdef CONFIG_UBSAN_MISC
       CFLAGS_UBSAN += $(call cc-option, -fsanitize=shift)
       CFLAGS_UBSAN += $(call cc-option, -fsanitize=integer-divide-by-zero)
       CFLAGS_UBSAN += $(call cc-option, -fsanitize=unreachable)
       CFLAGS_UBSAN += $(call cc-option, -fsanitize=signed-integer-overflow)
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=object-size)
       CFLAGS_UBSAN += $(call cc-option, -fsanitize=bool)
       CFLAGS_UBSAN += $(call cc-option, -fsanitize=enum)
 endif

-- 
Kees Cook

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Linus Torvalds]

On Thu, Aug 27, 2020 at 11:42 AM Kees Cook <keescook@chromium.org> wrote:
>
> Do you mean you checked both gcc and clang and it was only a problem with gcc?

I didn't check with clang, but Arnd claimed it was fine.

> (If so, I can tweak the "depends" below...)

Ugh.

Instead of making the Makefile even uglier, why don't you just make
this all be done in the Kconfig.

Also, I'm not seeing the point of your patch. You didn't actually
change anything, you just made a new config variable with the same
semantics as the old one.

Add a

        depends on CLANG

or something, with a comment saying that it doesn't work on gcc due to
excessive stack use.

> +ifdef CONFIG_UBSAN_OBJECT_SIZE
> +      CFLAGS_UBSAN += $(call cc-option, -fsanitize=object-size)
> +endif

All of this should be thrown out, and this code should use the proper
patterns for configuration entries in the Makefile, ie just

  ubsan-cflags-$(CONFIG_UBSAN_OBJECT_SIZE) += -fsanitize=object-size

and the Kconfig file is the thing that should check if that CC option
exists with

  config UBSAN_OBJECT_SIZE
        bool "Check for accesses beyond known object sizes"
        default UBSAN
        depends on CLANG  # gcc makes a mess of it
        depends on $(cc-option,-fsanitize-coverage=trace-pc)

and the same goes for all the other cases too:

>  ifdef CONFIG_UBSAN_MISC
>        CFLAGS_UBSAN += $(call cc-option, -fsanitize=shift)
>        CFLAGS_UBSAN += $(call cc-option, -fsanitize=integer-divide-by-zero)
>        CFLAGS_UBSAN += $(call cc-option, -fsanitize=unreachable)
>        CFLAGS_UBSAN += $(call cc-option, -fsanitize=signed-integer-overflow)
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=object-size)
>        CFLAGS_UBSAN += $(call cc-option, -fsanitize=bool)
>        CFLAGS_UBSAN += $(call cc-option, -fsanitize=enum)
>  endif

and if you don't want to ask for them (which is a good idea), you keep that

    config UBSAN_MISC
        bool "Misc UBSAN.."

thing, and just make all of the above have the pattern of

    config UBSAN_OBJECT_SIZE
        def_bool UBSAN_MISC
        depends on CLANG  # gcc makes a mess of it
        depends on $(cc-option,-fsanitize-coverage=trace-pc)

which makes the Makefile much cleaner, and makes all our choices very
visible in the config file when they then get passed around.

We should basically strive for our Makefiles to have as little "ifdef"
etc magic as possible. We did the config work already, the Makefiles
should primarily just have those

   XYZ-$(CONFIG_OPTION) += abc

kind of lines (and then  you often end up having

  CFLAGS_UBSAN := $(ubsan-cflags-y)

at the end).

Doesn't that all look much cleaner?

                   Linus

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Kees Cook]

On Thu, Aug 27, 2020 at 12:02:12PM -0700, Linus Torvalds wrote:
> On Thu, Aug 27, 2020 at 11:42 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > Do you mean you checked both gcc and clang and it was only a problem with gcc?
> 
> I didn't check with clang, but Arnd claimed it was fine.
> 
> > (If so, I can tweak the "depends" below...)
> 
> Ugh.
> 
> Instead of making the Makefile even uglier, why don't you just make
> this all be done in the Kconfig.
> 
> Also, I'm not seeing the point of your patch. You didn't actually
> change anything, you just made a new config variable with the same
> semantics as the old one.

Hmm? Yeah it did: it disallowed CONFIG_COMPILE_TEST, which you said was
the missing piece, I thought? (It's hardly the first time COMPILE_TEST
has collided unhappily with *SAN-ish things.)

> All of this should be thrown out, and this code should use the proper
> patterns for configuration entries in the Makefile, ie just
> 
>   ubsan-cflags-$(CONFIG_UBSAN_OBJECT_SIZE) += -fsanitize=object-size

Yeah, that would be a better pattern for sure.

> and the Kconfig file is the thing that should check if that CC option
> exists with
> 
>   config UBSAN_OBJECT_SIZE
>         bool "Check for accesses beyond known object sizes"
>         default UBSAN
>         depends on CLANG  # gcc makes a mess of it
>         depends on $(cc-option,-fsanitize-coverage=trace-pc)

Yup, for sure. I've only recently started poking at the ubsan stuff. I
can clean it up better.

> Doesn't that all look much cleaner?

Yup!

-- 
Kees Cook

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Arnd Bergmann]

On Thu, Aug 27, 2020 at 7:55 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Aug 27, 2020 at 10:34 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > How are you guys testing? I have UBSAN and GCOV on, and don't see
> > crazy frames on either i386 or x86-64.
>
> Oh, never mind. I also have COMPILE_TEST on, so it ends up disabling
> GCOV_PROFILE_ALL and UBSAN_SANITIZE_ALL.

Ah right, that explains why I never saw the warning in my randconfig
build tests, I run those with COMPILE_TEST force-enabled.

       Arnd

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Kees Cook]

On Thu, Aug 27, 2020 at 09:11:52PM +0200, Arnd Bergmann wrote:
> On Thu, Aug 27, 2020 at 7:55 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Thu, Aug 27, 2020 at 10:34 AM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > How are you guys testing? I have UBSAN and GCOV on, and don't see
> > > crazy frames on either i386 or x86-64.
> >
> > Oh, never mind. I also have COMPILE_TEST on, so it ends up disabling
> > GCOV_PROFILE_ALL and UBSAN_SANITIZE_ALL.
> 
> Ah right, that explains why I never saw the warning in my randconfig
> build tests, I run those with COMPILE_TEST force-enabled.

Ah, I got this backwards. It's not COMPILE_TEST breaking it, it's
actually FIXING it. :P Anyway, I'll go clean this up more.

-- 
Kees Cook

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Linus Torvalds]

On Thu, Aug 27, 2020 at 12:12 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> Ah right, that explains why I never saw the warning in my randconfig
> build tests, I run those with COMPILE_TEST force-enabled.

.. but your clang test did enable this?

.. never mind, I have clang locally anyway, and while I usually don't
do the allmodconfig test there, I did it now with COMPILE_TEST
disabled.

clang does seem fine. It generates 136 bytes of stack-frame (plus
register saves), which is certainly not optimal, but it's not horribly
excessive.

Of course, I don't know if clang actually does the same as gcc with
-fsanitize=object-size and -fprofile-arcs, but whatever they do, they
were on for that clang build.

So yes, this does seem to be a gcc-only problem.

               Linus

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Arnd Bergmann]

On Thu, Aug 27, 2020 at 10:10 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> On Thu, 27 Aug 2020 at 10:06, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > On Thu, Aug 27, 2020 at 11:52:50AM +0800, kernel test robot wrote:
> > >
> > > First bad commit (maybe != root cause):
> > >
> > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head:   15bc20c6af4ceee97a1f90b43c0e386643c071b4
> > > commit: 5fb8ef25803ef33e2eb60b626435828b937bed75 crypto: chacha - move existing library code into lib/crypto
> > > date:   9 months ago
> > > config: i386-randconfig-r015-20200827 (attached as .config)
> > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > reproduce (this is a W=1 build):
> > >         git checkout 5fb8ef25803ef33e2eb60b626435828b937bed75
> > >         # save the attached .config to linux build tree
> > >         make W=1 ARCH=i386
> > >
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <lkp@intel.com>
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > >    lib/crypto/chacha.c: In function 'chacha_permute':
> > > >> lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > >       65 | }
> > >          | ^
> >
> > This doesn't happen with a normal configuration.  To recreate
> > this warning, you need to enable both GCOV_KERNEL and UBSAN.
> >
> > This is the minimal gcc command-line to recreate it:
> >
> > gcc -Wframe-larger-than=1024 -fprofile-arcs -fsanitize=object-size -c -O2 chacha.c
> >
> > If you take away either profile-arcs or sanitize=object-size then
> > the problem goes away.
> >
> > Any suggestions?
> >
>
> Is it really worth it to obsess about this? Special compiler
> instrumentation simply leads to a larger stack footprint in many
> cases, which is why we use a larger stack to begin with (at least we
> do so for Kasan, so if we don't for Ubsan, we should consider it)
>
> Past experience also shows that this is highly dependent on the exact
> compiler version, so issues like these are often moving targets.

Yes, I do think it is important to address these in some form,
for multiple reasons:

* With the limited amount of stack space in normal uninstrumented
  kernels, I do think it is vital to have a fairly low default warning
  limit in order to catch all cases that do something dangerously
  stupid, either because of code bugs or compiler bugs.

* I also think we do want the warning enabled in other configurations,
  in particular because the compiler tends to make increasingly stupid
  decisions when combining less common instrumentations, which
  again can lead to instant exploitable bugs, e.g. when a function's
  stack frame grows beyond the total stack size. In many cases the
  gcc and clang developers are both able to address these quickly
  when we send a good bug report (which unfortunately can be a lot of
  work).

* The crypto cipher code unfortunately is particularly prone to running
  into these issues because each new compiler version tries to
  find more clever tricks to optimize code that in turn implements
  an algorithm that intentionally tries to not have any clever shortcuts.
  In many cases the stack size warning that we see in some
  configurations is an indicator for the compiler running into a false
  optimization even on the non-instrumented code that leads to lower
  performance from unnecessary register spilling that should be
  avoided.

      Arnd

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Ard Biesheuvel]

On Thu, 27 Aug 2020 at 10:33, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Aug 27, 2020 at 10:10 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Thu, 27 Aug 2020 at 10:06, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >
> > > On Thu, Aug 27, 2020 at 11:52:50AM +0800, kernel test robot wrote:
> > > >
> > > > First bad commit (maybe != root cause):
> > > >
> > > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > head:   15bc20c6af4ceee97a1f90b43c0e386643c071b4
> > > > commit: 5fb8ef25803ef33e2eb60b626435828b937bed75 crypto: chacha - move existing library code into lib/crypto
> > > > date:   9 months ago
> > > > config: i386-randconfig-r015-20200827 (attached as .config)
> > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > > reproduce (this is a W=1 build):
> > > >         git checkout 5fb8ef25803ef33e2eb60b626435828b937bed75
> > > >         # save the attached .config to linux build tree
> > > >         make W=1 ARCH=i386
> > > >
> > > > If you fix the issue, kindly add following tag as appropriate
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > >
> > > > All warnings (new ones prefixed by >>):
> > > >
> > > >    lib/crypto/chacha.c: In function 'chacha_permute':
> > > > >> lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > > >       65 | }
> > > >          | ^
> > >
> > > This doesn't happen with a normal configuration.  To recreate
> > > this warning, you need to enable both GCOV_KERNEL and UBSAN.
> > >
> > > This is the minimal gcc command-line to recreate it:
> > >
> > > gcc -Wframe-larger-than=1024 -fprofile-arcs -fsanitize=object-size -c -O2 chacha.c
> > >
> > > If you take away either profile-arcs or sanitize=object-size then
> > > the problem goes away.
> > >
> > > Any suggestions?
> > >
> >
> > Is it really worth it to obsess about this? Special compiler
> > instrumentation simply leads to a larger stack footprint in many
> > cases, which is why we use a larger stack to begin with (at least we
> > do so for Kasan, so if we don't for Ubsan, we should consider it)
> >
> > Past experience also shows that this is highly dependent on the exact
> > compiler version, so issues like these are often moving targets.
>
> Yes, I do think it is important to address these in some form,
> for multiple reasons:
>
> * With the limited amount of stack space in normal uninstrumented
>   kernels, I do think it is vital to have a fairly low default warning
>   limit in order to catch all cases that do something dangerously
>   stupid, either because of code bugs or compiler bugs.
>
> * I also think we do want the warning enabled in other configurations,
>   in particular because the compiler tends to make increasingly stupid
>   decisions when combining less common instrumentations, which
>   again can lead to instant exploitable bugs, e.g. when a function's
>   stack frame grows beyond the total stack size. In many cases the
>   gcc and clang developers are both able to address these quickly
>   when we send a good bug report (which unfortunately can be a lot of
>   work).
>
> * The crypto cipher code unfortunately is particularly prone to running
>   into these issues because each new compiler version tries to
>   find more clever tricks to optimize code that in turn implements
>   an algorithm that intentionally tries to not have any clever shortcuts.
>   In many cases the stack size warning that we see in some
>   configurations is an indicator for the compiler running into a false
>   optimization even on the non-instrumented code that leads to lower
>   performance from unnecessary register spilling that should be
>   avoided.
>

In that case, I suppose we should simply disable instrumentation for
chacha_permute()? It is a straight-forward arithmetic transformation
on a u32[16] array, where ubsan has limited value afaict.

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Arnd Bergmann]

On Thu, Aug 27, 2020 at 10:42 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> In that case, I suppose we should simply disable instrumentation for
> chacha_permute()? It is a straight-forward arithmetic transformation
> on a u32[16] array, where ubsan has limited value afaict.

I guess that always works as a last resort, but shouldn't we first try
to figure out why ubsan even makes a difference and whether the
object code without ubsan looks like a reasonable representation
of the source form?

Since it really is a fairly simple transformation, I would have
expected the compiler to not emit any ubsan checks. If gcc
only gets confused about the fixed offsets possibly overflowing
the fixed-length array, maybe it helps to give it a little extra
information like (untested):

--- a/lib/crypto/chacha.c
+++ b/lib/crypto/chacha.c
@@ -13,7 +13,7 @@
 #include <asm/unaligned.h>
 #include <crypto/chacha.h>

-static void chacha_permute(u32 *x, int nrounds)
+static void chacha_permute(u32 x[16], int nrounds)
 {
        int i;

      Arnd

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Ard Biesheuvel]

On Thu, 27 Aug 2020 at 11:20, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Aug 27, 2020 at 10:42 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > In that case, I suppose we should simply disable instrumentation for
> > chacha_permute()? It is a straight-forward arithmetic transformation
> > on a u32[16] array, where ubsan has limited value afaict.
>
> I guess that always works as a last resort, but shouldn't we first try
> to figure out why ubsan even makes a difference and whether the
> object code without ubsan looks like a reasonable representation
> of the source form?
>
> Since it really is a fairly simple transformation, I would have
> expected the compiler to not emit any ubsan checks. If gcc
> only gets confused about the fixed offsets possibly overflowing
> the fixed-length array, maybe it helps to give it a little extra
> information like (untested):
>
> --- a/lib/crypto/chacha.c
> +++ b/lib/crypto/chacha.c
> @@ -13,7 +13,7 @@
>  #include <asm/unaligned.h>
>  #include <crypto/chacha.h>
>
> -static void chacha_permute(u32 *x, int nrounds)
> +static void chacha_permute(u32 x[16], int nrounds)
>  {
>         int i;
>

That does not help, unfortunately.

What does seem to work is

struct chacha_state { u32 x[16]; };

struct chacha_state chacha_permute(struct chacha_state st, int nrounds)
{
  struct chacha_state ret = st;
  u32 *x = ret.x;

  ...

  return st;
}

(and updating the caller accordingly, obviously)

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Herbert Xu]

On Thu, Aug 27, 2020 at 12:41:53PM +0200, Ard Biesheuvel wrote:
>
> That does not help, unfortunately.
> 
> What does seem to work is
> 
> struct chacha_state { u32 x[16]; };
> 
> struct chacha_state chacha_permute(struct chacha_state st, int nrounds)

Passing 64 bytes by value is not good.

Passing struct chacha_state as a pointer doesn't work either.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes

[Arnd Bergmann]

On Thu, Aug 27, 2020 at 1:51 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Aug 27, 2020 at 12:41:53PM +0200, Ard Biesheuvel wrote:
> >
> > That does not help, unfortunately.
> >
> > What does seem to work is
> >
> > struct chacha_state { u32 x[16]; };
> >
> > struct chacha_state chacha_permute(struct chacha_state st, int nrounds)
>
> Passing 64 bytes by value is not good.
>
> Passing struct chacha_state as a pointer doesn't work either.

Marking the function as __always_inline avoids the problem, as it
lets the compiler see the issue, but seems to produce somewhat
worse object code.

I also tested with clang-11, which supports both -fsanitize-bounds
and -fprofile-arcs but only needs 8 bytes of stack for this function.

One more data point, I looked at the actual object code and found
that neither -fprofile-arcs  nor -fsanitize-bounds has a noticeable
impact on the object code output by themselves (aside of not
leading to the warning as you already mentioned). I would
conclude that there is an actual problem with gcc here.

      Arnd