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 <[email protected]> wrote:
>>>
>>> On Wed, 17 Jun 2020, H.J. Lu wrote:
>>>
>>>> On Wed, Jun 17, 2020 at 5:00 AM Richard Biener <[email protected]> wrote:
>>>>>
>>>>> On Wed, 17 Jun 2020, H.J. Lu wrote:
>>>>>
>>>>>> On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <[email protected]> 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.
Matthias
# DP: Fix PR lto/95604, proposed patch
PR lto/95604
* lto-wrapper.c (merge_and_complain): Add decoded options as parameter,
error on different values for -fcf-protection.
(append_compiler_options): Pass -fcf-protection option.
(find_and_merge_options): Add decoded options as parameter,
pass decoded_options to merge_and_complain.
(run_gcc): Pass decoded options to find_and_merge_options.
* lto-opts.c (lto_write_options): Pass -fcf-protection option.
--- a/src/gcc/lto-opts.c
+++ b/src/gcc/lto-opts.c
@@ -94,6 +94,21 @@ lto_write_options (void)
: "-fno-pie");
}
+ if (!global_options_set.x_flag_cf_protection)
+ {
+ append_to_collect_gcc_options (
+ &temporary_obstack, &first_p,
+ global_options.x_flag_cf_protection == CF_NONE
+ ? "-fcf-protection=none"
+ : global_options.x_flag_cf_protection == CF_FULL
+ ? "-fcf-protection=full"
+ : global_options.x_flag_cf_protection == CF_BRANCH
+ ? "-fcf-protection=branch"
+ : global_options.x_flag_cf_protection == CF_RETURN
+ ? "-fcf-protection=RETURN"
+ : "");
+ }
+
/* If debug info is enabled append -g. */
if (debug_info_level > DINFO_LEVEL_NONE)
append_to_collect_gcc_options (&temporary_obstack, &first_p, "-g");
--- a/src/gcc/lto-wrapper.c
+++ b/src/gcc/lto-wrapper.c
@@ -192,11 +192,14 @@ static void
merge_and_complain (struct cl_decoded_option **decoded_options,
unsigned int *decoded_options_count,
struct cl_decoded_option *fdecoded_options,
- unsigned int fdecoded_options_count)
+ unsigned int fdecoded_options_count,
+ struct cl_decoded_option *decoded_cl_options,
+ unsigned int decoded_cl_options_count)
{
unsigned int i, j;
struct cl_decoded_option *pic_option = NULL;
struct cl_decoded_option *pie_option = NULL;
+ struct cl_decoded_option *cf_protection_option = NULL;
/* ??? Merge options from files. Most cases can be
handled by either unioning or intersecting
@@ -211,6 +214,17 @@ merge_and_complain (struct cl_decoded_op
In absence of that it's unclear what a good default is.
It's also difficult to get positional handling correct. */
+ /* Look for a -fcf-protection option in the link-time options
+ which overrides any -fcf-protection from the lto sections. */
+ for (i = 0; i < decoded_cl_options_count; ++i)
+ {
+ struct cl_decoded_option *foption = &decoded_cl_options[i];
+ if (foption->opt_index == OPT_fcf_protection_)
+ {
+ cf_protection_option = foption;
+ }
+ }
+
/* The following does what the old LTO option code did,
union all target and a selected set of common options. */
for (i = 0; i < fdecoded_options_count; ++i)
@@ -287,6 +301,23 @@ merge_and_complain (struct cl_decoded_op
foption->orig_option_with_args_text);
break;
+ case OPT_fcf_protection_:
+ /* Default to link-time option, else append or check identical. */
+ if (!cf_protection_option)
+ {
+ 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))
+ fatal_error (input_location,
+ "option -fcf-protection with mismatching values"
+ " (%s, %s)",
+ (*decoded_options)[j].arg, foption->arg);
+ }
+ break;
+
case OPT_O:
case OPT_Ofast:
case OPT_Og:
@@ -631,6 +662,7 @@ append_compiler_options (obstack *argv_o
case OPT_fopenacc:
case OPT_fopenacc_dim_:
case OPT_foffload_abi_:
+ case OPT_fcf_protection_:
case OPT_g:
case OPT_O:
case OPT_Ofast:
@@ -988,12 +1020,14 @@ find_crtoffloadtable (void)
/* A subroutine of run_gcc. Examine the open file FD for lto sections with
name prefix PREFIX, at FILE_OFFSET, and store any options we find in OPTS
- and OPT_COUNT. Return true if we found a matchingn section, false
+ and OPT_COUNT. Return true if we found a matching section, false
otherwise. COLLECT_GCC holds the value of the environment variable with
the same name. */
static bool
find_and_merge_options (int fd, off_t file_offset, const char *prefix,
+ struct cl_decoded_option *decoded_cl_options,
+ unsigned int decoded_cl_options_count,
struct cl_decoded_option **opts,
unsigned int *opt_count, const char *collect_gcc)
{
@@ -1040,7 +1074,9 @@ find_and_merge_options (int fd, off_t fi
else
merge_and_complain (&fdecoded_options,
&fdecoded_options_count,
- f2decoded_options, f2decoded_options_count);
+ f2decoded_options, f2decoded_options_count,
+ decoded_cl_options,
+ decoded_cl_options_count);
fopts += strlen (fopts) + 1;
}
@@ -1373,6 +1409,7 @@ run_gcc (unsigned argc, char *argv[])
}
if (find_and_merge_options (fd, file_offset, LTO_SECTION_NAME_PREFIX,
+ decoded_options, decoded_options_count,
&fdecoded_options, &fdecoded_options_count,
collect_gcc))
{
@@ -1576,6 +1613,7 @@ cont1:
fatal_error (input_location, "cannot open %s: %m", filename);
if (!find_and_merge_options (fd, file_offset,
OFFLOAD_SECTION_NAME_PREFIX,
+ decoded_options, decoded_options_count,
&offload_fdecoded_options,
&offload_fdecoded_options_count,
collect_gcc))
