Re: [patch] fix PR lto/95604, -flto and -fcf-protection
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 wrote:
> >>>
> >>> On Wed, 17 Jun 2020, H.J. Lu wrote:
> >>>
> On Wed, Jun 17, 2020 at 5:00 AM Richard Biener wrote:
> >
> > On Wed, 17 Jun 2020, H.J. Lu wrote:
> >
> >> On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
> >> wrote:
> >>>
> >>> On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose
> >>> 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).
> >>>
>
Re: [patch] fix PR lto/95604, -flto and -fcf-protection
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 wrote:
>>>
>>> On Wed, 17 Jun 2020, H.J. Lu wrote:
>>>
On Wed, Jun 17, 2020 at 5:00 AM Richard Biener wrote:
>
> On Wed, 17 Jun 2020, H.J. Lu wrote:
>
>> On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
>> wrote:
>>>
>>> On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose 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
Re: [patch] fix PR lto/95604, -flto and -fcf-protection
On Wed, 17 Jun 2020, H.J. Lu wrote:
> On Wed, Jun 17, 2020 at 5:33 AM Richard Biener wrote:
> >
> > On Wed, 17 Jun 2020, H.J. Lu wrote:
> >
> > > On Wed, Jun 17, 2020 at 5:00 AM Richard Biener wrote:
> > > >
> > > > On Wed, 17 Jun 2020, H.J. Lu wrote:
> > > >
> > > > > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
> > > > > wrote:
> > > > > >
> > > > > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose
> > > > > > 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
Re: [patch] fix PR lto/95604, -flto and -fcf-protection
On Wed, Jun 17, 2020 at 5:33 AM Richard Biener wrote:
>
> On Wed, 17 Jun 2020, H.J. Lu wrote:
>
> > On Wed, Jun 17, 2020 at 5:00 AM Richard Biener wrote:
> > >
> > > On Wed, 17 Jun 2020, H.J. Lu wrote:
> > >
> > > > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
> > > > wrote:
> > > > >
> > > > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose
> > > > > 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 .
--
H.J.
Re: [patch] fix PR lto/95604, -flto and -fcf-protection
On Wed, 17 Jun 2020, H.J. Lu wrote:
> On Wed, Jun 17, 2020 at 5:00 AM Richard Biener wrote:
> >
> > On Wed, 17 Jun 2020, H.J. Lu wrote:
> >
> > > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
> > > wrote:
> > > >
> > > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose 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.
Re: [patch] fix PR lto/95604, -flto and -fcf-protection
On Wed, Jun 17, 2020 at 5:00 AM Richard Biener wrote:
>
> On Wed, 17 Jun 2020, H.J. Lu wrote:
>
> > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
> > wrote:
> > >
> > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose 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.
--
H.J.
Re: [patch] fix PR lto/95604, -flto and -fcf-protection
On Wed, 17 Jun 2020, H.J. Lu wrote:
> On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
> wrote:
> >
> > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose 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).
Richard.
Re: [patch] fix PR lto/95604, -flto and -fcf-protection
On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
wrote:
>
> On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose 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?
--
H.J.
Re: [patch] fix PR lto/95604, -flto and -fcf-protection
On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose 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?
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.
Thanks,
Richard.
> Thanks, Matthias
>
>
[patch] fix PR lto/95604, -flto and -fcf-protection
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?
Thanks, Matthias
PR lto/95604
* lto-wrapper.c (merge_and_complain): Warn about different
values for -fcf-protection.
(append_compiler_options): Pass -fcf-protection option.
* 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 (
+_obstack, _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 (_obstack, _p, "-g");
--- a/src/gcc/lto-wrapper.c
+++ b/src/gcc/lto-wrapper.c
@@ -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;
+
case OPT_O:
case OPT_Ofast:
case OPT_Og:
@@ -645,6 +677,7 @@
case OPT_fopenacc:
case OPT_fopenacc_dim_:
case OPT_foffload_abi_:
+ case OPT_fcf_protection_:
case OPT_g:
case OPT_O:
case OPT_Ofast:
* common.opt (fcf-protection, fcf-protection=): Mark as optimization.
--- a/src/gcc/common.opt
+++ b/src/gcc/common.opt
@@ -1739,10 +1739,10 @@
Inline __atomic operations when a lock free instruction sequence is available.
fcf-protection
-Common RejectNegative Alias(fcf-protection=,full)
+Common Optimization RejectNegative Alias(fcf-protection=,full)
fcf-protection=
-Common Report Joined RejectNegative Enum(cf_protection_level) Var(flag_cf_protection) Init(CF_NONE)
+Common Optimization Report Joined RejectNegative Enum(cf_protection_level) Var(flag_cf_protection) Init(CF_NONE)
-fcf-protection=[full|branch|return|none] Instrument functions with checks to verify jump/call/return control-flow transfer
instructions have valid targets.
