| Message ID | c98546f7-0e55-54d7-c401-7195aec66b7f@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix PR lto/95604, -flto and -fcf-protection | expand |
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 > >
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?
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.
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.
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.
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 .
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.
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
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 > >
* 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.