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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com>
Date:   Thu, 27 Aug 2020 12:02:12 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        Ard Biesheuvel <ardb@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        kernel test robot <lkp@...el.com>,
        Peter Oberparleiter <oberpar@...ux.ibm.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        kbuild-all@...ts.01.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>
Subject: Re: lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes
 is larger than 1024 bytes

On Thu, Aug 27, 2020 at 11:42 AM Kees Cook <keescook@...omium.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