Now that nolibc enable stackprotector support automatically when the
compiler enables it we only have to get the -fstack-protector flags
correct.
The cc-options are structured so that -fstack-protector-all is only
enabled if -mstack-protector=guard works, as that is the only mode
supported by nolibc.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
tools/testing/selftests/nolibc/Makefile | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index bd41102ea299..445c352b1b33 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -76,20 +76,11 @@ else
Q=@
endif
-CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
- $(call cc-option,-mstack-protector-guard=global) \
- $(call cc-option,-fstack-protector-all)
-CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
-CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
-CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
-CFLAGS_STKP_arm64 = $(CFLAGS_STACKPROTECTOR)
-CFLAGS_STKP_arm = $(CFLAGS_STACKPROTECTOR)
-CFLAGS_STKP_mips = $(CFLAGS_STACKPROTECTOR)
-CFLAGS_STKP_riscv = $(CFLAGS_STACKPROTECTOR)
-CFLAGS_STKP_loongarch = $(CFLAGS_STACKPROTECTOR)
+CFLAGS_STACKPROTECTOR = $(call cc-option,-mstack-protector-guard=global -fstack-protector-all)
CFLAGS_s390 = -m64
CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
$(call cc-option,-fno-stack-protector) \
+ $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all)) \
$(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
LDFLAGS := -s
--
2.40.1
On 2023-05-21 11:36:35+0200, Thomas Weißschuh wrote:
> Now that nolibc enable stackprotector support automatically when the
> compiler enables it we only have to get the -fstack-protector flags
> correct.
>
> The cc-options are structured so that -fstack-protector-all is only
> enabled if -mstack-protector=guard works, as that is the only mode
> supported by nolibc.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> tools/testing/selftests/nolibc/Makefile | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> index bd41102ea299..445c352b1b33 100644
> --- a/tools/testing/selftests/nolibc/Makefile
> +++ b/tools/testing/selftests/nolibc/Makefile
> @@ -76,20 +76,11 @@ else
> Q=@
> endif
>
> -CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
> - $(call cc-option,-mstack-protector-guard=global) \
> - $(call cc-option,-fstack-protector-all)
> -CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_arm64 = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_arm = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_mips = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_riscv = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_loongarch = $(CFLAGS_STACKPROTECTOR)
> +CFLAGS_STACKPROTECTOR = $(call cc-option,-mstack-protector-guard=global -fstack-protector-all)
> CFLAGS_s390 = -m64
> CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
> $(call cc-option,-fno-stack-protector) \
> + $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all)) \
> $(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
> LDFLAGS := -s
I noticed, of course after having sent the series, that the cleanup here
was not done properly.
CFLAGS_STACKPROTECTOR and CFLAGS_STKP should be deleted completely.
This will be fixed in v2, or feel free to fix it up when applying the
series.
Hi Thomas, Zhangjin,
I merged and pushed this series on top of the previous series, it's in
branch 20230523-nolibc-rv32+stkp3.
However, Thomas, I found an issue with this last patch that I partially
reverted in a last patch I pushed as well so that we can discuss it:
On Sun, May 21, 2023 at 11:36:35AM +0200, Thomas Wei?schuh wrote:
>
> -CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
> - $(call cc-option,-mstack-protector-guard=global) \
> - $(call cc-option,-fstack-protector-all)
> -CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_arm64 = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_arm = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_mips = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_riscv = $(CFLAGS_STACKPROTECTOR)
> -CFLAGS_STKP_loongarch = $(CFLAGS_STACKPROTECTOR)
> +CFLAGS_STACKPROTECTOR = $(call cc-option,-mstack-protector-guard=global -fstack-protector-all)
> CFLAGS_s390 = -m64
> CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
> $(call cc-option,-fno-stack-protector) \
> + $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all)) \
> $(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
By doing so, we reintroduce the forced stack-protector mechanism
that cannot be disabled anymore. The ability to disable it was the
main point of the options above. In fact checking __SSP__* was a
solution to save the user from having to set -DNOLIBC_STACKPROTECTOR
to match their compiler's flags, but here in the nolibc-test we still
want to be able to forcefully disable stkp for a run (at least to
unbreak gcc-9, and possibly to confirm that non-stkp builds still
continue to work when your local toolchain has it by default).
So I reverted that part and only dropped -DNOLIBC_STACKPROTECTOR.
This way I could run the test on all archs with gcc-9 by forcing
CFLAGS_STACKPROTECTOR= and verified it was still possible to
disable it for a specific arch only by setting CFLAGS_STKP_$ARCH.
If you're fine with this we can squash this one into your cleanup
patch.
Zhangjin I think this branch should work well enough for you to
serve as a base for your upcoming series. We'll clean it up later
once we all agree on the stat() replacement for syscall() and on
the various remaining points including your time32 alternatives.
Thanks to you both,
Willy
PS: we're still carrying a long CC list of individuals who are likely
not that much interested in our discussion, I propose that we trim
it on next exchanges and only keep us 3 and the lists, in order to
remove some of their e-mail pollution.
On 2023-05-23 22:12:38+0200, Willy Tarreau wrote:
> Hi Thomas, Zhangjin,
>
> I merged and pushed this series on top of the previous series, it's in
> branch 20230523-nolibc-rv32+stkp3.
>
> However, Thomas, I found an issue with this last patch that I partially
> reverted in a last patch I pushed as well so that we can discuss it:
>
> On Sun, May 21, 2023 at 11:36:35AM +0200, Thomas Weißschuh wrote:
> >
> > -CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
> > - $(call cc-option,-mstack-protector-guard=global) \
> > - $(call cc-option,-fstack-protector-all)
> > -CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_arm64 = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_arm = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_mips = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_riscv = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_loongarch = $(CFLAGS_STACKPROTECTOR)
> > +CFLAGS_STACKPROTECTOR = $(call cc-option,-mstack-protector-guard=global -fstack-protector-all)
> > CFLAGS_s390 = -m64
> > CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
> > $(call cc-option,-fno-stack-protector) \
> > + $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all)) \
> > $(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
>
> By doing so, we reintroduce the forced stack-protector mechanism
> that cannot be disabled anymore. The ability to disable it was the
> main point of the options above. In fact checking __SSP__* was a
> solution to save the user from having to set -DNOLIBC_STACKPROTECTOR
> to match their compiler's flags, but here in the nolibc-test we still
> want to be able to forcefully disable stkp for a run (at least to
> unbreak gcc-9, and possibly to confirm that non-stkp builds still
> continue to work when your local toolchain has it by default).
Wouldn't this work?
make CFLAGS_x86=-fno-stack-protector nolibc-test
Or we do
CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
CFLAGS ?= ... $(CFLAGS_STACKPROTECTOR)
And then it is always:
make CFLAGS_STACKPROTECTOR= nolibc-test
An alternative would also be to use a GCC 9 compatible mechanism:
#if __has_attribute(no_stack_protector)
#define __no_stack_protector __attribute__((no_stack_protector))
#else
#define __no_stack_protector __attribute__((__optimize__("-fno-stack-protector")))
#endif
Or we combine CFLAGS_STACKPROTECTOR and __no_stack_protector.
> So I reverted that part and only dropped -DNOLIBC_STACKPROTECTOR.
> This way I could run the test on all archs with gcc-9 by forcing
> CFLAGS_STACKPROTECTOR= and verified it was still possible to
> disable it for a specific arch only by setting CFLAGS_STKP_$ARCH.
>
> If you're fine with this we can squash this one into your cleanup
> patch.
To be honest I was happy to get rid of all these architecture specific
variables.
> Zhangjin I think this branch should work well enough for you to
> serve as a base for your upcoming series. We'll clean it up later
> once we all agree on the stat() replacement for syscall() and on
> the various remaining points including your time32 alternatives.
>
> Thanks to you both,
> Willy
>
> PS: we're still carrying a long CC list of individuals who are likely
> not that much interested in our discussion, I propose that we trim
> it on next exchanges and only keep us 3 and the lists, in order to
> remove some of their e-mail pollution.
I trimmed the list a bit.
Thomas
On Tue, May 23, 2023 at 10:38:48PM +0200, Thomas Wei?schuh wrote:
> On 2023-05-23 22:12:38+0200, Willy Tarreau wrote:
> > Hi Thomas, Zhangjin,
> >
> > I merged and pushed this series on top of the previous series, it's in
> > branch 20230523-nolibc-rv32+stkp3.
> >
> > However, Thomas, I found an issue with this last patch that I partially
> > reverted in a last patch I pushed as well so that we can discuss it:
> >
> > On Sun, May 21, 2023 at 11:36:35AM +0200, Thomas Wei?schuh wrote:
> > >
> > > -CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
> > > - $(call cc-option,-mstack-protector-guard=global) \
> > > - $(call cc-option,-fstack-protector-all)
> > > -CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
> > > -CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
> > > -CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
> > > -CFLAGS_STKP_arm64 = $(CFLAGS_STACKPROTECTOR)
> > > -CFLAGS_STKP_arm = $(CFLAGS_STACKPROTECTOR)
> > > -CFLAGS_STKP_mips = $(CFLAGS_STACKPROTECTOR)
> > > -CFLAGS_STKP_riscv = $(CFLAGS_STACKPROTECTOR)
> > > -CFLAGS_STKP_loongarch = $(CFLAGS_STACKPROTECTOR)
> > > +CFLAGS_STACKPROTECTOR = $(call cc-option,-mstack-protector-guard=global -fstack-protector-all)
> > > CFLAGS_s390 = -m64
> > > CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
> > > $(call cc-option,-fno-stack-protector) \
> > > + $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all)) \
> > > $(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
> >
> > By doing so, we reintroduce the forced stack-protector mechanism
> > that cannot be disabled anymore. The ability to disable it was the
> > main point of the options above. In fact checking __SSP__* was a
> > solution to save the user from having to set -DNOLIBC_STACKPROTECTOR
> > to match their compiler's flags, but here in the nolibc-test we still
> > want to be able to forcefully disable stkp for a run (at least to
> > unbreak gcc-9, and possibly to confirm that non-stkp builds still
> > continue to work when your local toolchain has it by default).
>
> Wouldn't this work?
>
> make CFLAGS_x86=-fno-stack-protector nolibc-test
Not that much because as you can see, CFLAGS is more of an
accumulator than an input cflags. Once it starts to accumulate
automatic flags detected for various reasons it becomes a huge
burden for end users to figure what to manually put there.
> Or we do
>
> CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
> CFLAGS ?= ... $(CFLAGS_STACKPROTECTOR)
> And then it is always:
>
> make CFLAGS_STACKPROTECTOR= nolibc-test
This one is more in line with the partial revert. I don't have a
*strong* feeling for the per-arch setting but it did make sense when
not all archs were supporting it, and I've used per-arch setting when
I was having different compiler versions for certain archs (e.g. for
loongarch I used gcc12).
> An alternative would also be to use a GCC 9 compatible mechanism:
>
> #if __has_attribute(no_stack_protector)
> #define __no_stack_protector __attribute__((no_stack_protector))
> #else
> #define __no_stack_protector __attribute__((__optimize__("-fno-stack-protector")))
> #endif
I tried this one but I remember some initial failures and a possible
later success when instrumenting the correct function, so my memory
is not intact on this one. However if we manage to find a working
solution for gcc-9 and it affects the nolibc part (not the test part),
we definitely need to merge it because if some users have working stkp
on gcc-9 and we break it, that will not be cool.
> Or we combine CFLAGS_STACKPROTECTOR and __no_stack_protector.
I think that CFLAGS_STACKPROTECTOR should only be a makefile variable
to forcefully enable/disable that but not separately exposed in the
code.
> > So I reverted that part and only dropped -DNOLIBC_STACKPROTECTOR.
> > This way I could run the test on all archs with gcc-9 by forcing
> > CFLAGS_STACKPROTECTOR= and verified it was still possible to
> > disable it for a specific arch only by setting CFLAGS_STKP_$ARCH.
> >
> > If you're fine with this we can squash this one into your cleanup
> > patch.
>
> To be honest I was happy to get rid of all these architecture specific
> variables.
I generally agree with the principle and I think I can live with that.
So let's summarize: how about:
- we only keep the CFLAGS_STACKPROTECTOR variable that is easy to
reset from the make command line. Those with multiple compilers
are free to globally enable/disable or pass that on multiple
make command lines if needed, but we decide it's the users'
test instrumentation problem, not the maintainers'.
- we try our best to make sure that gcc-9 with stkp enabled still
produces working code (with/without stkp I don't know but not
something that smashes the stack at least).
IMHO this would be clean enough and sufficiently maintainable for the
long term.
> I trimmed the list a bit.
Thanks for them ;-)
Willy