On Wed, 17 Jun 2020, H.J. Lu wrote:
> On Wed, Jun 17, 2020 at 5:33 AM Richard Biener <rguent...@suse.de> wrote:
> >
> > 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?
>
> Linker will silently change CET run-time behavior.
>
> > Does -fcf-protection have effects "early" (up to IPA)? That is, is it
> > safe to turn it on only post-IPA?
>
> I think so. We don't do anything with SHSTK. For IBT, we just insert an
> ENDBR when we output the indirect branch target.
>
> > 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
>
> I don't think we should warn about the explicit -fcf-protection option at link
> time.
>
> > 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).
> >
>
> I think we should give an error if there are any mismatches in
> inputs and let the explicit -fcf-protection option at link time override
> -fcf-protection options in inputs .
Note the linker will then still silently change CET run-time behavior
when there's a non-LTO object involved in the link that has
mismatched settings.
Matthias - currently find_and_merge_options does not have access
to the link-time options, you'd need to pass them down (they are
in 'decoded_options', gathered via get_options_from_collect_gcc_options).
Note the option overriding simply happens because we append the link-time
options to any options coming from the IL file so the only thing you
need to do is turn the warning back to an error but guard that with
the presence of the option in the link-time options.
Richard.
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
