fix PR lto/95604, -flto and -fcf-protection

Message ID c98546f7-0e55-54d7-c401-7195aec66b7f@ubuntu.com
State New
Headers show
Series
  • fix PR lto/95604, -flto and -fcf-protection
Related show

Commit Message

Matthias Klose June 15, 2020, 3:29 p.m.
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

Comments

Andreas Krebbel via Gcc-patches June 17, 2020, 8:59 a.m. | #1
On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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?

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

>

>
Andreas Krebbel via Gcc-patches June 17, 2020, 11:31 a.m. | #2
On Wed, Jun 17, 2020 at 1:59 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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?

-- 
H.J.
Richard Biener June 17, 2020, noon | #3
On Wed, 17 Jun 2020, H.J. Lu wrote:

> On Wed, Jun 17, 2020 at 1:59 AM Richard Biener

> <richard.guenther@gmail.com> wrote:

> >

> > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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).

Richard.
Andreas Krebbel via Gcc-patches June 17, 2020, 12:26 p.m. | #4
On Wed, Jun 17, 2020 at 5:00 AM Richard Biener <rguenther@suse.de> wrote:
>

> On Wed, 17 Jun 2020, H.J. Lu wrote:

>

> > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener

> > <richard.guenther@gmail.com> wrote:

> > >

> > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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.

-- 
H.J.
Richard Biener June 17, 2020, 12:33 p.m. | #5
On Wed, 17 Jun 2020, H.J. Lu wrote:

> On Wed, Jun 17, 2020 at 5:00 AM Richard Biener <rguenther@suse.de> wrote:

> >

> > On Wed, 17 Jun 2020, H.J. Lu wrote:

> >

> > > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener

> > > <richard.guenther@gmail.com> wrote:

> > > >

> > > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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.
Andreas Krebbel via Gcc-patches June 17, 2020, 12:42 p.m. | #6
On Wed, Jun 17, 2020 at 5:33 AM Richard Biener <rguenther@suse.de> wrote:
>

> On Wed, 17 Jun 2020, H.J. Lu wrote:

>

> > On Wed, Jun 17, 2020 at 5:00 AM Richard Biener <rguenther@suse.de> wrote:

> > >

> > > On Wed, 17 Jun 2020, H.J. Lu wrote:

> > >

> > > > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener

> > > > <richard.guenther@gmail.com> wrote:

> > > > >

> > > > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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 .

-- 
H.J.
Richard Biener June 17, 2020, 1:11 p.m. | #7
On Wed, 17 Jun 2020, H.J. Lu wrote:

> On Wed, Jun 17, 2020 at 5:33 AM Richard Biener <rguenther@suse.de> wrote:

> >

> > On Wed, 17 Jun 2020, H.J. Lu wrote:

> >

> > > On Wed, Jun 17, 2020 at 5:00 AM Richard Biener <rguenther@suse.de> wrote:

> > > >

> > > > On Wed, 17 Jun 2020, H.J. Lu wrote:

> > > >

> > > > > On Wed, Jun 17, 2020 at 1:59 AM Richard Biener

> > > > > <richard.guenther@gmail.com> wrote:

> > > > > >

> > > > > > On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Matthias Klose July 13, 2020, 11:32 a.m. | #8
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 <rguenther@suse.de> wrote:

>>>

>>> On Wed, 17 Jun 2020, H.J. Lu wrote:

>>>

>>>> On Wed, Jun 17, 2020 at 5:00 AM Richard Biener <rguenther@suse.de> wrote:

>>>>>

>>>>> On Wed, 17 Jun 2020, H.J. Lu wrote:

>>>>>

>>>>>> On Wed, Jun 17, 2020 at 1:59 AM Richard Biener

>>>>>> <richard.guenther@gmail.com> wrote:

>>>>>>>

>>>>>>> On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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.

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))
Richard Biener July 14, 2020, 6:37 a.m. | #9
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 <rguenther@suse.de> wrote:

> >>>

> >>> On Wed, 17 Jun 2020, H.J. Lu wrote:

> >>>

> >>>> On Wed, Jun 17, 2020 at 5:00 AM Richard Biener <rguenther@suse.de> wrote:

> >>>>>

> >>>>> On Wed, 17 Jun 2020, H.J. Lu wrote:

> >>>>>

> >>>>>> On Wed, Jun 17, 2020 at 1:59 AM Richard Biener

> >>>>>> <richard.guenther@gmail.com> wrote:

> >>>>>>>

> >>>>>>> On Mon, Jun 15, 2020 at 5:30 PM Matthias Klose <doko@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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Patch

	* 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.