[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 Eric Gallager changed: What|Removed |Added CC||mpolacek at gcc dot gnu.org --- Comment #27 from Eric Gallager --- Marek Polacek has had a new idea to have it be enabled by the new -fhardened flag instead: https://gcc.gnu.org/pipermail/gcc-patches/2023-December/638987.html
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 Martin Uecker changed: What|Removed |Added CC||muecker at gwdg dot de --- Comment #26 from Martin Uecker --- I would be nice to have a way to avoid executable code while still being able to use this feature (which is useful!).
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 Sam James changed: What|Removed |Added CC||sjames at gcc dot gnu.org --- Comment #25 from Sam James --- fwiw Gentoo and Alpine have been doing this for many, many years
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #24 from Mark Wielaard --- (In reply to Eric Gallager from comment #23) > (In reply to Mark Wielaard from comment #22) > > (In reply to Eric Gallager from comment #21) > > > (In reply to Mark Wielaard from comment #20) > > > > https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02055.html > > > > > > Did this make it in? If not, have you pinged it lately? > > > > No, there was some review, I think we are generally good, but I am waiting > > for stage1 to open. > > Stage1 has opened. And it has opened again. So this comment is mostly for myself to not forget about this again (and again...)
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #23 from Eric Gallager --- (In reply to Mark Wielaard from comment #22) > (In reply to Eric Gallager from comment #21) > > (In reply to Mark Wielaard from comment #20) > > > https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02055.html > > > > Did this make it in? If not, have you pinged it lately? > > No, there was some review, I think we are generally good, but I am waiting > for stage1 to open. Stage1 has opened.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #22 from Mark Wielaard --- (In reply to Eric Gallager from comment #21) > (In reply to Mark Wielaard from comment #20) > > https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02055.html > > Did this make it in? If not, have you pinged it lately? No, there was some review, I think we are generally good, but I am waiting for stage1 to open.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #21 from Eric Gallager --- (In reply to Mark Wielaard from comment #20) > https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02055.html Did this make it in? If not, have you pinged it lately?
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #20 from Mark Wielaard --- https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02055.html
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #19 from Mark Wielaard --- I think we are just talking past each other because we don't fully agree when the warning should trigger and whether it is (trivial and/or) desirable to avoid that specific corner case. We do agree that nested functions are a useful C language extension and I am using them extensively in my own code. The only corner case I would avoid is when taking a nested function pointer escapes the current lexical context and when calling that function pointer would need access to some lexical context variables (because it will generate something that isn't a real simple function pointer and it is hard to reason about when calling it is safe). Which is precisely when -Wtrampolines triggers whenever that requires creating an executable trampoline on the stack. IMHO it is easy to rewrite any code that does that to pass around the bare function pointer plus context, which is what I showed in my example (indeed by rewriting h also). You argue that this would only be an issue if the stack was non-executable by default already. I am fine with only triggering the warning with -Wall when otherwise the stack would silently be marked as executable. The only problem is that there currently is no target macro that can easily be used to tell whether or not a target has the stack non-executable by default. So we would have to introduce that.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #18 from Segher Boessenkool --- Your "trivial rewrite" does not work at all (you didn't modify "h"). It isn't trivial to avoid this warning at all, which was half of my point. The other half is that we should not warn that the normal use of a GCC C extension would be "bad" or a "security problem" when it is neither. Yes, marking the whole stack executable for the whole lifetime of a process when something in your program may use a trampoline is less than optimal, may make some exploits easier to pull off. But that is all. Making -Wall complain about it in all cases will only result in more people not using nested functions, although those are a useful C language extension (and required for some other languages, so GCC has to implement it *anyway*), while not actually giving them any more security. Nested functions are not insecure. Trampolines are not insecure. Executable trampolines are not insecure. Executable trampolines on the stack are not insecure. Executable *everything* on the stack is not insecure, on many architectures. The only thing is that having the whole stack executable makes it easier to exploit other problems, on other architectures.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088
--- Comment #17 from Mark Wielaard ---
(In reply to Segher Boessenkool from comment #16)
> Something as trivial as this
>
> ===
> void h(int (*)(void));
> void f(int x)
> {
> int g(void) { return x; }
> h(g);
> }
> ===
>
> will already do. *Anything* that needs trampolines will do.
Sure, if you pass around a nested function pointer that captures context that
the compiler cannot inline because it is passed to another function that cannot
capture that same context.
I just haven't seen any code base that does that. That is why I was asking for
a concrete example.
But I know it is possible to accidentally do this and that is why I like the
have -Wall/-Wtrampolines warn about it. The above example can trivially be
rewritten to not give the warning:
void h(int (*)(int), int);
void f(int x)
{
int g(int i) { return i; }
h(g, x);
}
And I believe that is what normally people would want. Having
-Wall/-Wtrampolines warn about it when they make the mistake of writing code
that would require executable code on the stack when they use nested functions.
I could be wrong of course, I might not have enough imagination to appreciate
that what I believe is the correct/trivial way to rewrite the code to avoid the
warning (and potential security issue) is in fact a real burden in some
environments.
Is your objection to add -Wtrampolines to -Wall because you have actual code
that would be hard to rewrite to avoid the warning? Or is it just that you
believe it should not be a default warning on some targets (those which already
have the security issue of having a default executable stack)?
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088
--- Comment #16 from Segher Boessenkool ---
Something as trivial as this
===
void h(int (*)(void));
void f(int x)
{
int g(void) { return x; }
h(g);
}
===
will already do. *Anything* that needs trampolines will do.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #15 from Mark Wielaard --- (In reply to Segher Boessenkool from comment #14) > It is very hard to avoid the warning if you use this feature (you need to > stop using the feature altogether!), which would disqualify it for -Wall > immediately Could you give an example of a program where you see -Wtrampolines produce a warning that you believe you cannot easily work around to silence it? In practice I found the warning almost never triggers even in code that uses nested functions a lot since it is only when there is a need to generate executable code on the stack that it shows up. > -Wtrampolines as it currently is does not belong in -Wall. I think it would be more useful if done generically, but I don't mind not adding it to -Wall for targets you feel strongly about should not have it. I just don't know which guard to use to skip targets which explicitly make their stack always executable.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #14 from Segher Boessenkool --- It is *not* a security issue for many environments, and the warning warns whenever you use a useful feature. It is very hard to avoid the warning if you use this feature (you need to stop using the feature altogether!), which would disqualify it for -Wall immediately, except you may say "but the security impact is so important". Since the security impact is zero for many environments, that is not a good argument. -Wtrampolines as it currently is does not belong in -Wall.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #13 from Mark Wielaard --- (In reply to Segher Boessenkool from comment #12) > Requiring everything on the stack to always be executable, while normally it > is > not, is an issue, sure. > > Requiring the stack to be executable when *everything* is executable > *always*, > is not an issue. I think we are talking past each other. -Wtrampolines warns about putting executable code on the stack, which is a questionable construct (that almost never happens, because it would only happen with specific uses of nested functions where taking the address of such a function is put into an environment that need an executable code trampoline put on on the stack) but that you would like to know about so you can avoid it because it also is a security issue and easy to avoid if you know it is happening, which is why it should be added to -Wall. You seem to argue that -Wtrampolines should warn differently for different targets (or that -Wtrampolines should only be added to -Wall for certain targets, but not others). I am not sure it really should be treated differently, it is a questionable code construct whether or not the stack is already executable or not IMHO. I think it would be helpful if you gave a concrete example where having -Wtrampolines in -Wall would produce the warning for a specific piece of code where you think it shouldn't. Do you have a specific program for a specific target in mind?
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #12 from Segher Boessenkool --- Requiring everything on the stack to always be executable, while normally it is not, is an issue, sure. Requiring the stack to be executable when *everything* is executable *always*, is not an issue.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #11 from Mark Wielaard --- (In reply to Segher Boessenkool from comment #10) > As I said, very many targets have no concept of "executable" at all. > Most of the *-elf targets, most (all?) of the *-aout targets. > > Not all of the world is Linux. But generating code on the stack requiring an executable stack is a security issue. If only because by creating executable that need that prevent the target from adopting a non-executable stack. > -Wall is only for *easy to avoid* warnings, which -Wtrampolines very much > is not: > > '-Wall' > This enables all the warnings about constructions that some users > consider questionable, and that are easy to avoid (or modify to > prevent the warning), even in conjunction with macros. I just don't agree I think. This is a warning about a generally frowned upon use of a corner case of a GNU extension. The warning is just about that specific questionable construct (taking a pointer of a nested function and passing it around for use in a way that GCC cannot simply inline its usage) that cause a security issue (generation of on-stack executable code). It would really be helpful if you could point to a concrete example where this specific usage causes a warning with -Wtrampoline which is not correct.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #10 from Segher Boessenkool --- As I said, very many targets have no concept of "executable" at all. Most of the *-elf targets, most (all?) of the *-aout targets. Not all of the world is Linux. -Wall is only for *easy to avoid* warnings, which -Wtrampolines very much is not: '-Wall' This enables all the warnings about constructions that some users consider questionable, and that are easy to avoid (or modify to prevent the warning), even in conjunction with macros.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #9 from Mark Wielaard --- (In reply to Segher Boessenkool from comment #5) > The documentation currently says > > '-Wtrampolines' > Warn about trampolines generated for pointers to nested functions. > A trampoline is a small piece of data or code that is created at > run time on the stack when the address of a nested function is > taken, and is used to call the nested function indirectly. For > some targets, it is made up of data only and thus requires no > special treatment. But, for most targets, it is made up of code > and thus requires the stack to be made executable in order for the > program to work properly. > > This isn't true if the trampoline is just data, if your reading in comment 2 > is correct. The documentation can be improved for sure. It doesn't make explicit that the warning is only when a trampoline is generated that is code placed on the stack (which might require the stack to be made executable on some targets). But a strict reading might suggest that it also warns in other cases. Lets just improve the documentation. > This isn't something to warn about (or at least not in -Wall or -W) if > executable code on the stack does not open up security concerns. > > For how to detect it, you can start at varasm.c:file_end_indicate_exec_stack > , > which isn't exactly what is needed, but you can start there. Or perhaps > builtins,c:expand_builtin_init_trampoline is better. You mean move the warning to some later point? I am not sure that is a good idea because then you loose the diagnostic location of the code. In general I do agree with comment #6. If at all possible we should make the warning generic. Generating code on the stack is a bad thing in general whether or not the stack is already executable or not IMHO. It would be helpful if you could show a concrete example of -Wtrampolines giving a warning where it is wrong to warn. What specific target are you concerned about?
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #8 from Florian Weimer --- (In reply to Segher Boessenkool from comment #7) > The number of targets where such a warning is meaningless is _big_, that is > the point (most of the (older) embedded targets). There are a lot of targets which were once read-implies-exec, but have later implementations which are not. I guess the dealbreaker is whether GCC emits code or markers to make the stack executable. If it does that, it should warn.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #7 from Segher Boessenkool --- The number of targets where such a warning is meaningless is _big_, that is the point (most of the (older) embedded targets). If the warning warns where there is no problem, it does not belong in -Wall or -Wextra.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #6 from Florian Weimer --- I'm not a fan of target-specific warnings. In this case, the number of targets where this the warning would not be appropriate would be vanishingly small, though, so I do not think this is a problem in practice.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #5 from Segher Boessenkool --- The documentation currently says '-Wtrampolines' Warn about trampolines generated for pointers to nested functions. A trampoline is a small piece of data or code that is created at run time on the stack when the address of a nested function is taken, and is used to call the nested function indirectly. For some targets, it is made up of data only and thus requires no special treatment. But, for most targets, it is made up of code and thus requires the stack to be made executable in order for the program to work properly. This isn't true if the trampoline is just data, if your reading in comment 2 is correct. This isn't something to warn about (or at least not in -Wall or -W) if executable code on the stack does not open up security concerns. For how to detect it, you can start at varasm.c:file_end_indicate_exec_stack , which isn't exactly what is needed, but you can start there. Or perhaps builtins,c:expand_builtin_init_trampoline is better.
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #4 from Mark Wielaard --- I think the point of the warning is to note that executable code is generated on the stack (which seems to always be something to warn about IMHO). But I am fine with only enabling -Wtrampolines with -Wall for targets that explicitly need an executable stack for that. What is the correct way to detect the target needs that?
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #3 from Segher Boessenkool --- It is good that it doesn't warn if trampolines are not data. This is not documented, fwiw. It also warns if the stack is executable *anyway*, like it is for many targets. This is not useful; as documented, the point of the warning is to warn the user that the stack is made executable because some trampoline somewhere needs it. Not warn the user that there is a trampoline somewhere(*), which would be as useful as warning the user that the program source code is a multiple of six lines long, which is exactly as harmful :-P (*) Which, as you say, is not what it does anyway!
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 --- Comment #2 from Mark Wielaard --- (In reply to Segher Boessenkool from comment #1) > This also would warn for targets where it is not an issue at all (where > trampolines are just data, or where the stack is executable anyway, or where > there is no such "executable" concept at all, for example). Do you have an example where -Wtrampolines warns unnecessarily? I might be reading the code wrong, but the trampolines warning is guarded by on_stack and targetm.calls.custom_function_descriptors != 0. So I believe it wouldn't warn in case the trampoline is generated on the heap or if the target uses descriptors for nested functions (which eliminates the need for trampolines that reside on the stack and require it to be made executable).
[Bug c/88088] -Wtrampolines should be enabled by -Wall (or -Wextra)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88088 Segher Boessenkool changed: What|Removed |Added CC||segher at gcc dot gnu.org --- Comment #1 from Segher Boessenkool --- This also would warn for targets where it is not an issue at all (where trampolines are just data, or where the stack is executable anyway, or where there is no such "executable" concept at all, for example).
