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

From: Arnd Bergmann
Date: Thu Aug 27 2020 - 04:33:40 EST


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