On Wed, 17 Jun 2020, H.J. Lu wrote:
> On Wed, Jun 17, 2020 at 5:00 AM Richard Biener <rguent...@suse.de> wrote:
> >
> > On Wed, 17 Jun 2020, H.J. Lu wrote:
> >
> > > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
> > > <richard.guent...@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <d...@ubuntu.com> wrote:
> > > > >
> > > > > PR lto/95604 was seen when checking for binaries without having CET
> > > > > support in a
> > > > > distro archive, for binaries built with LTO optimization. The
> > > > > hardening flag
> > > > > -fcf-protection=full is passed in CFLAGS, and maybe should be passed
> > > > > in LDFLAGS
> > > > > as well. However to make it work when not passed to the link step,
> > > > > it should be
> > > > > extracted from the options found in the lto opts section.
> > > > >
> > > > > Richard suggested two solutions offline. I checked that both fix the
> > > > > test case.
> > > > > Which one to install? Also ok for the 9 and 10 branches?
> > > >
> > > > I guess even though variant two is simpler it doesn't make much sense to
> > > > have differing settings of -fcf-protection between different functions?
> > > > HJ?
> > >
> > > -fcf-protection is applied to a file, not a function since CET marker
> > > is per file.
> > >
> > > > So looking at variant one,
> > > >
> > > > @@ -287,6 +287,18 @@
> > > > foption->orig_option_with_args_text);
> > > > break;
> > > >
> > > > + case OPT_fcf_protection_:
> > > > + /* Append or check identical. */
> > > > + for (j = 0; j < *decoded_options_count; ++j)
> > > > + if ((*decoded_options)[j].opt_index == foption->opt_index)
> > > > + break;
> > > > + if (j == *decoded_options_count)
> > > > + append_option (decoded_options, decoded_options_count,
> > > > foption);
> > > > + else if (strcmp ((*decoded_options)[j].arg, foption->arg))
> > > > + warning (input_location, "option %s with different values",
> > > > + foption->orig_option_with_args_text);
> > > > + break;
> > > >
> > > > you are always streaming a -fcf-protection option so the if (j ==
> > > > *decoded_options_count)
> > > > case shouldn't ever happen but I guess it's safe to leave the code
> > > > as-is. Can you
> > > > amend the warning with the option that prevails? Maybe
> > > >
> > > > + else if (strcmp ((*decoded_options)[j].arg, foption->arg))
> > > > {
> > > > static bool noted;
> > > > + warning (input_location, "option %s with different values",
> > > > + foption->orig_option_with_args_text);
> > > > if (!noted)
> > > > {
> > > > inform ("%s will be used instead",
> > > > (*decoded_options)[j].orig_option_with_args_text);
> > > > noted = true;
> > > > }
> > > >
> > > > I guess input_location is simply zero so the diagnostic doesn't
> > > > contain the actual file we're
> > > > looking at. Something to improve I guess (also applyign to other
> > > > diagnostics we emit).
> > > >
> > > > Otherwise looks OK.
> > > >
> > > > Please wait for HJ in case he'd like to go with option two.
> > > >
> > >
> > > I prefer option one. But what happens if input files are compiled
> > > with different -fcf-protection settings?
> >
> > You get a warning and the first option setting wins (and I requested
> > to inform the user which that is).
> >
>
> I think it should be an error and the user should explicitly specify the
> option with -lfto in the final link command.
But AFAIK ld will not reject linking objects with mismatched settings?
Does -fcf-protection have effects "early" (up to IPA)? That is, is it
safe to turn it on only post-IPA?
So yeah, if the user provided an explicit -fcf-protection option at link
we honor that (but with the current patch we'd still warn about conflicts
between the original TU setting - but not conflicts with the setting
at link time). If we want to error on mismatches but allow if at link
time a setting is specified that needs extra code (it would be the
first time we have this behavior).
Richard.
