From: Marek Polacek <polacek@redhat.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: Jakub Jelinek <jakub@redhat.com>, GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: RFC: Introduce -fhardened to enable security-related flags
Date: Fri, 15 Sep 2023 11:21:09 -0400 [thread overview]
Message-ID: <ZQR2ZaTziaQwfJ6J@redhat.com> (raw)
In-Reply-To: <CAFiYyc3F08W-ZTwBhgOJM___o8FD3qP7k_HxAf=YNLZsuG612Q@mail.gmail.com>
On Wed, Aug 30, 2023 at 03:08:46PM +0200, Richard Biener wrote:
> On Wed, Aug 30, 2023 at 12:51 PM Jakub Jelinek via Gcc-patches
> <gcc-patches@gcc.gnu.org> 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.
> >
> > > + 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
> >
> > > + }
> > > + }
> > > +
> > > + 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.
> >
> > > @@ -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.
> >
> > > @@ -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).
>
> I'll note that with selectively enabling parts of -fhardening it can
> also give a false
> sensation of safety when under the hood we ignore half of the option due to
> one or another reason ...
I suppose you're right :/.
> How does -fhardening reflect into -[gf]record-gcc-switches? Is it at
> least possible
> to verify the actually enabled bits?
In DW_AT_producer it simply shows "-fhardened". It doesn't expand to what
it actually enabled. I imagine gen_producer_string could be tweaked to
print the options enabled by -fhardened. But, DW_AT_producer doesn't seem
to show -D options, or linked options like -Wl,-z,now. So I think we need
something better. Maybe add a line in gcc -v? I would not like to add
a new option just for this.
(It would have to be careful not to show options that the target doesn't
support, like -fstack-protector on BPF.)
Marek
next prev parent reply other threads:[~2023-09-15 15:21 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-29 19:42 Marek Polacek
2023-08-29 20:11 ` Sam James
2023-08-29 22:07 ` Marek Polacek
2023-08-30 8:46 ` Martin Uecker
2023-09-15 15:11 ` Marek Polacek
2023-09-16 7:40 ` Martin Uecker
2023-08-30 9:06 ` Xi Ruoyao
2023-09-15 15:12 ` Marek Polacek
2023-08-30 10:50 ` Jakub Jelinek
2023-08-30 13:08 ` Richard Biener
2023-09-15 15:21 ` Marek Polacek [this message]
2023-09-15 15:17 ` Marek Polacek
2023-09-01 22:09 ` Qing Zhao
2023-09-04 22:40 ` Richard Sandiford
2023-09-15 15:23 ` Marek Polacek
2023-09-15 15:40 ` Marek Polacek
2023-09-14 14:48 ` Hongtao Liu
2023-09-17 3:16 ` Hans-Peter Nilsson
2023-09-17 4:00 ` Sam James
2023-09-17 16:36 ` Hans-Peter Nilsson
2023-09-18 7:21 ` Sam James
2023-09-18 14:34 ` Hans-Peter Nilsson
2023-09-19 14:19 ` Qing Zhao
2023-09-21 16:53 ` Hans-Peter Nilsson
2023-09-20 11:33 ` jvoisin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZQR2ZaTziaQwfJ6J@redhat.com \
--to=polacek@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).