| Message ID | CAFiYyc0=pkpVTSX9W7YZmPo19StCHsPK7PMmO29QmQ8XPCFvxg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Richard Biener [mailto:richard.guenther@gmail.com] > Sent: Tuesday, August 15, 2017 3:43 PM > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> wrote: > > Part#1. Add generic part for Intel CET enabling. > > > > The spec is available at > > > > https://software.intel.com/sites/default/files/managed/4d/2a/control-f > > low-enforcement-technology-preview.pdf > > > > High-level design. > > ------------------ > > > > A proposal is to introduce a target independent flag > > -finstrument-control-flow with a semantic to instrument a code to > > control validness or integrity of control-flow transfers using jump > > and call instructions. The main goal is to detect and block a possible > > malware execution through transfer the execution to unknown target > > address. Implementation could be either software or target based. Any > > target platforms can provide their implementation for instrumentation > > under this option. > > > > When the -finstrument-control-flow flag is set each implementation has > > to check if a support exists for a target platform and report an error > > if no support is found. > > > > The compiler should instrument any control-flow transfer points in a > > program (ex. call/jmp/ret) as well as any landing pads, which are > > targets of for control-flow transfers. > > > > A new 'notrack' attribute is introduced to provide hand tuning support. > > The attribute directs the compiler to skip a call to a function and a > > function's landing pad from instrumentation (tracking). The attribute > > can be used for function and pointer to function types, otherwise it > > will be ignored. The attribute is saved in a type and propagated to a > > GIMPLE call statement and later to a call instruction. > > > > Currently all platforms except i386 will report the error and do no > > instrumentation. i386 will provide the implementation based on a > > specification published by Intel for a new technology called > > Control-flow Enforcement Technology (CET). > > diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d 100644 > --- a/gcc/gimple.c > +++ b/gcc/gimple.c > @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t) > gimple_set_no_warning (call, TREE_NO_WARNING (t)); > gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t)); > > + if (fndecl == NULL_TREE) > + { > + /* Find the type of an indirect call. */ > + tree addr = CALL_EXPR_FN (t); > + if (TREE_CODE (addr) != FUNCTION_DECL) > + { > + tree fntype = TREE_TYPE (addr); > + gcc_assert (POINTER_TYPE_P (fntype)); > + fntype = TREE_TYPE (fntype); > + > + /* Check if its type has the no-track attribute and propagate > + it to the CALL insn. */ > + if (lookup_attribute ("notrack", TYPE_ATTRIBUTES (fntype))) > + gimple_call_set_with_notrack (call, TRUE); > + } > + } > > this means notrack is not recognized if fndecl is not NULL. Note that only the > two callers know the real function type in effect (they call > gimple_call_set_fntype with it). I suggest to pass down that type to > gimple_build_call_from_tree and move the gimple_call_set_fntype call > there as well. And simply use the type for the above. The best way to say is notrack is not propagated if fndecl is not NULL. Fndecl, if not NULL, is a direct call and notrack is not applicable for such calls. I will add a comment before the if. I would like to propose modifying the existing code without changing interfaces. The idea is that at the time the notrack is propagated (the code snippet above) the gimple call was created and the correct type was assigned to the 'call' exactly by gimple_call_set_fntype. My proposal is to get the type out of the gimple 'call' (like gimple_call_fntype) instead of the tree 't'. Is it right? > +static inline bool > +gimple_call_with_notrack_p (const gimple *gs) { > + const gcall *gc = GIMPLE_CHECK2<const gcall *> (gs); > + return gimple_call_with_notrack_p (gc); } > > please do not add gimple * overloads for new APIs, instead make sure to > pass down gcalls at callers. Ok, I will remove. > Please change the names to omit 'with_', thus just notrack and > GF_CALL_NOTRACK. Ok, I will rename. > I think 'notrack' is somewhat unspecific of a name, what prevented you to > use 'nocet'? Actually it's specific. The HW will have a prefix with exactly this name and the same meaning. And I think, what is more important, 'track/notrack' gives better semantic for a user. CET is a name bound with Intel specific technology. > Any idea how to implement a software-based solution efficiently? > Creating a table of valid destination addresses in a special section should be > possible without too much work, am I right in that only indirect control > transfer is checked? Thus CET assumes the code itself cannot be changed > (and thus the stack isn't executable)? Yes, your idea looks right. Microsoft has published the option /guard:cf and according to the description it uses similar idea of gathering information about control-transfer targets. This info is kept in the binary. And 'yes' for last two your questions. Thanks, Igor > Thanks, > Richard.
On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> wrote: >> -----Original Message----- >> From: Richard Biener [mailto:richard.guenther@gmail.com] >> Sent: Tuesday, August 15, 2017 3:43 PM >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling >> >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V >> <igor.v.tsimbalist@intel.com> wrote: >> > Part#1. Add generic part for Intel CET enabling. >> > >> > The spec is available at >> > >> > https://software.intel.com/sites/default/files/managed/4d/2a/control-f >> > low-enforcement-technology-preview.pdf >> > >> > High-level design. >> > ------------------ >> > >> > A proposal is to introduce a target independent flag >> > -finstrument-control-flow with a semantic to instrument a code to >> > control validness or integrity of control-flow transfers using jump >> > and call instructions. The main goal is to detect and block a possible >> > malware execution through transfer the execution to unknown target >> > address. Implementation could be either software or target based. Any >> > target platforms can provide their implementation for instrumentation >> > under this option. >> > >> > When the -finstrument-control-flow flag is set each implementation has >> > to check if a support exists for a target platform and report an error >> > if no support is found. >> > >> > The compiler should instrument any control-flow transfer points in a >> > program (ex. call/jmp/ret) as well as any landing pads, which are >> > targets of for control-flow transfers. >> > >> > A new 'notrack' attribute is introduced to provide hand tuning support. >> > The attribute directs the compiler to skip a call to a function and a >> > function's landing pad from instrumentation (tracking). The attribute >> > can be used for function and pointer to function types, otherwise it >> > will be ignored. The attribute is saved in a type and propagated to a >> > GIMPLE call statement and later to a call instruction. >> > >> > Currently all platforms except i386 will report the error and do no >> > instrumentation. i386 will provide the implementation based on a >> > specification published by Intel for a new technology called >> > Control-flow Enforcement Technology (CET). >> >> diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d 100644 >> --- a/gcc/gimple.c >> +++ b/gcc/gimple.c >> @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t) >> gimple_set_no_warning (call, TREE_NO_WARNING (t)); >> gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t)); >> >> + if (fndecl == NULL_TREE) >> + { >> + /* Find the type of an indirect call. */ >> + tree addr = CALL_EXPR_FN (t); >> + if (TREE_CODE (addr) != FUNCTION_DECL) >> + { >> + tree fntype = TREE_TYPE (addr); >> + gcc_assert (POINTER_TYPE_P (fntype)); >> + fntype = TREE_TYPE (fntype); >> + >> + /* Check if its type has the no-track attribute and propagate >> + it to the CALL insn. */ >> + if (lookup_attribute ("notrack", TYPE_ATTRIBUTES (fntype))) >> + gimple_call_set_with_notrack (call, TRUE); >> + } >> + } >> >> this means notrack is not recognized if fndecl is not NULL. Note that only the >> two callers know the real function type in effect (they call >> gimple_call_set_fntype with it). I suggest to pass down that type to >> gimple_build_call_from_tree and move the gimple_call_set_fntype call >> there as well. And simply use the type for the above. > > The best way to say is notrack is not propagated if fndecl is not NULL. Fndecl, if not NULL, is a direct call and notrack is not applicable for such calls. I will add a comment before the if. Hmm. But what does this mean? I guess direct calls are always 'notrack' then and thus we're fine to ignore it. > I would like to propose modifying the existing code without changing interfaces. The idea is that at the time the notrack is propagated (the code snippet above) the gimple call was created and the correct type was assigned to the 'call' exactly by gimple_call_set_fntype. My proposal is to get the type out of the gimple 'call' (like gimple_call_fntype) instead of the tree 't'. Is it right? Yes, but note that the type on the gimple 'call' isn't yet correctly set (only the caller does that). The gimple_build_call_from_tree is really an ugly thing and it should be privatized inside the gimplifier... >> +static inline bool >> +gimple_call_with_notrack_p (const gimple *gs) { >> + const gcall *gc = GIMPLE_CHECK2<const gcall *> (gs); >> + return gimple_call_with_notrack_p (gc); } >> >> please do not add gimple * overloads for new APIs, instead make sure to >> pass down gcalls at callers. > > Ok, I will remove. > >> Please change the names to omit 'with_', thus just notrack and >> GF_CALL_NOTRACK. > > Ok, I will rename. > >> I think 'notrack' is somewhat unspecific of a name, what prevented you to >> use 'nocet'? > > Actually it's specific. The HW will have a prefix with exactly this name and the same meaning. And I think, what is more important, 'track/notrack' gives better semantic for a user. CET is a name bound with Intel specific technology. But 'tracking' something is quite unspecific. Tracking for what? 'no_verify_cf' (aka do not verify control flow) maybe? >> Any idea how to implement a software-based solution efficiently? >> Creating a table of valid destination addresses in a special section should be >> possible without too much work, am I right in that only indirect control >> transfer is checked? Thus CET assumes the code itself cannot be changed >> (and thus the stack isn't executable)? > > Yes, your idea looks right. Microsoft has published the option /guard:cf and according to the description it uses similar idea of gathering information about control-transfer targets. This info is kept in the binary. And 'yes' for last two your questions. Ok, so if we generate a special section with say 'cettable' as symbol the verification runtime would be invoked at each indirect call site with the destination address and it would search the table and abort if the destination is not recorded. We probably have to support a set of CET tables and register them to support shared libraries (and cross shared object indirect calls). Do you plan to contribute sth like this? Maybe within the sanitizer framework? Thanks, Richard. > Thanks, > Igor > >> Thanks, >> Richard.
> -----Original Message----- > From: Richard Biener [mailto:richard.guenther@gmail.com] > Sent: Friday, August 18, 2017 3:53 PM > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> wrote: > >> -----Original Message----- > >> From: Richard Biener [mailto:richard.guenther@gmail.com] > >> Sent: Tuesday, August 15, 2017 3:43 PM > >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > >> Cc: gcc-patches@gcc.gnu.org > >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V > >> <igor.v.tsimbalist@intel.com> wrote: > >> > Part#1. Add generic part for Intel CET enabling. > >> > > >> > The spec is available at > >> > > >> > https://software.intel.com/sites/default/files/managed/4d/2a/contro > >> > l-f low-enforcement-technology-preview.pdf > >> > > >> > High-level design. > >> > ------------------ > >> > > >> > A proposal is to introduce a target independent flag > >> > -finstrument-control-flow with a semantic to instrument a code to > >> > control validness or integrity of control-flow transfers using jump > >> > and call instructions. The main goal is to detect and block a > >> > possible malware execution through transfer the execution to > >> > unknown target address. Implementation could be either software or > >> > target based. Any target platforms can provide their implementation > >> > for instrumentation under this option. > >> > > >> > When the -finstrument-control-flow flag is set each implementation > >> > has to check if a support exists for a target platform and report > >> > an error if no support is found. > >> > > >> > The compiler should instrument any control-flow transfer points in > >> > a program (ex. call/jmp/ret) as well as any landing pads, which are > >> > targets of for control-flow transfers. > >> > > >> > A new 'notrack' attribute is introduced to provide hand tuning support. > >> > The attribute directs the compiler to skip a call to a function and > >> > a function's landing pad from instrumentation (tracking). The > >> > attribute can be used for function and pointer to function types, > >> > otherwise it will be ignored. The attribute is saved in a type and > >> > propagated to a GIMPLE call statement and later to a call instruction. > >> > > >> > Currently all platforms except i386 will report the error and do no > >> > instrumentation. i386 will provide the implementation based on a > >> > specification published by Intel for a new technology called > >> > Control-flow Enforcement Technology (CET). > >> > >> diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d > >> 100644 > >> --- a/gcc/gimple.c > >> +++ b/gcc/gimple.c > >> @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t) > >> gimple_set_no_warning (call, TREE_NO_WARNING (t)); > >> gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t)); > >> > >> + if (fndecl == NULL_TREE) > >> + { > >> + /* Find the type of an indirect call. */ > >> + tree addr = CALL_EXPR_FN (t); > >> + if (TREE_CODE (addr) != FUNCTION_DECL) > >> + { > >> + tree fntype = TREE_TYPE (addr); > >> + gcc_assert (POINTER_TYPE_P (fntype)); > >> + fntype = TREE_TYPE (fntype); > >> + > >> + /* Check if its type has the no-track attribute and propagate > >> + it to the CALL insn. */ > >> + if (lookup_attribute ("notrack", TYPE_ATTRIBUTES (fntype))) > >> + gimple_call_set_with_notrack (call, TRUE); > >> + } > >> + } > >> > >> this means notrack is not recognized if fndecl is not NULL. Note > >> that only the two callers know the real function type in effect (they > >> call gimple_call_set_fntype with it). I suggest to pass down that > >> type to gimple_build_call_from_tree and move the > >> gimple_call_set_fntype call there as well. And simply use the type for the > above. > > > > The best way to say is notrack is not propagated if fndecl is not NULL. > Fndecl, if not NULL, is a direct call and notrack is not applicable for such calls. I > will add a comment before the if. > > Hmm. But what does this mean? I guess direct calls are always 'notrack' then > and thus we're fine to ignore it. Yes, that's correct - direct calls are always 'notrack' and we are ignoring it in calls. > > I would like to propose modifying the existing code without changing > interfaces. The idea is that at the time the notrack is propagated (the code > snippet above) the gimple call was created and the correct type was assigned > to the 'call' exactly by gimple_call_set_fntype. My proposal is to get the type > out of the gimple 'call' (like gimple_call_fntype) instead of the tree 't'. Is it > right? > > Yes, but note that the type on the gimple 'call' isn't yet correctly set (only the > caller does that). The gimple_build_call_from_tree is really an ugly thing and > it should be privatized inside the gimplifier... I'll look into this more carefully. It looks like it has to be rewritten as you suggested. > >> +static inline bool > >> +gimple_call_with_notrack_p (const gimple *gs) { > >> + const gcall *gc = GIMPLE_CHECK2<const gcall *> (gs); > >> + return gimple_call_with_notrack_p (gc); } > >> > >> please do not add gimple * overloads for new APIs, instead make sure > >> to pass down gcalls at callers. > > > > Ok, I will remove. > > > >> Please change the names to omit 'with_', thus just notrack and > >> GF_CALL_NOTRACK. > > > > Ok, I will rename. > > > >> I think 'notrack' is somewhat unspecific of a name, what prevented > >> you to use 'nocet'? > > > > Actually it's specific. The HW will have a prefix with exactly this name and > the same meaning. And I think, what is more important, 'track/notrack' gives > better semantic for a user. CET is a name bound with Intel specific > technology. > > But 'tracking' something is quite unspecific. Tracking for what? > 'no_verify_cf' (aka do not verify control flow) maybe? The name just has to suggest the right semantic. 'no_verify_cf' is good, let's use it unless different name appears. > >> Any idea how to implement a software-based solution efficiently? > >> Creating a table of valid destination addresses in a special section > >> should be possible without too much work, am I right in that only > >> indirect control transfer is checked? Thus CET assumes the code > >> itself cannot be changed (and thus the stack isn't executable)? > > > > Yes, your idea looks right. Microsoft has published the option /guard:cf and > according to the description it uses similar idea of gathering information > about control-transfer targets. This info is kept in the binary. And 'yes' for last > two your questions. > > Ok, so if we generate a special section with say 'cettable' as symbol the > verification runtime would be invoked at each indirect call site with the > destination address and it would search the table and abort if the destination > is not recorded. We probably have to support a set of CET tables and register > them to support shared libraries (and cross shared object indirect calls). > > Do you plan to contribute sth like this? Maybe within the sanitizer > framework? I do not have plans for any sw based implementation. My plan is to introduce the concept in gcc and make its enabling for Intel CET. Even with my current plans more work needs to be done to completely enable CET. Thanks, Igor > Thanks, > Richard. > > > Thanks, > > Igor > > > >> Thanks, > >> Richard.
On 08/15/2017 07:42 AM, Richard Biener wrote: > > Please change the names to omit 'with_', thus just notrack > and GF_CALL_NOTRACK. > > I think 'notrack' is somewhat unspecific of a name, what prevented > you to use 'nocet'? I think we should look for something better than notrack. I think "control flow enforcement/CFE" is commonly used for this stuff. CET is an Intel marketing name IIRC. The tracking is for indirect branch/call targets. So some combination of cfe, branch/call and track should be sufficient. > Any idea how to implement a software-based solution efficiently? > Creating a table of valid destination addresses in a special section > should be possible without too much work, am I right in that only > indirect control transfer is checked? Thus CET assumes the > code itself cannot be changed (and thus the stack isn't executable)? Well, there's two broad areas that have to be addressed. First you need to separate the call stack from the rest of the call frame, or at least the parts of the call frame that are potentially vulnerable to overruns. LLVM has some code to do this. Essentially any object in the stack that is not proven to be safely accessed gets put into a separate stack. That roughly duplicates the shadow stack capability. I think their implementation is just x86 and IIRC doesn't work in some circumstances -- I'd consider it a proof of concept, not something ready for production use. Bernd and I also spec'd a couple more approaches to protect the return address. Essentially, the return address turns into a cookie that a particular caller can use to lookup/map to a real return address. We didn't take any of this to completion because it was pretty clear the ROP mitigation landscape was going to change and make software only solutions less appealing. Second you need the indirect branch/call tracking. I spec'd something out in this space with Intel's engineers years ago. Essentially building tables of valid targets for indirect branches and checking instrumentation. You can have a global table, per-DSO tables or you can have a per-branch table. It gets a little hairy in mixed mode environments. But it basically works how you'd expect. Indirect branches/calls turn into something considerably more complex as do the branch/call targets if you have access to something like a last-taken-branch. Jeff
> -----Original Message----- > From: Tsimbalist, Igor V > Sent: Friday, August 18, 2017 4:43 PM > To: 'Richard Biener' <richard.guenther@gmail.com> > Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > -----Original Message----- > > From: Richard Biener [mailto:richard.guenther@gmail.com] > > Sent: Friday, August 18, 2017 3:53 PM > > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > > Cc: gcc-patches@gcc.gnu.org > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V > > <igor.v.tsimbalist@intel.com> wrote: > > >> -----Original Message----- > > >> From: Richard Biener [mailto:richard.guenther@gmail.com] > > >> Sent: Tuesday, August 15, 2017 3:43 PM > > >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > > >> Cc: gcc-patches@gcc.gnu.org > > >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > >> > > >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V > > >> <igor.v.tsimbalist@intel.com> wrote: > > >> > Part#1. Add generic part for Intel CET enabling. > > >> > > > >> > The spec is available at > > >> > > > >> > https://software.intel.com/sites/default/files/managed/4d/2a/cont > > >> > ro l-f low-enforcement-technology-preview.pdf > > >> > > > >> > High-level design. > > >> > ------------------ > > >> > > > >> > A proposal is to introduce a target independent flag > > >> > -finstrument-control-flow with a semantic to instrument a code to > > >> > control validness or integrity of control-flow transfers using > > >> > jump and call instructions. The main goal is to detect and block > > >> > a possible malware execution through transfer the execution to > > >> > unknown target address. Implementation could be either software > > >> > or target based. Any target platforms can provide their > > >> > implementation for instrumentation under this option. > > >> > > > >> > When the -finstrument-control-flow flag is set each > > >> > implementation has to check if a support exists for a target > > >> > platform and report an error if no support is found. > > >> > > > >> > The compiler should instrument any control-flow transfer points > > >> > in a program (ex. call/jmp/ret) as well as any landing pads, > > >> > which are targets of for control-flow transfers. > > >> > > > >> > A new 'notrack' attribute is introduced to provide hand tuning > support. > > >> > The attribute directs the compiler to skip a call to a function > > >> > and a function's landing pad from instrumentation (tracking). The > > >> > attribute can be used for function and pointer to function types, > > >> > otherwise it will be ignored. The attribute is saved in a type > > >> > and propagated to a GIMPLE call statement and later to a call > instruction. > > >> > > > >> > Currently all platforms except i386 will report the error and do > > >> > no instrumentation. i386 will provide the implementation based on > > >> > a specification published by Intel for a new technology called > > >> > Control-flow Enforcement Technology (CET). > > >> > > >> diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d > > >> 100644 > > >> --- a/gcc/gimple.c > > >> +++ b/gcc/gimple.c > > >> @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t) > > >> gimple_set_no_warning (call, TREE_NO_WARNING (t)); > > >> gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t)); > > >> > > >> + if (fndecl == NULL_TREE) > > >> + { > > >> + /* Find the type of an indirect call. */ > > >> + tree addr = CALL_EXPR_FN (t); > > >> + if (TREE_CODE (addr) != FUNCTION_DECL) > > >> + { > > >> + tree fntype = TREE_TYPE (addr); > > >> + gcc_assert (POINTER_TYPE_P (fntype)); > > >> + fntype = TREE_TYPE (fntype); > > >> + > > >> + /* Check if its type has the no-track attribute and propagate > > >> + it to the CALL insn. */ > > >> + if (lookup_attribute ("notrack", TYPE_ATTRIBUTES (fntype))) > > >> + gimple_call_set_with_notrack (call, TRUE); > > >> + } > > >> + } > > >> > > >> this means notrack is not recognized if fndecl is not NULL. Note > > >> that only the two callers know the real function type in effect > > >> (they call gimple_call_set_fntype with it). I suggest to pass down > > >> that type to gimple_build_call_from_tree and move the > > >> gimple_call_set_fntype call there as well. And simply use the type > > >> for the > > above. > > > > > > The best way to say is notrack is not propagated if fndecl is not NULL. > > Fndecl, if not NULL, is a direct call and notrack is not applicable > > for such calls. I will add a comment before the if. > > > > Hmm. But what does this mean? I guess direct calls are always > > 'notrack' then and thus we're fine to ignore it. > > Yes, that's correct - direct calls are always 'notrack' and we are ignoring it in > calls. > > > > I would like to propose modifying the existing code without changing > > interfaces. The idea is that at the time the notrack is propagated > > (the code snippet above) the gimple call was created and the correct > > type was assigned to the 'call' exactly by gimple_call_set_fntype. My > > proposal is to get the type out of the gimple 'call' (like > > gimple_call_fntype) instead of the tree 't'. Is it right? > > > > Yes, but note that the type on the gimple 'call' isn't yet correctly > > set (only the caller does that). The gimple_build_call_from_tree is > > really an ugly thing and it should be privatized inside the gimplifier... > > I'll look into this more carefully. It looks like it has to be rewritten as you > suggested. I've rewrote the code as you suggested. There are couple calls to gimple_build_call_from_tree from c/gimple-parser.c. As I introduced an additional argument in gimple_build_call_from_tree I pass NULL argument for these cases and check for NULL inside gimple_build_call_from_tree. > > >> +static inline bool > > >> +gimple_call_with_notrack_p (const gimple *gs) { > > >> + const gcall *gc = GIMPLE_CHECK2<const gcall *> (gs); > > >> + return gimple_call_with_notrack_p (gc); } > > >> > > >> please do not add gimple * overloads for new APIs, instead make > > >> sure to pass down gcalls at callers. > > > > > > Ok, I will remove. Fixed. > > >> Please change the names to omit 'with_', thus just notrack and > > >> GF_CALL_NOTRACK. > > > > > > Ok, I will rename. Fixed. > > >> I think 'notrack' is somewhat unspecific of a name, what prevented > > >> you to use 'nocet'? > > > > > > Actually it's specific. The HW will have a prefix with exactly this > > > name and > > the same meaning. And I think, what is more important, 'track/notrack' > > gives better semantic for a user. CET is a name bound with Intel > > specific technology. > > > > But 'tracking' something is quite unspecific. Tracking for what? > > 'no_verify_cf' (aka do not verify control flow) maybe? > > The name just has to suggest the right semantic. 'no_verify_cf' is good, let's > use it unless different name appears. I have renamed all newly introduced function and macro names to use 'noverify_cf'. But I still keep the attribute name as 'notrack'. Historically the attribute name follows the public CET specification, which uses 'no-track prefix' wording. Is it ok to keep such attribute name? Thanks, Igor > > >> Any idea how to implement a software-based solution efficiently? > > >> Creating a table of valid destination addresses in a special > > >> section should be possible without too much work, am I right in > > >> that only indirect control transfer is checked? Thus CET assumes > > >> the code itself cannot be changed (and thus the stack isn't executable)? > > > > > > Yes, your idea looks right. Microsoft has published the option > > > /guard:cf and > > according to the description it uses similar idea of gathering > > information about control-transfer targets. This info is kept in the > > binary. And 'yes' for last two your questions. > > > > Ok, so if we generate a special section with say 'cettable' as symbol > > the verification runtime would be invoked at each indirect call site > > with the destination address and it would search the table and abort > > if the destination is not recorded. We probably have to support a set > > of CET tables and register them to support shared libraries (and cross > shared object indirect calls). > > > > Do you plan to contribute sth like this? Maybe within the sanitizer > > framework? > > I do not have plans for any sw based implementation. My plan is to introduce > the concept in gcc and make its enabling for Intel CET. Even with my current > plans more work needs to be done to completely enable CET. > > Thanks, > Igor > > > Thanks, > > Richard. > > > > > Thanks, > > > Igor > > > > > >> Thanks, > > >> Richard.
> -----Original Message----- > From: Jeff Law [mailto:law@redhat.com] > Sent: Friday, August 25, 2017 10:32 PM > To: Richard Biener <richard.guenther@gmail.com>; Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On 08/15/2017 07:42 AM, Richard Biener wrote: > > > > Please change the names to omit 'with_', thus just notrack and > > GF_CALL_NOTRACK. > > > > I think 'notrack' is somewhat unspecific of a name, what prevented you > > to use 'nocet'? > I think we should look for something better than notrack. I think "control > flow enforcement/CFE" is commonly used for this stuff. CET is an Intel > marketing name IIRC. > > The tracking is for indirect branch/call targets. So some combination of cfe, > branch/call and track should be sufficient. Still remaining question from me - is it ok to use 'notrack' as the attribute name. I've asked Richard about this in this thread. Thanks, Igor > > > Any idea how to implement a software-based solution efficiently? > > Creating a table of valid destination addresses in a special section > > should be possible without too much work, am I right in that only > > indirect control transfer is checked? Thus CET assumes the code > > itself cannot be changed (and thus the stack isn't executable)? > Well, there's two broad areas that have to be addressed. > > First you need to separate the call stack from the rest of the call frame, or at > least the parts of the call frame that are potentially vulnerable to overruns. > LLVM has some code to do this. Essentially any object in the stack that is not > proven to be safely accessed gets put into a separate stack. That roughly > duplicates the shadow stack capability. I think their implementation is just > x86 and IIRC doesn't work in some circumstances -- I'd consider it a proof of > concept, not something ready for production use. > > > Bernd and I also spec'd a couple more approaches to protect the return > address. Essentially, the return address turns into a cookie that a particular > caller can use to lookup/map to a real return address. We didn't take any of > this to completion because it was pretty clear the ROP mitigation landscape > was going to change and make software only solutions less appealing. > > Second you need the indirect branch/call tracking. I spec'd something out in > this space with Intel's engineers years ago. Essentially building tables of valid > targets for indirect branches and checking > instrumentation. You can have a global table, per-DSO tables or you > can have a per-branch table. It gets a little hairy in mixed mode > environments. But it basically works how you'd expect. Indirect > branches/calls turn into something considerably more complex as do the > branch/call targets if you have access to something like a last-taken-branch. > > Jeff
On 09/12/2017 09:40 AM, Tsimbalist, Igor V wrote: > >> -----Original Message----- >> From: Jeff Law [mailto:law@redhat.com] >> Sent: Friday, August 25, 2017 10:32 PM >> To: Richard Biener <richard.guenther@gmail.com>; Tsimbalist, Igor V >> <igor.v.tsimbalist@intel.com> >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling >> >> On 08/15/2017 07:42 AM, Richard Biener wrote: >>> >>> Please change the names to omit 'with_', thus just notrack and >>> GF_CALL_NOTRACK. >>> >>> I think 'notrack' is somewhat unspecific of a name, what prevented you >>> to use 'nocet'? >> I think we should look for something better than notrack. I think "control >> flow enforcement/CFE" is commonly used for this stuff. CET is an Intel >> marketing name IIRC. >> >> The tracking is for indirect branch/call targets. So some combination of cfe, >> branch/call and track should be sufficient. > Still remaining question from me - is it ok to use 'notrack' as the attribute name. I've asked Richard > about this in this thread. I tend to agree with Richi that "track" is a bit too generic. no_cfe might be better. Or no_cfi, but cfi is commonly used to represent call-frame-info :-) jeff
> -----Original Message----- > From: Tsimbalist, Igor V > Sent: Tuesday, September 12, 2017 5:35 PM > To: 'Richard Biener' <richard.guenther@gmail.com> > Cc: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org>; Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > -----Original Message----- > > From: Tsimbalist, Igor V > > Sent: Friday, August 18, 2017 4:43 PM > > To: 'Richard Biener' <richard.guenther@gmail.com> > > Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V > > <igor.v.tsimbalist@intel.com> > > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > > -----Original Message----- > > > From: Richard Biener [mailto:richard.guenther@gmail.com] > > > Sent: Friday, August 18, 2017 3:53 PM > > > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > > > Cc: gcc-patches@gcc.gnu.org > > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > > > On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V > > > <igor.v.tsimbalist@intel.com> wrote: > > > >> -----Original Message----- > > > >> From: Richard Biener [mailto:richard.guenther@gmail.com] > > > >> Sent: Tuesday, August 15, 2017 3:43 PM > > > >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > > > >> Cc: gcc-patches@gcc.gnu.org > > > >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > >> > > > >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V > > > >> <igor.v.tsimbalist@intel.com> wrote: > > > >> > Part#1. Add generic part for Intel CET enabling. > > > >> > > > > >> > The spec is available at > > > >> > > > > >> > https://software.intel.com/sites/default/files/managed/4d/2a/co > > > >> > nt ro l-f low-enforcement-technology-preview.pdf <..skipped..> > > > >> I think 'notrack' is somewhat unspecific of a name, what > > > >> prevented you to use 'nocet'? > > > > > > > > Actually it's specific. The HW will have a prefix with exactly > > > > this name and > > > the same meaning. And I think, what is more important, 'track/notrack' > > > gives better semantic for a user. CET is a name bound with Intel > > > specific technology. > > > > > > But 'tracking' something is quite unspecific. Tracking for what? > > > 'no_verify_cf' (aka do not verify control flow) maybe? > > > > The name just has to suggest the right semantic. 'no_verify_cf' is > > good, let's use it unless different name appears. > I have renamed all newly introduced function and macro names to use > 'noverify_cf'. But I still keep the attribute name as 'notrack'. Historically the > attribute name follows the public CET specification, which uses 'no-track > prefix' wording. Is it ok to keep such attribute name? Here is an updated proposal about option name and attribute name. The new option has values to let a user to choose what control-flow protection to activate. -fcf-protection=[full|branch|return|none] branch - do control-flow protection for indirect jumps and calls return - do control-flow protection for function returns full - alias to specify both branch + return none - turn off protection. This value is needed when/if cf-protection is turned on by default by driver in future Attribute name is the most tough one. Here are several names to evaluate: 'nocf_verify' or 'nocf_check', or to be more specific and to mimic option name 'nocf_branch_verify' or 'nocf_branch_check'. I would prefer 'nocf_check' as it applies to functions and function pointers so it's definitely related to a branch and it's a smaller one. If you ok with the new proposal I'll implement it in a general parts (code, documentation and tests) and resend these patches for review. Thanks, Igor
On Fri, Sep 15, 2017 at 1:12 PM, Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> wrote: >> -----Original Message----- >> From: Tsimbalist, Igor V >> Sent: Tuesday, September 12, 2017 5:35 PM >> To: 'Richard Biener' <richard.guenther@gmail.com> >> Cc: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org>; Tsimbalist, Igor V >> <igor.v.tsimbalist@intel.com> >> Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling >> >> > -----Original Message----- >> > From: Tsimbalist, Igor V >> > Sent: Friday, August 18, 2017 4:43 PM >> > To: 'Richard Biener' <richard.guenther@gmail.com> >> > Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V >> > <igor.v.tsimbalist@intel.com> >> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling >> > >> > > -----Original Message----- >> > > From: Richard Biener [mailto:richard.guenther@gmail.com] >> > > Sent: Friday, August 18, 2017 3:53 PM >> > > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> >> > > Cc: gcc-patches@gcc.gnu.org >> > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling >> > > >> > > On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V >> > > <igor.v.tsimbalist@intel.com> wrote: >> > > >> -----Original Message----- >> > > >> From: Richard Biener [mailto:richard.guenther@gmail.com] >> > > >> Sent: Tuesday, August 15, 2017 3:43 PM >> > > >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> >> > > >> Cc: gcc-patches@gcc.gnu.org >> > > >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling >> > > >> >> > > >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V >> > > >> <igor.v.tsimbalist@intel.com> wrote: >> > > >> > Part#1. Add generic part for Intel CET enabling. >> > > >> > >> > > >> > The spec is available at >> > > >> > >> > > >> > https://software.intel.com/sites/default/files/managed/4d/2a/co >> > > >> > nt ro l-f low-enforcement-technology-preview.pdf > > <..skipped..> > >> > > >> I think 'notrack' is somewhat unspecific of a name, what >> > > >> prevented you to use 'nocet'? >> > > > >> > > > Actually it's specific. The HW will have a prefix with exactly >> > > > this name and >> > > the same meaning. And I think, what is more important, 'track/notrack' >> > > gives better semantic for a user. CET is a name bound with Intel >> > > specific technology. >> > > >> > > But 'tracking' something is quite unspecific. Tracking for what? >> > > 'no_verify_cf' (aka do not verify control flow) maybe? >> > >> > The name just has to suggest the right semantic. 'no_verify_cf' is >> > good, let's use it unless different name appears. >> I have renamed all newly introduced function and macro names to use >> 'noverify_cf'. But I still keep the attribute name as 'notrack'. Historically the >> attribute name follows the public CET specification, which uses 'no-track >> prefix' wording. Is it ok to keep such attribute name? > > Here is an updated proposal about option name and attribute name. > > The new option has values to let a user to choose what control-flow protection to activate. > > -fcf-protection=[full|branch|return|none] > branch - do control-flow protection for indirect jumps and calls > return - do control-flow protection for function returns > full - alias to specify both branch + return > none - turn off protection. This value is needed when/if cf-protection is turned on by default by driver in future > > Attribute name is the most tough one. Here are several names to evaluate: 'nocf_verify' or 'nocf_check', or to be more specific and to mimic option name 'nocf_branch_verify' or 'nocf_branch_check'. I would prefer 'nocf_check' as it applies to functions and function pointers so it's definitely related to a branch and it's a smaller one. > > If you ok with the new proposal I'll implement it in a general parts (code, documentation and tests) and resend these patches for review. nocf_check sounds fine to me. Richard. > Thanks, > Igor >
Here is an updated patch (version #2). The main differences are: - Change attribute and option names; - Add additional parameter to gimple_build_call_from_tree by adding a type parameter and use it 'nocf_check' attribute propagation; - Reimplement fixes in expand_call_stmt to propagate 'nocf_check' attribute; - Consider 'nocf_check' attribute in Identical Code Folding (ICF) optimization; - Add warning for type inconsistency regarding 'nocf_check' attribute; - Many small fixes; gcc/c-family/ * c-attribs.c (handle_nocf_check_attribute): New function. (c_common_attribute_table): Add 'nocf_check' handling. * c-common.c (check_missing_format_attribute): New function. * c-common.h: Likewise. gcc/c/ * c-typeck.c (convert_for_assignment): Add check for nocf_check attribute. * gimple-parser.c: Add second argument NULL to gimple_build_call_from_tree. gcc/cp/ * typeck.c (convert_for_assignment): Add check for nocf_check attribute. gcc/ * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOCF_CHECK for call insn. * combine.c (distribute_notes): Add REG_CALL_NOCF_CHECK handling. * common.opt: Add fcf-protection flag. * emit-rtl.c (try_split): Add REG_CALL_NOCF_CHECK handling. * flag-types.h: Add enum cf_protection_level. * gimple.c (gimple_build_call_from_tree): Add second parameter. Add 'nocf_check' attribute propagation to gimple call. * gimple.h (gf_mask): Add GF_CALL_NOCF_CHECK. (gimple_call_nocf_check_p): New function. (gimple_call_set_nocf_check): Likewise. * gimplify.c: Add second argument to gimple_build_call_from_tree. * ipa-icf.c: Add nocf_check attribute in statement hash. * recog.c (peep2_attempt): Add REG_CALL_NOCF_CHECK handling. * reg-notes.def: Add REG_NOTE (CALL_NOCF_CHECK). * toplev.c (process_options): Add flag_cf_protection handling. Is it ok for trunk? Thanks, Igor > -----Original Message----- > From: Richard Biener [mailto:richard.guenther@gmail.com] > Sent: Friday, September 15, 2017 2:14 PM > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On Fri, Sep 15, 2017 at 1:12 PM, Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> wrote: > >> -----Original Message----- > >> From: Tsimbalist, Igor V > >> Sent: Tuesday, September 12, 2017 5:35 PM > >> To: 'Richard Biener' <richard.guenther@gmail.com> > >> Cc: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org>; Tsimbalist, > >> Igor V <igor.v.tsimbalist@intel.com> > >> Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > >> > -----Original Message----- > >> > From: Tsimbalist, Igor V > >> > Sent: Friday, August 18, 2017 4:43 PM > >> > To: 'Richard Biener' <richard.guenther@gmail.com> > >> > Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V > >> > <igor.v.tsimbalist@intel.com> > >> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > > >> > > -----Original Message----- > >> > > From: Richard Biener [mailto:richard.guenther@gmail.com] > >> > > Sent: Friday, August 18, 2017 3:53 PM > >> > > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > >> > > Cc: gcc-patches@gcc.gnu.org > >> > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > > > >> > > On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V > >> > > <igor.v.tsimbalist@intel.com> wrote: > >> > > >> -----Original Message----- > >> > > >> From: Richard Biener [mailto:richard.guenther@gmail.com] > >> > > >> Sent: Tuesday, August 15, 2017 3:43 PM > >> > > >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > >> > > >> Cc: gcc-patches@gcc.gnu.org > >> > > >> Subject: Re: > >> > > >> 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > > >> > >> > > >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V > >> > > >> <igor.v.tsimbalist@intel.com> wrote: > >> > > >> > Part#1. Add generic part for Intel CET enabling. > >> > > >> > > >> > > >> > The spec is available at > >> > > >> > > >> > > >> > https://software.intel.com/sites/default/files/managed/4d/2a > >> > > >> > /co nt ro l-f low-enforcement-technology-preview.pdf > > > > <..skipped..> > > > >> > > >> I think 'notrack' is somewhat unspecific of a name, what > >> > > >> prevented you to use 'nocet'? > >> > > > > >> > > > Actually it's specific. The HW will have a prefix with exactly > >> > > > this name and > >> > > the same meaning. And I think, what is more important, > 'track/notrack' > >> > > gives better semantic for a user. CET is a name bound with Intel > >> > > specific technology. > >> > > > >> > > But 'tracking' something is quite unspecific. Tracking for what? > >> > > 'no_verify_cf' (aka do not verify control flow) maybe? > >> > > >> > The name just has to suggest the right semantic. 'no_verify_cf' is > >> > good, let's use it unless different name appears. > >> I have renamed all newly introduced function and macro names to use > >> 'noverify_cf'. But I still keep the attribute name as 'notrack'. > >> Historically the attribute name follows the public CET specification, > >> which uses 'no-track prefix' wording. Is it ok to keep such attribute name? > > > > Here is an updated proposal about option name and attribute name. > > > > The new option has values to let a user to choose what control-flow > protection to activate. > > > > -fcf-protection=[full|branch|return|none] > > branch - do control-flow protection for indirect jumps and calls > > return - do control-flow protection for function returns > > full - alias to specify both branch + return > > none - turn off protection. This value is needed when/if > > cf-protection is turned on by default by driver in future > > > > Attribute name is the most tough one. Here are several names to evaluate: > 'nocf_verify' or 'nocf_check', or to be more specific and to mimic option > name 'nocf_branch_verify' or 'nocf_branch_check'. I would prefer > 'nocf_check' as it applies to functions and function pointers so it's definitely > related to a branch and it's a smaller one. > > > > If you ok with the new proposal I'll implement it in a general parts (code, > documentation and tests) and resend these patches for review. > > nocf_check sounds fine to me. > > Richard. > > > Thanks, > > Igor > >
On 09/19/2017 07:39 AM, Tsimbalist, Igor V wrote: > Here is an updated patch (version #2). The main differences are: > > - Change attribute and option names; > - Add additional parameter to gimple_build_call_from_tree by adding a type parameter and > use it 'nocf_check' attribute propagation; > - Reimplement fixes in expand_call_stmt to propagate 'nocf_check' attribute; > - Consider 'nocf_check' attribute in Identical Code Folding (ICF) optimization; > - Add warning for type inconsistency regarding 'nocf_check' attribute; > - Many small fixes; > > gcc/c-family/ > * c-attribs.c (handle_nocf_check_attribute): New function. > (c_common_attribute_table): Add 'nocf_check' handling. > * c-common.c (check_missing_format_attribute): New function. > * c-common.h: Likewise. > > gcc/c/ > * c-typeck.c (convert_for_assignment): Add check for nocf_check > attribute. > * gimple-parser.c: Add second argument NULL to > gimple_build_call_from_tree. > > gcc/cp/ > * typeck.c (convert_for_assignment): Add check for nocf_check > attribute. > > gcc/ > * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOCF_CHECK for > call insn. > * combine.c (distribute_notes): Add REG_CALL_NOCF_CHECK handling. > * common.opt: Add fcf-protection flag. > * emit-rtl.c (try_split): Add REG_CALL_NOCF_CHECK handling. > * flag-types.h: Add enum cf_protection_level. > * gimple.c (gimple_build_call_from_tree): Add second parameter. > Add 'nocf_check' attribute propagation to gimple call. > * gimple.h (gf_mask): Add GF_CALL_NOCF_CHECK. > (gimple_call_nocf_check_p): New function. > (gimple_call_set_nocf_check): Likewise. > * gimplify.c: Add second argument to gimple_build_call_from_tree. > * ipa-icf.c: Add nocf_check attribute in statement hash. > * recog.c (peep2_attempt): Add REG_CALL_NOCF_CHECK handling. > * reg-notes.def: Add REG_NOTE (CALL_NOCF_CHECK). > * toplev.c (process_options): Add flag_cf_protection handling. > > Is it ok for trunk? > > Thanks, > Igor > > > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > index 0337537..77d1909 100644 > --- a/gcc/c-family/c-attribs.c > +++ b/gcc/c-family/c-attribs.c > @@ -65,6 +65,7 @@ static tree handle_asan_odr_indicator_attribute (tree *, tree, tree, int, > static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *); > static tree handle_noinline_attribute (tree *, tree, tree, int, bool *); > static tree handle_noclone_attribute (tree *, tree, tree, int, bool *); > +static tree handle_nocf_check_attribute (tree *, tree, tree, int, bool *); > static tree handle_noicf_attribute (tree *, tree, tree, int, bool *); > static tree handle_noipa_attribute (tree *, tree, tree, int, bool *); > static tree handle_leaf_attribute (tree *, tree, tree, int, bool *); > @@ -367,6 +368,8 @@ const struct attribute_spec c_common_attribute_table[] = > { "patchable_function_entry", 1, 2, true, false, false, > handle_patchable_function_entry_attribute, > false }, > + { "nocf_check", 0, 0, false, true, true, > + handle_nocf_check_attribute, false }, > { NULL, 0, 0, false, false, false, NULL, false } > }; > > @@ -783,6 +786,26 @@ handle_noclone_attribute (tree *node, tree name, > return NULL_TREE; > } > > +/* Handle a "nocf_check" attribute; arguments as in > + struct attribute_spec.handler. */ > + > +static tree > +handle_nocf_check_attribute (tree *node, tree name, > + tree ARG_UNUSED (args), > + int ARG_UNUSED (flags), bool *no_add_attrs) > +{ > + if (TREE_CODE (*node) != FUNCTION_TYPE > + && TREE_CODE (*node) != METHOD_TYPE > + && TREE_CODE (*node) != FIELD_DECL > + && TREE_CODE (*node) != TYPE_DECL) So curious, is this needed for FIELD_DECL and TYPE_DECL? ISTM the attribute is applied to function/method types. If we do need to handle FIELD_DECL and TYPE_DECL here, can you add a quick comment why? > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c > index b3ec3a0..78a730e 100644 > --- a/gcc/c-family/c-common.c > +++ b/gcc/c-family/c-common.c > @@ -7253,6 +7253,26 @@ check_missing_format_attribute (tree ltype, tree rtype) > return false; > } > > +/* Check for missing nocf_check attributes on function pointers. LTYPE is > + the new type or left-hand side type. RTYPE is the old type or > + right-hand side type. Returns TRUE if LTYPE is missing the desired > + attribute. */ > + > +bool > +check_missing_nocf_check_attribute (tree ltype, tree rtype) > +{ > + tree const ttr = TREE_TYPE (rtype), ttl = TREE_TYPE (ltype); > + tree ra, la; > + > + for (ra = TYPE_ATTRIBUTES (ttr); ra; ra = TREE_CHAIN (ra)) > + if (is_attribute_p ("nocf_check", TREE_PURPOSE (ra))) > + break; > + for (la = TYPE_ATTRIBUTES (ttl); la; la = TREE_CHAIN (la)) > + if (is_attribute_p ("nocf_check", TREE_PURPOSE (la))) > + break; > + return la != ra; ? ISTM the only time la == ra here is when they're both NULL. Aren't the TYPE_ATTRIBUTE chain members unique and thus pointer equality isn't the right check? Shouldn't you be looking at the TREE_PURPOSE of ra and la and comparing those? Not accepting or rejecting at this point as I could mis-understand how how this is supposed to work in my two comments above. jeff
> -----Original Message----- > From: Jeff Law [mailto:law@redhat.com] > Sent: Friday, September 29, 2017 12:44 AM > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc- > patches@gcc.gnu.org > Cc: richard.guenther@gmail.com > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On 09/19/2017 07:39 AM, Tsimbalist, Igor V wrote: > > Here is an updated patch (version #2). The main differences are: > > > > - Change attribute and option names; > > - Add additional parameter to gimple_build_call_from_tree by adding a > type parameter and > > use it 'nocf_check' attribute propagation; > > - Reimplement fixes in expand_call_stmt to propagate 'nocf_check' > > attribute; > > - Consider 'nocf_check' attribute in Identical Code Folding (ICF) > > optimization; > > - Add warning for type inconsistency regarding 'nocf_check' attribute; > > - Many small fixes; > > > > gcc/c-family/ > > * c-attribs.c (handle_nocf_check_attribute): New function. > > (c_common_attribute_table): Add 'nocf_check' handling. > > * c-common.c (check_missing_format_attribute): New function. > > * c-common.h: Likewise. > > > > gcc/c/ > > * c-typeck.c (convert_for_assignment): Add check for nocf_check > > attribute. > > * gimple-parser.c: Add second argument NULL to > > gimple_build_call_from_tree. > > > > gcc/cp/ > > * typeck.c (convert_for_assignment): Add check for nocf_check > > attribute. > > > > gcc/ > > * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOCF_CHECK for > > call insn. > > * combine.c (distribute_notes): Add REG_CALL_NOCF_CHECK > handling. > > * common.opt: Add fcf-protection flag. > > * emit-rtl.c (try_split): Add REG_CALL_NOCF_CHECK handling. > > * flag-types.h: Add enum cf_protection_level. > > * gimple.c (gimple_build_call_from_tree): Add second parameter. > > Add 'nocf_check' attribute propagation to gimple call. > > * gimple.h (gf_mask): Add GF_CALL_NOCF_CHECK. > > (gimple_call_nocf_check_p): New function. > > (gimple_call_set_nocf_check): Likewise. > > * gimplify.c: Add second argument to gimple_build_call_from_tree. > > * ipa-icf.c: Add nocf_check attribute in statement hash. > > * recog.c (peep2_attempt): Add REG_CALL_NOCF_CHECK handling. > > * reg-notes.def: Add REG_NOTE (CALL_NOCF_CHECK). > > * toplev.c (process_options): Add flag_cf_protection handling. > > > > Is it ok for trunk? > > > > Thanks, > > Igor > > > > > > > > > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index > > 0337537..77d1909 100644 > > --- a/gcc/c-family/c-attribs.c > > +++ b/gcc/c-family/c-attribs.c > > @@ -65,6 +65,7 @@ static tree handle_asan_odr_indicator_attribute > > (tree *, tree, tree, int, static tree handle_stack_protect_attribute > > (tree *, tree, tree, int, bool *); static tree > > handle_noinline_attribute (tree *, tree, tree, int, bool *); static > > tree handle_noclone_attribute (tree *, tree, tree, int, bool *); > > +static tree handle_nocf_check_attribute (tree *, tree, tree, int, > > +bool *); > > static tree handle_noicf_attribute (tree *, tree, tree, int, bool *); > > static tree handle_noipa_attribute (tree *, tree, tree, int, bool *); > > static tree handle_leaf_attribute (tree *, tree, tree, int, bool *); > > @@ -367,6 +368,8 @@ const struct attribute_spec > c_common_attribute_table[] = > > { "patchable_function_entry", 1, 2, true, false, false, > > handle_patchable_function_entry_attribute, > > false }, > > + { "nocf_check", 0, 0, false, true, true, > > + handle_nocf_check_attribute, false }, > > { NULL, 0, 0, false, false, false, NULL, false } > > }; > > > > @@ -783,6 +786,26 @@ handle_noclone_attribute (tree *node, tree > name, > > return NULL_TREE; > > } > > > > +/* Handle a "nocf_check" attribute; arguments as in > > + struct attribute_spec.handler. */ > > + > > +static tree > > +handle_nocf_check_attribute (tree *node, tree name, > > + tree ARG_UNUSED (args), > > + int ARG_UNUSED (flags), bool *no_add_attrs) { > > + if (TREE_CODE (*node) != FUNCTION_TYPE > > + && TREE_CODE (*node) != METHOD_TYPE > > + && TREE_CODE (*node) != FIELD_DECL > > + && TREE_CODE (*node) != TYPE_DECL) > So curious, is this needed for FIELD_DECL and TYPE_DECL? ISTM the attribute > is applied to function/method types. > > If we do need to handle FIELD_DECL and TYPE_DECL here, can you add a > quick comment why? You are right. Probably it was left from the attribute transition from decl to type. I removed these two lines. All CET tests passed. > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index > > b3ec3a0..78a730e 100644 > > --- a/gcc/c-family/c-common.c > > +++ b/gcc/c-family/c-common.c > > @@ -7253,6 +7253,26 @@ check_missing_format_attribute (tree ltype, > tree rtype) > > return false; > > } > > > > +/* Check for missing nocf_check attributes on function pointers. LTYPE is > > + the new type or left-hand side type. RTYPE is the old type or > > + right-hand side type. Returns TRUE if LTYPE is missing the desired > > + attribute. */ > > + > > +bool > > +check_missing_nocf_check_attribute (tree ltype, tree rtype) { > > + tree const ttr = TREE_TYPE (rtype), ttl = TREE_TYPE (ltype); > > + tree ra, la; > > + > > + for (ra = TYPE_ATTRIBUTES (ttr); ra; ra = TREE_CHAIN (ra)) > > + if (is_attribute_p ("nocf_check", TREE_PURPOSE (ra))) > > + break; > > + for (la = TYPE_ATTRIBUTES (ttl); la; la = TREE_CHAIN (la)) > > + if (is_attribute_p ("nocf_check", TREE_PURPOSE (la))) > > + break; > > + return la != ra; > ? ISTM the only time la == ra here is when they're both NULL. Aren't the > TYPE_ATTRIBUTE chain members unique and thus pointer equality isn't the > right check? > > Shouldn't you be looking at the TREE_PURPOSE of ra and la and comparing > those? It looks I was lucky :). I see the point and re-wrote the return statement as if ((la && ra) /* Both types have the attribute. */ || (la == ra)) /* Both types do not have the attribute. */ return false; else return true; /* One of the types has the attribute. */ Igor > Not accepting or rejecting at this point as I could mis-understand how how > this is supposed to work in my two comments above. > > jeff > > >
Updated patch, version #3. Igor > -----Original Message----- > From: Tsimbalist, Igor V > Sent: Friday, September 29, 2017 4:32 PM > To: Jeff Law <law@redhat.com>; gcc-patches@gcc.gnu.org > Cc: richard.guenther@gmail.com; Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > -----Original Message----- > > From: Jeff Law [mailto:law@redhat.com] > > Sent: Friday, September 29, 2017 12:44 AM > > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc- > > patches@gcc.gnu.org > > Cc: richard.guenther@gmail.com > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > On 09/19/2017 07:39 AM, Tsimbalist, Igor V wrote: > > > Here is an updated patch (version #2). The main differences are: > > > > > > - Change attribute and option names; > > > - Add additional parameter to gimple_build_call_from_tree by adding > > > a > > type parameter and > > > use it 'nocf_check' attribute propagation; > > > - Reimplement fixes in expand_call_stmt to propagate 'nocf_check' > > > attribute; > > > - Consider 'nocf_check' attribute in Identical Code Folding (ICF) > > > optimization; > > > - Add warning for type inconsistency regarding 'nocf_check' > > > attribute; > > > - Many small fixes; > > > > > > gcc/c-family/ > > > * c-attribs.c (handle_nocf_check_attribute): New function. > > > (c_common_attribute_table): Add 'nocf_check' handling. > > > * c-common.c (check_missing_format_attribute): New function. > > > * c-common.h: Likewise. > > > > > > gcc/c/ > > > * c-typeck.c (convert_for_assignment): Add check for nocf_check > > > attribute. > > > * gimple-parser.c: Add second argument NULL to > > > gimple_build_call_from_tree. > > > > > > gcc/cp/ > > > * typeck.c (convert_for_assignment): Add check for nocf_check > > > attribute. > > > > > > gcc/ > > > * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOCF_CHECK for > > > call insn. > > > * combine.c (distribute_notes): Add REG_CALL_NOCF_CHECK > > handling. > > > * common.opt: Add fcf-protection flag. > > > * emit-rtl.c (try_split): Add REG_CALL_NOCF_CHECK handling. > > > * flag-types.h: Add enum cf_protection_level. > > > * gimple.c (gimple_build_call_from_tree): Add second parameter. > > > Add 'nocf_check' attribute propagation to gimple call. > > > * gimple.h (gf_mask): Add GF_CALL_NOCF_CHECK. > > > (gimple_call_nocf_check_p): New function. > > > (gimple_call_set_nocf_check): Likewise. > > > * gimplify.c: Add second argument to gimple_build_call_from_tree. > > > * ipa-icf.c: Add nocf_check attribute in statement hash. > > > * recog.c (peep2_attempt): Add REG_CALL_NOCF_CHECK handling. > > > * reg-notes.def: Add REG_NOTE (CALL_NOCF_CHECK). > > > * toplev.c (process_options): Add flag_cf_protection handling. > > > > > > Is it ok for trunk? > > > > > > Thanks, > > > Igor > > > > > > > > > > > > > > > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > > > index > > > 0337537..77d1909 100644 > > > --- a/gcc/c-family/c-attribs.c > > > +++ b/gcc/c-family/c-attribs.c > > > @@ -65,6 +65,7 @@ static tree handle_asan_odr_indicator_attribute > > > (tree *, tree, tree, int, static tree > > > handle_stack_protect_attribute (tree *, tree, tree, int, bool *); > > > static tree handle_noinline_attribute (tree *, tree, tree, int, bool > > > *); static tree handle_noclone_attribute (tree *, tree, tree, int, > > > bool *); > > > +static tree handle_nocf_check_attribute (tree *, tree, tree, int, > > > +bool *); > > > static tree handle_noicf_attribute (tree *, tree, tree, int, bool > > > *); static tree handle_noipa_attribute (tree *, tree, tree, int, > > > bool *); static tree handle_leaf_attribute (tree *, tree, tree, int, > > > bool *); @@ -367,6 +368,8 @@ const struct attribute_spec > > c_common_attribute_table[] = > > > { "patchable_function_entry", 1, 2, true, false, false, > > > handle_patchable_function_entry_attribute, > > > false }, > > > + { "nocf_check", 0, 0, false, true, true, > > > + handle_nocf_check_attribute, false }, > > > { NULL, 0, 0, false, false, false, NULL, false } > > > }; > > > > > > @@ -783,6 +786,26 @@ handle_noclone_attribute (tree *node, tree > > name, > > > return NULL_TREE; > > > } > > > > > > +/* Handle a "nocf_check" attribute; arguments as in > > > + struct attribute_spec.handler. */ > > > + > > > +static tree > > > +handle_nocf_check_attribute (tree *node, tree name, > > > + tree ARG_UNUSED (args), > > > + int ARG_UNUSED (flags), bool *no_add_attrs) { > > > + if (TREE_CODE (*node) != FUNCTION_TYPE > > > + && TREE_CODE (*node) != METHOD_TYPE > > > + && TREE_CODE (*node) != FIELD_DECL > > > + && TREE_CODE (*node) != TYPE_DECL) > > So curious, is this needed for FIELD_DECL and TYPE_DECL? ISTM the > > attribute is applied to function/method types. > > > > If we do need to handle FIELD_DECL and TYPE_DECL here, can you add a > > quick comment why? > > You are right. Probably it was left from the attribute transition from decl to > type. > I removed these two lines. All CET tests passed. > > > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index > > > b3ec3a0..78a730e 100644 > > > --- a/gcc/c-family/c-common.c > > > +++ b/gcc/c-family/c-common.c > > > @@ -7253,6 +7253,26 @@ check_missing_format_attribute (tree ltype, > > tree rtype) > > > return false; > > > } > > > > > > +/* Check for missing nocf_check attributes on function pointers. LTYPE > is > > > + the new type or left-hand side type. RTYPE is the old type or > > > + right-hand side type. Returns TRUE if LTYPE is missing the desired > > > + attribute. */ > > > + > > > +bool > > > +check_missing_nocf_check_attribute (tree ltype, tree rtype) { > > > + tree const ttr = TREE_TYPE (rtype), ttl = TREE_TYPE (ltype); > > > + tree ra, la; > > > + > > > + for (ra = TYPE_ATTRIBUTES (ttr); ra; ra = TREE_CHAIN (ra)) > > > + if (is_attribute_p ("nocf_check", TREE_PURPOSE (ra))) > > > + break; > > > + for (la = TYPE_ATTRIBUTES (ttl); la; la = TREE_CHAIN (la)) > > > + if (is_attribute_p ("nocf_check", TREE_PURPOSE (la))) > > > + break; > > > + return la != ra; > > ? ISTM the only time la == ra here is when they're both NULL. Aren't > > the TYPE_ATTRIBUTE chain members unique and thus pointer equality > > isn't the right check? > > > > Shouldn't you be looking at the TREE_PURPOSE of ra and la and > > comparing those? > > It looks I was lucky :). I see the point and re-wrote the return statement as > > if ((la && ra) /* Both types have the attribute. */ > || (la == ra)) /* Both types do not have the attribute. */ > return false; > else > return true; /* One of the types has the attribute. */ > > Igor > > > Not accepting or rejecting at this point as I could mis-understand how > > how this is supposed to work in my two comments above. > > > > jeff > > > > > >
I would like to implement the patch in a bit different way depending on answers I will get for my following proposals: - I propose to make a type with 'nocf_check' attribute to be different from type w/o the attribute. The reason is that the type with 'nocf_check' attribute implies different code generation. It will be done by setting affects_type_identity field to true for the attribute. That in turn will trigger needed or expected type checking; - I propose to ignore the 'nocf_check' attribute if 'fcf-protection' option is not specified and output the warning about this. If there is no instrumentation the type with attribute should not be treated differently from type w/o the attribute (see previous item) and should not be recorded into the type. If it's ok, please ignore my previous patch (version#3) and I will post the updated patch shortly. Igor > -----Original Message----- > From: Tsimbalist, Igor V > Sent: Friday, September 29, 2017 6:04 PM > To: Jeff Law <law@redhat.com>; gcc-patches@gcc.gnu.org > Cc: richard.guenther@gmail.com; Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > Updated patch, version #3. > > Igor > > > > -----Original Message----- > > From: Tsimbalist, Igor V > > Sent: Friday, September 29, 2017 4:32 PM > > To: Jeff Law <law@redhat.com>; gcc-patches@gcc.gnu.org > > Cc: richard.guenther@gmail.com; Tsimbalist, Igor V > > <igor.v.tsimbalist@intel.com> > > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > > -----Original Message----- > > > From: Jeff Law [mailto:law@redhat.com] > > > Sent: Friday, September 29, 2017 12:44 AM > > > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc- > > > patches@gcc.gnu.org > > > Cc: richard.guenther@gmail.com > > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > > > On 09/19/2017 07:39 AM, Tsimbalist, Igor V wrote: > > > > Here is an updated patch (version #2). The main differences are: > > > > > > > > - Change attribute and option names; > > > > - Add additional parameter to gimple_build_call_from_tree by > > > > adding a > > > type parameter and > > > > use it 'nocf_check' attribute propagation; > > > > - Reimplement fixes in expand_call_stmt to propagate 'nocf_check' > > > > attribute; > > > > - Consider 'nocf_check' attribute in Identical Code Folding (ICF) > > > > optimization; > > > > - Add warning for type inconsistency regarding 'nocf_check' > > > > attribute; > > > > - Many small fixes; > > > > > > > > gcc/c-family/ > > > > * c-attribs.c (handle_nocf_check_attribute): New function. > > > > (c_common_attribute_table): Add 'nocf_check' handling. > > > > * c-common.c (check_missing_format_attribute): New function. > > > > * c-common.h: Likewise. > > > > > > > > gcc/c/ > > > > * c-typeck.c (convert_for_assignment): Add check for nocf_check > > > > attribute. > > > > * gimple-parser.c: Add second argument NULL to > > > > gimple_build_call_from_tree. > > > > > > > > gcc/cp/ > > > > * typeck.c (convert_for_assignment): Add check for nocf_check > > > > attribute. > > > > > > > > gcc/ > > > > * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOCF_CHECK for > > > > call insn. > > > > * combine.c (distribute_notes): Add REG_CALL_NOCF_CHECK > > > handling. > > > > * common.opt: Add fcf-protection flag. > > > > * emit-rtl.c (try_split): Add REG_CALL_NOCF_CHECK handling. > > > > * flag-types.h: Add enum cf_protection_level. > > > > * gimple.c (gimple_build_call_from_tree): Add second parameter. > > > > Add 'nocf_check' attribute propagation to gimple call. > > > > * gimple.h (gf_mask): Add GF_CALL_NOCF_CHECK. > > > > (gimple_call_nocf_check_p): New function. > > > > (gimple_call_set_nocf_check): Likewise. > > > > * gimplify.c: Add second argument to gimple_build_call_from_tree. > > > > * ipa-icf.c: Add nocf_check attribute in statement hash. > > > > * recog.c (peep2_attempt): Add REG_CALL_NOCF_CHECK handling. > > > > * reg-notes.def: Add REG_NOTE (CALL_NOCF_CHECK). > > > > * toplev.c (process_options): Add flag_cf_protection handling. > > > > > > > > Is it ok for trunk? > > > > > > > > Thanks, > > > > Igor > > > > > > > > > > > > > > > > > > > > > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > > > > index > > > > 0337537..77d1909 100644 > > > > --- a/gcc/c-family/c-attribs.c > > > > +++ b/gcc/c-family/c-attribs.c > > > > @@ -65,6 +65,7 @@ static tree handle_asan_odr_indicator_attribute > > > > (tree *, tree, tree, int, static tree > > > > handle_stack_protect_attribute (tree *, tree, tree, int, bool *); > > > > static tree handle_noinline_attribute (tree *, tree, tree, int, > > > > bool *); static tree handle_noclone_attribute (tree *, tree, > > > > tree, int, bool *); > > > > +static tree handle_nocf_check_attribute (tree *, tree, tree, int, > > > > +bool *); > > > > static tree handle_noicf_attribute (tree *, tree, tree, int, bool > > > > *); static tree handle_noipa_attribute (tree *, tree, tree, int, > > > > bool *); static tree handle_leaf_attribute (tree *, tree, tree, > > > > int, bool *); @@ -367,6 +368,8 @@ const struct attribute_spec > > > c_common_attribute_table[] = > > > > { "patchable_function_entry", 1, 2, true, false, false, > > > > handle_patchable_function_entry_attribute, > > > > false }, > > > > + { "nocf_check", 0, 0, false, true, true, > > > > + handle_nocf_check_attribute, false }, > > > > { NULL, 0, 0, false, false, false, NULL, false } > > > > }; > > > > > > > > @@ -783,6 +786,26 @@ handle_noclone_attribute (tree *node, tree > > > name, > > > > return NULL_TREE; > > > > } > > > > > > > > +/* Handle a "nocf_check" attribute; arguments as in > > > > + struct attribute_spec.handler. */ > > > > + > > > > +static tree > > > > +handle_nocf_check_attribute (tree *node, tree name, > > > > + tree ARG_UNUSED (args), > > > > + int ARG_UNUSED (flags), bool *no_add_attrs) { > > > > + if (TREE_CODE (*node) != FUNCTION_TYPE > > > > + && TREE_CODE (*node) != METHOD_TYPE > > > > + && TREE_CODE (*node) != FIELD_DECL > > > > + && TREE_CODE (*node) != TYPE_DECL) > > > So curious, is this needed for FIELD_DECL and TYPE_DECL? ISTM the > > > attribute is applied to function/method types. > > > > > > If we do need to handle FIELD_DECL and TYPE_DECL here, can you add a > > > quick comment why? > > > > You are right. Probably it was left from the attribute transition from > > decl to type. > > I removed these two lines. All CET tests passed. > > > > > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c > > > > index b3ec3a0..78a730e 100644 > > > > --- a/gcc/c-family/c-common.c > > > > +++ b/gcc/c-family/c-common.c > > > > @@ -7253,6 +7253,26 @@ check_missing_format_attribute (tree ltype, > > > tree rtype) > > > > return false; > > > > } > > > > > > > > +/* Check for missing nocf_check attributes on function pointers. > > > > +LTYPE > > is > > > > + the new type or left-hand side type. RTYPE is the old type or > > > > + right-hand side type. Returns TRUE if LTYPE is missing the desired > > > > + attribute. */ > > > > + > > > > +bool > > > > +check_missing_nocf_check_attribute (tree ltype, tree rtype) { > > > > + tree const ttr = TREE_TYPE (rtype), ttl = TREE_TYPE (ltype); > > > > + tree ra, la; > > > > + > > > > + for (ra = TYPE_ATTRIBUTES (ttr); ra; ra = TREE_CHAIN (ra)) > > > > + if (is_attribute_p ("nocf_check", TREE_PURPOSE (ra))) > > > > + break; > > > > + for (la = TYPE_ATTRIBUTES (ttl); la; la = TREE_CHAIN (la)) > > > > + if (is_attribute_p ("nocf_check", TREE_PURPOSE (la))) > > > > + break; > > > > + return la != ra; > > > ? ISTM the only time la == ra here is when they're both NULL. > > > Aren't the TYPE_ATTRIBUTE chain members unique and thus pointer > > > equality isn't the right check? > > > > > > Shouldn't you be looking at the TREE_PURPOSE of ra and la and > > > comparing those? > > > > It looks I was lucky :). I see the point and re-wrote the return > > statement as > > > > if ((la && ra) /* Both types have the attribute. */ > > || (la == ra)) /* Both types do not have the attribute. */ > > return false; > > else > > return true; /* One of the types has the attribute. */ > > > > Igor > > > > > Not accepting or rejecting at this point as I could mis-understand > > > how how this is supposed to work in my two comments above. > > > > > > jeff > > > > > > > > >
On 10/05/2017 04:20 AM, Tsimbalist, Igor V wrote: > I would like to implement the patch in a bit different way depending on answers I will get for > my following proposals: > > - I propose to make a type with 'nocf_check' attribute to be different from type w/o the attribute. > The reason is that the type with 'nocf_check' attribute implies different code generation. It will be > done by setting affects_type_identity field to true for the attribute. That in turn will trigger > needed or expected type checking; Seems reasonable. As a result something like check_missing_nocf_check_attribute is going to just go away along with the code in *-typeck.c which called it, right? If so that seems like a nice cleanup. > > - I propose to ignore the 'nocf_check' attribute if 'fcf-protection' option is not specified and output > the warning about this. If there is no instrumentation the type with attribute should not be treated > differently from type w/o the attribute (see previous item) and should not be recorded into the > type. Seems reasonable. > > If it's ok, please ignore my previous patch (version#3) and I will post the updated patch shortly. OK. FWIW, I think we're ready to ACK on this. So get it posted :-) jeff
> Seems reasonable. As a result something like > check_missing_nocf_check_attribute is going to just go away along with the > code in *-typeck.c which called it, right? If so that seems like a nice cleanup. Yes, you are right. Updated patch is attached. Igor > -----Original Message----- > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches- > owner@gcc.gnu.org] On Behalf Of Jeff Law > Sent: Thursday, October 12, 2017 8:07 AM > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc- > patches@gcc.gnu.org > Cc: richard.guenther@gmail.com > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On 10/05/2017 04:20 AM, Tsimbalist, Igor V wrote: > > I would like to implement the patch in a bit different way depending > > on answers I will get for my following proposals: > > > > - I propose to make a type with 'nocf_check' attribute to be different from > type w/o the attribute. > > The reason is that the type with 'nocf_check' attribute implies different > code generation. It will be > > done by setting affects_type_identity field to true for the attribute. That > in turn will trigger > > needed or expected type checking; > Seems reasonable. As a result something like > check_missing_nocf_check_attribute is going to just go away along with the > code in *-typeck.c which called it, right? If so that seems like a nice cleanup. > > > > > > - I propose to ignore the 'nocf_check' attribute if 'fcf-protection' option is > not specified and output > > the warning about this. If there is no instrumentation the type with > attribute should not be treated > > differently from type w/o the attribute (see previous item) and should > not be recorded into the > > type. > Seems reasonable. > > > > If it's ok, please ignore my previous patch (version#3) and I will post the > updated patch shortly. > OK. FWIW, I think we're ready to ACK on this. So get it posted :-) > > jeff
On 10/12/2017 02:12 AM, Tsimbalist, Igor V wrote: >> Seems reasonable. As a result something like >> check_missing_nocf_check_attribute is going to just go away along with the >> code in *-typeck.c which called it, right? If so that seems like a nice cleanup. > Yes, you are right. > > Updated patch is attached. > > > High-level design. > ------------------ > > A proposal is to introduce a target independent flag > -fcf-protection=[none|branch|return|full] with a semantic to > instrument a code to control validness or integrity of control-flow > transfers using jump and call instructions. The main goal is to detect > and block a possible malware execution through transfer the execution > to unknown target address. Implementation could be either software or > target based. Any target platforms can provide their implementation > for instrumentation under this option. > > When the -fcf-protection flag is set each implementation has > to check if a support exists for a target platform and report an error > if no support is found. > > The compiler should instrument any control-flow transfer points in a > program (ex. call/jmp/ret) as well as any landing pads, which are > targets of control-flow transfers. > > A new 'nocf_check' attribute is introduced to provide hand tuning > support. The attribute directs the compiler to skip a call to a > function and a function's landing pad from instrumentation. The > attribute can be used for function and pointer to function types, > otherwise it will be ignored. The attribute is saved in a type and > propagated to a GIMPLE call statement and later to a call instruction. > > Currently all platforms except i386 will report the error and do no > instrumentation. i386 will provide the implementation based on a > specification published by Intel for a new technology called > Control-flow Enforcement Technology (CET). > > gcc/c-family/ > * c-attribs.c (handle_nocf_check_attribute): New function. > (c_common_attribute_table): Add 'nocf_check' handling. > > gcc/c/ > * gimple-parser.c: Add second argument NULL to > gimple_build_call_from_tree. > > gcc/ > * attrib.c (comp_type_attributes): Check nocf_check attribute. > * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOCF_CHECK for > call insn. > * combine.c (distribute_notes): Add REG_CALL_NOCF_CHECK handling. > * common.opt: Add fcf-protection flag. > * emit-rtl.c (try_split): Add REG_CALL_NOCF_CHECK handling. > * flag-types.h: Add enum cf_protection_level. > * gimple.c (gimple_build_call_from_tree): Add second parameter. > Add 'nocf_check' attribute propagation to gimple call. > * gimple.h (gf_mask): Add GF_CALL_NOCF_CHECK. > (gimple_build_call_from_tree): Update prototype. > (gimple_call_nocf_check_p): New function. > (gimple_call_set_nocf_check): Likewise. > * gimplify.c: Add second argument to gimple_build_call_from_tree. > * ipa-icf.c: Add nocf_check attribute in statement hash. > * recog.c (peep2_attempt): Add REG_CALL_NOCF_CHECK handling. > * reg-notes.def: Add REG_NOTE (CALL_NOCF_CHECK). > * toplev.c (process_options): Add flag_cf_protection handling. OK. Sorry about the long delays. jeff
diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t) gimple_set_no_warning (call, TREE_NO_WARNING (t)); gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t)); + if (fndecl == NULL_TREE) + { + /* Find the type of an indirect call. */ + tree addr = CALL_EXPR_FN (t); + if (TREE_CODE (addr) != FUNCTION_DECL) + { + tree fntype = TREE_TYPE (addr); + gcc_assert (POINTER_TYPE_P (fntype)); + fntype = TREE_TYPE (fntype); + + /* Check if its type has the no-track attribute and propagate + it to the CALL insn. */ + if (lookup_attribute ("notrack", TYPE_ATTRIBUTES (fntype))) + gimple_call_set_with_notrack (call, TRUE); + } + } this means notrack is not recognized if fndecl is not NULL. Note that only the two callers know the real function type in effect (they call gimple_call_set_fntype with it). I suggest to pass down that type