Bug 94722

Summary: implement __attribute__((no_stack_protector)) function attribute
Product: gcc Reporter: Nick Desaulniers <ndesaulniers>
Component: cAssignee: Martin Liška <marxin>
Status: RESOLVED FIXED    
Severity: normal CC: i, jakub, jakub, llozano, marxin, matz, pageexec
Priority: P3 Keywords: patch
Version: 10.0   
Target Milestone: 11.0   
URL: https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545916.html
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2020-04-23 00:00:00

Description Nick Desaulniers 2020-04-22 21:03:40 UTC
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.
Comment 1 joseph@codesourcery.com 2020-04-22 21:16:52 UTC
Note that glibc currently uses __attribute__ ((__optimize__ 
("-fno-stack-protector"))) (via a macro called inhibit_stack_protector) 
for this.
Comment 2 Nick Desaulniers 2020-04-22 21:28:47 UTC
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. :)
Comment 3 Nick Desaulniers 2020-04-22 21:30:21 UTC
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.
Comment 4 Martin Liška 2020-04-23 05:31:39 UTC
I can work on that for GCC 11.
Comment 5 CVS Commits 2020-10-22 08:11:43 UTC
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.
Comment 6 Martin Liška 2020-10-22 08:13:26 UTC
Implemented.
Comment 7 Fangrui Song 2020-12-17 05:34:46 UTC
(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.
Comment 8 Martin Liška 2020-12-17 08:37:11 UTC
I guess Jakub can chime in. He was actively involved in Linux kernel discussion.
Comment 9 Jakub Jelinek 2020-12-17 08:52:59 UTC
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.
Comment 10 Nick Desaulniers 2020-12-18 00:17:21 UTC
(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.