[Bug c/94722] New: implement __attribute__((no_stack_protector)) function attribute

[Bug c/94722] New: implement __attribute__((no_stack_protector)) function attribute

[ndesaulniers at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

            Bug ID: 94722
           Summary: implement __attribute__((no_stack_protector)) function
                    attribute
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ndesaulniers at google dot com
                CC: jakub at redhat dot com, mliska at suse dot cz
  Target Milestone: ---

There's a couple of places in the Linux kernel where the placement of stack
protector guards causes problems for functions that do some tricky things. 
We'd like to have the ability to keep -fstack-protector* protections throughout
the kernel, but have finer grain resolution to disable the placement and
checking of stack guards on a per function granularity.

clang-8 added support for the function attribute no_stack_protector.
https://clang.llvm.org/docs/AttributeReference.html#no-stack-protector

With this feature implemented, we could have a more portable solution for the
kernel.

Two examples in the kernel where we could make use of this are
[0] https://lore.kernel.org/lkml/20200422192113.GG26846@zn.tnic/T/#t (call to
cpu_startup_entry() in start_secondary() in arch/x86/kernel/smpboot.c after
calls to boot_init_stack_canary() which has modified the stack guard).
[1]
https://lore.kernel.org/lkml/20180621162324.36656-1-ndesaulniers@google.com/
(custom calling conventions)

[0] was worked around with an empty asm statement and a descriptive comment.
[1] was worked around with `extern inline` with gnu_inline semantics.

I would prefer to use a function attribute for both cases.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

--- Comment #1 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
Note that glibc currently uses __attribute__ ((__optimize__ 
("-fno-stack-protector"))) (via a macro called inhibit_stack_protector) 
for this.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[ndesaulniers at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

--- Comment #2 from Nick Desaulniers <ndesaulniers at google dot com> ---
Also note this post in the thread [0]:
https://lore.kernel.org/lkml/20200422192113.GG26846@zn.tnic/T/#m88641fe74bdffe7beaa925dfe2d8146a08a47bac,
also
https://lore.kernel.org/lkml/20200422192113.GG26846@zn.tnic/T/#m691ff07f537102f1f5f63d4b65405b44f15956cc

"As we determined upthread (and the reason why we even still have this 
thread): the optimize attribute (and pragma) reset flags from the command 
line (the case in point was -fno-omit-frame-pointer).  So, that's not a
solution for now."

"As you will discover upthread that was tried with GCC and found 
insufficient, as GCC is a bit surprising with optimize attributes: it 
resets every -f option from the command line and applies only the ones 
from the attributes.  Including a potential -fno-omit-frame-pointer, 
causing all kinds of itches :)"

We have the ability to work around differences in attribute identifiers easily
in the Linux kernel, so the difference in naming while suboptimal, isn't
painful for the Linux kernel in practice.  Though I will buy another beer for
the developer if it happens to match Clang's naming. :)

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[ndesaulniers at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

--- Comment #3 from Nick Desaulniers <ndesaulniers at google dot com> ---
Ah, sorry, just was sent:
https://lore.kernel.org/lkml/20200316180303.GR2156@tucnak/ which is also
relevant regarding naming.  Still happy to buy the beers though.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
                 CC|                            |marxin at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |marxin at gcc dot gnu.org
   Last reconfirmed|                            |2020-04-23
     Ever confirmed|0                           |1
            Version|unknown                     |10.0
   Target Milestone|---                         |11.0

--- Comment #4 from Martin Liška <marxin at gcc dot gnu.org> ---
I can work on that for GCC 11.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:346b302d09c1e6db56d9fe69048acb32fbb97845

commit r11-4218-g346b302d09c1e6db56d9fe69048acb32fbb97845
Author: Martin Liska <mliska@suse.cz>
Date:   Fri May 15 14:42:12 2020 +0200

    Implement no_stack_protector attribute.

    gcc/ChangeLog:

    2020-05-18  Martin Liska  <mliska@suse.cz>

            PR c/94722
            * cfgexpand.c (stack_protect_decl_phase):
            Guard with lookup_attribute("no_stack_protector") at
            various places.
            (expand_used_vars): Likewise here.
            * doc/extend.texi: Document no_stack_protector attribute.

    gcc/ada/ChangeLog:

    2020-05-18  Martin Liska  <mliska@suse.cz>

            PR c/94722
            * gcc-interface/utils.c (handle_no_stack_protect_attribute):
            New.
            (handle_stack_protect_attribute): Add error message for a
            no_stack_protector function.

    gcc/c-family/ChangeLog:

    2020-05-18  Martin Liska  <mliska@suse.cz>

            PR c/94722
            * c-attribs.c (handle_no_stack_protect_function_attribute): New.
            (handle_stack_protect_attribute): Add error message for a
            no_stack_protector function.

    gcc/testsuite/ChangeLog:

    2020-05-18  Martin Liska  <mliska@suse.cz>

            PR c/94722
            * g++.dg/no-stack-protector-attr-2.C: New test.
            * g++.dg/no-stack-protector-attr-3.C: New test.
            * g++.dg/no-stack-protector-attr.C: New test.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #6 from Martin Liška <marxin at gcc dot gnu.org> ---
Implemented.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[i at maskray dot me]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

Fangrui Song <i at maskray dot me> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |i at maskray dot me

--- Comment #7 from Fangrui Song <i at maskray dot me> ---
(In reply to Martin Liška from comment #6)
> Implemented.

#include <string.h>
void foo(const char *a) { char b[34]; strcpy(b, a); }
__attribute__((no_stack_protector))
void bar(const char *a) { foo(a); }


#include <string.h>
__attribute__((no_stack_protector))
void foo(const char *a) { char b[34]; strcpy(b, a); }
void bar(const char *a) { foo(a); }

In both cases, foo can be inlined.

In Clang, the recent resolution https://reviews.llvm.org/D91816 is that a ssp
function cannot be inlined into a nossp function and a nossp function cannot be
inlined into a ssp function.

I think one argument for the no-inline behavior is that ssp conveys the
security enforcement intention and the GCC behavior may degrade the security
hardening while inlining a ssp chunk.

Previously Clang upgraded the caller from nossp to ssp after inlining. However,
that behavior caused
https://lore.kernel.org/lkml/20200422192113.GG26846@zn.tnic/T/#t
(the caller may not have set up %gs and upgrading it to ssp can break it)

The new Clang behavior also disallows a nossp callee from being inlined into a
ssp caller. That makes the rules easier to explain but I haven't thought very
clearly about the implications though.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

--- Comment #8 from Martin Liška <marxin at gcc dot gnu.org> ---
I guess Jakub can chime in. He was actively involved in Linux kernel
discussion.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I've said in that thread that I don't really like disabling the inlining, if we
wanted to make sure everything is stack protected, we'd need to disable all the
inlining no matter whether the attribute is there or not, because inlining by
definition removes the stack protector checks, it is only tested on function
boundaries, not on inline function boundaries.
The user has the option to add noinline when he wants.

[Bug c/94722] implement __attribute__((no_stack_protector)) function attribute

[ndesaulniers at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

--- Comment #10 from Nick Desaulniers <ndesaulniers at google dot com> ---
(In reply to Jakub Jelinek from comment #9)
> I've said in that thread that I don't really like disabling the inlining, if
> we wanted to make sure everything is stack protected, we'd need to disable
> all the inlining no matter whether the attribute is there or not, because
> inlining by definition removes the stack protector checks, it is only tested
> on function boundaries, not on inline function boundaries.
> The user has the option to add noinline when he wants.

I'm not against reverting my recent change to LLVM's inlining behavior
(https://reviews.llvm.org/D91816, and the follow up to the docs
https://reviews.llvm.org/D93422).  Would be less work to do for inlining
decisions.

In the kernel, we could introduce a macro that's "noinline_for_lto" or
"noinline_due_to_stackprotectors" or such; this specific case only comes about
due to LTO between TUs with -fstack-protector* and -fno-stack-protector and
inlining between them.

Though, I'm still concerned about local cases (forgetting LTO for a moment)
where folks use __attribute__((no_stack_protector)) will basically have to
remember to additionally use __attribute__((noinline)) lest they hit the same
kind of bugs within a TU.  LLVM's current behavior "helps" developers avoid
such pitfalls; otherwise developers will have to be able to diagnose on their
own when such a case arises such that it causes bugs.