On Mon, 13 Jul 2020, Matthias Klose wrote:
> On 6/17/20 3:11 PM, Richard Biener wrote:
> > 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.
>
> here's an updated patch. checked that it errors out with .o files compiled
> with
> different -fcf-protection values, and checked that overriding the
> -fcf-protection value at link time works.
+ : global_options.x_flag_cf_protection == CF_RETURN
+ ? "-fcf-protection=RETURN"
typo? Should be -fcf-protection=return
OK with that fixed. If you smoke-tested this in the real-world in
Ubuntu with GCC 10 it's also OK for the GCC 10 branch if you are
quick (RC is tomorrow).
Thanks,
Richard.
> Matthias
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
