On Wed, Aug 30, 2023 at 12:50:40PM +0200, Jakub Jelinek wrote:
> On Tue, Aug 29, 2023 at 03:42:27PM -0400, Marek Polacek via Gcc-patches wrote:
> > + if (UNLIKELY (flag_hardened)
> > + && (opt->code == OPT_D || opt->code == OPT_U))
> > + {
> > + if (!fortify_seen_p)
> > + fortify_seen_p = !strncmp (opt->arg, "_FORTIFY_SOURCE", 15);
>
> Perhaps this should check that the char after it is either '\0' or '=', we
> shouldn't care if user defines or undefines _FORTIFY_SOURCE_WHATEVER macro.
Right, should be fixed in
https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630550.html
> > + if (!cxx_assert_seen_p)
> > + cxx_assert_seen_p = !strcmp (opt->arg, "_GLIBCXX_ASSERTIONS");
>
> Like we don't care in this case about -D_GLIBCXX_ASSERTIONS42
This too.
> > + }
> > + }
> > +
> > + if (flag_hardened)
> > + {
> > + if (!fortify_seen_p && optimize > 0)
> > + {
> > + if (TARGET_GLIBC_MAJOR == 2 && TARGET_GLIBC_MINOR >= 35)
> > + cpp_define (parse_in, "_FORTIFY_SOURCE=3");
> > + else
> > + cpp_define (parse_in, "_FORTIFY_SOURCE=2");
>
> I wonder if it wouldn't be better to enable _FORTIFY_SOURCE=2 by default for
> -fhardened only for targets which actually have such a support in the C
> library. There is some poor man's _FORTIFY_SOURCE support in libssp,
> but e.g. one has to link with -lssp in that case and compile with
> -isystem `gcc -print-include-filename=include`/ssp .
> For glibc that is >= 2.3.4, https://maskray.me/blog/2022-11-06-fortify-source
> mentions NetBSD support since 2006, newlib since 2017, some Darwin libc,
> bionic (but seems they have only some clang support and dropped GCC
> support) and some third party reimplementation of libssp.
> Or do we just enable it and hope that either it works well or isn't
> supported at all quietly? E.g. it would certainly break the ssp case
> where -isystem finds ssp headers but -lssp isn't linked in.
I don't really have an idea how this should be handled. I left it as
it was.
> > @@ -4976,6 +4993,22 @@ process_command (unsigned int decoded_options_count,
> > #endif
> > }
> >
> > + /* TODO: check if -static -pie works and maybe use it. */
> > + if (flag_hardened && !any_link_options_p && !static_p)
> > + {
> > + save_switch ("-pie", 0, NULL, /*validated=*/true, /*known=*/false);
> > + /* TODO: check if BIND_NOW/RELRO is supported. */
> > + if (true)
> > + {
> > + /* These are passed straight down to collect2 so we have to break
> > + it up like this. */
> > + add_infile ("-z", "*");
> > + add_infile ("now", "*");
> > + add_infile ("-z", "*");
> > + add_infile ("relro", "*");
>
> As the TODO comment says, to do that we need to check at configure time that
> linker supports -z now and -z relro options.
I've added configure checks in
https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630550.html
> > @@ -1117,9 +1121,12 @@ finish_options (struct gcc_options *opts, struct
> > gcc_options *opts_set,
> > }
> >
> > /* We initialize opts->x_flag_stack_protect to -1 so that targets
> > - can set a default value. */
> > + can set a default value. With --enable-default-ssp or -fhardened
> > + the default is -fstack-protector-strong. */
> > if (opts->x_flag_stack_protect == -1)
> > - opts->x_flag_stack_protect = DEFAULT_FLAG_SSP;
> > + opts->x_flag_stack_protect = (opts->x_flag_hardened
> > + ? SPCT_FLAG_STRONG
> > + : DEFAULT_FLAG_SSP);
>
> This needs to be careful, -fstack-protector isn't supported on all targets
> (e.g. ia64) and we don't want toplev.cc warning:
> /* Targets must be able to place spill slots at lower addresses. If the
> target already uses a soft frame pointer, the transition is trivial. */
> if (!FRAME_GROWS_DOWNWARD && flag_stack_protect)
> {
> warning_at (UNKNOWN_LOCATION, 0,
> "%<-fstack-protector%> not supported for this target");
> flag_stack_protect = 0;
> }
> to be emitted whenever using -fhardened, it should not be enabled there
> silently (for ia64 Fedora/RHEL gcc actually had a short patch to make it
> work, turn the target into FRAME_GROWS_DOWNWARD one if -fstack-protect* was
> enabled and otherwise keep it !FRAME_GROWS_DOWNWARD).
Unfortunately, I found out that I can't just check FRAME_GROWS_DOWNWARD.
Some targets like rs6000 and mips use flag_stack_protect in the definition
of FRAME_GROWS_DOWNWARD, and that is not usable in finish_options. There,
you have to use opts->x_flag_*. Besides, some targets like BPF don't
support -fstack-protector at all, but in finish_options we can't know that
yet (AFAIK).
I fixed it, but in a very ugly way, using a new global. :/
Thanks,
Marek
