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@xxxxxxxxxx> wrote:
> On Thu, 27 Aug 2020 at 10:06, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > >
> > > 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