* 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling @ 2017-08-01 8:56 Tsimbalist, Igor V 2017-08-15 14:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2017-08-25 21:05 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 0 siblings, 2 replies; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-08-01 8:56 UTC (permalink / raw) To: 'gcc-patches@gcc.gnu.org'; +Cc: Tsimbalist, Igor V [-- Attachment #1: Type: text/plain, Size: 1733 bytes --] 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-flow-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). [-- Attachment #2: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling.patch --] [-- Type: application/octet-stream, Size: 11535 bytes --] From 403fc8239fb1f690cc378287b4def57dcc9d25bf Mon Sep 17 00:00:00 2001 From: Igor Tsimbalist <igor.v.tsimbalist@intel.com> Date: Mon, 3 Jul 2017 17:11:58 +0300 Subject: [PATCH 1/9] 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-flow-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). gcc/c-family/ * c-attribs.c (handle_notrack_attribute): New function. (c_common_attribute_table): Add 'notrack' handling. gcc/ * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOTRACK. * combine.c (distribute_notes): Add REG_CALL_NOTRACK handling. * common.opt: Add finstrument-control-flow flag. * emit-rtl.c (try_split): Add REG_CALL_NOTRACK handling. * gimple.c (gimple_build_call_from_tree): Add 'notrack' propogation. * gimple.h (gf_mask): Add GF_CALL_WITH_NOTRACK. (gimple_call_with_notrack_p): function. (gimple_call_set_with_notrack): Likewise. * recog.c (peep2_attempt): Add REG_CALL_NOTRACK handling. * reg-notes.def: Add REG_NOTE (CALL_NOTRACK). * toplev.c (process_options): Add flag_instrument_control_flow handling. --- gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ gcc/cfgexpand.c | 11 +++++++++++ gcc/combine.c | 1 + gcc/common.opt | 5 +++++ gcc/emit-rtl.c | 1 + gcc/gimple.c | 17 +++++++++++++++++ gcc/gimple.h | 34 ++++++++++++++++++++++++++++++++++ gcc/recog.c | 1 + gcc/reg-notes.def | 7 +++++++ gcc/toplev.c | 11 +++++++++++ 10 files changed, 111 insertions(+) --- gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ gcc/cfgexpand.c | 11 +++++++++++ gcc/combine.c | 1 + gcc/common.opt | 5 +++++ gcc/emit-rtl.c | 1 + gcc/gimple.c | 17 +++++++++++++++++ gcc/gimple.h | 34 ++++++++++++++++++++++++++++++++++ gcc/recog.c | 1 + gcc/reg-notes.def | 7 +++++++ gcc/toplev.c | 11 +++++++++++ 10 files changed, 111 insertions(+) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 0d9ab2d..4d62f7c 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_notrack_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 *); @@ -359,6 +360,8 @@ const struct attribute_spec c_common_attribute_table[] = { "patchable_function_entry", 1, 2, true, false, false, handle_patchable_function_entry_attribute, false }, + { "notrack", 0, 0, false, true, true, + handle_notrack_attribute, false }, { NULL, 0, 0, false, false, false, NULL, false } }; @@ -775,6 +778,26 @@ handle_noclone_attribute (tree *node, tree name, return NULL_TREE; } +/* Handle a "notrack" attribute; arguments as in + struct attribute_spec.handler. */ + +static tree +handle_notrack_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) + { + warning (OPT_Wattributes, "%qE attribute ignored", name); + *no_add_attrs = true; + } + + return NULL_TREE; +} + /* Handle a "no_icf" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index c9d8118..ed344c5 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -2657,12 +2657,23 @@ expand_call_stmt (gcall *stmt) } } + rtx_insn *before_call = get_last_insn (); lhs = gimple_call_lhs (stmt); if (lhs) expand_assignment (lhs, exp, false); else expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); + /* Find a generated CALL insn to propagate a 'notrack' attribute. */ + rtx_insn *last = get_last_insn (); + while (!CALL_P (last) + && (last != before_call)) + last = PREV_INSN (last); + + if (last != before_call + && gimple_call_with_notrack_p (stmt)) + add_reg_note (last, REG_CALL_NOTRACK, const0_rtx); + mark_transaction_restart_calls (stmt); } diff --git a/gcc/combine.c b/gcc/combine.c index 8dc62b5..a9e5e4f 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -14029,6 +14029,7 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, case REG_SETJMP: case REG_TM: case REG_CALL_DECL: + case REG_CALL_NOTRACK: /* These notes must remain with the call. It should not be possible for both I2 and I3 to be a call. */ if (CALL_P (i3)) diff --git a/gcc/common.opt b/gcc/common.opt index 78cfa56..21069bb 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1602,6 +1602,11 @@ finline-atomics Common Report Var(flag_inline_atomics) Init(1) Optimization Inline __atomic operations when a lock free instruction sequence is available. +finstrument-control-flow +Common Report Var(flag_instrument_control_flow) Init(-1) +Instrument functions with checks to verify jump/call control-flow transfer +instructions have valid targets. + finstrument-functions Common Report Var(flag_instrument_function_entry_exit) Instrument function entry and exit with profiling calls. diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index 2bc5d56..b8744fb 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -3780,6 +3780,7 @@ try_split (rtx pat, rtx_insn *trial, int last) case REG_NORETURN: case REG_SETJMP: case REG_TM: + case REG_CALL_NOTRACK: for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn)) { if (CALL_P (insn)) 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); + } + } + return call; } diff --git a/gcc/gimple.h b/gcc/gimple.h index 2d81eed..afd38f0 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -148,6 +148,7 @@ enum gf_mask { GF_CALL_WITH_BOUNDS = 1 << 8, GF_CALL_MUST_TAIL_CALL = 1 << 9, GF_CALL_BY_DESCRIPTOR = 1 << 10, + GF_CALL_WITH_NOTRACK = 1 << 11, GF_OMP_PARALLEL_COMBINED = 1 << 0, GF_OMP_PARALLEL_GRID_PHONY = 1 << 1, GF_OMP_TASK_TASKLOOP = 1 << 0, @@ -2898,6 +2899,39 @@ gimple_call_set_with_bounds (gimple *gs, bool with_bounds) } +/* Return true if call GS is marked as no-track. */ + +static inline bool +gimple_call_with_notrack_p (const gcall *gs) +{ + return (gs->subcode & GF_CALL_WITH_NOTRACK) != 0; +} + +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); +} + +/* Mark statement GS as no-track call. */ + +static inline void +gimple_call_set_with_notrack (gcall *gs, bool with_notrack) +{ + if (with_notrack) + gs->subcode |= GF_CALL_WITH_NOTRACK; + else + gs->subcode &= ~GF_CALL_WITH_NOTRACK; +} + +static inline void +gimple_call_set_with_notrack (gimple *gs, bool with_notrack) +{ + gcall *gc = GIMPLE_CHECK2<gcall *> (gs); + gimple_call_set_with_notrack (gc, with_notrack); +} + /* Return the target of internal call GS. */ static inline enum internal_fn diff --git a/gcc/recog.c b/gcc/recog.c index fd4e460..f2af977 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -3387,6 +3387,7 @@ peep2_attempt (basic_block bb, rtx_insn *insn, int match_len, rtx_insn *attempt) case REG_NORETURN: case REG_SETJMP: case REG_TM: + case REG_CALL_NOTRACK: add_reg_note (new_insn, REG_NOTE_KIND (note), XEXP (note, 0)); break; diff --git a/gcc/reg-notes.def b/gcc/reg-notes.def index 943eff4..f02bcc9 100644 --- a/gcc/reg-notes.def +++ b/gcc/reg-notes.def @@ -228,3 +228,10 @@ REG_NOTE (RETURNED) The decl might not be available in the call due to splitting of the call insn. This note is a SYMBOL_REF. */ REG_NOTE (CALL_DECL) + +/* Indicate that a call is a NOTRACK call. The target address of the call + is assumed as a valid address and no check to validate the target + address is needed. The call is marked when a called function has a + 'notrack' attribute. This note is used by the compiler when the option + -finstrument-control-flow is specified. */ +REG_NOTE (CALL_NOTRACK) diff --git a/gcc/toplev.c b/gcc/toplev.c index b28f184..c20a6ea 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1283,6 +1283,17 @@ process_options (void) "-floop-parallelize-all)"); #endif + if (flag_instrument_control_flow == -1) + flag_instrument_control_flow = 0; + else if (flag_instrument_control_flow > 0 + && flag_instrument_control_flow != 2) + { + error_at (UNKNOWN_LOCATION, + "%<-finstrument-control-flow%> is not supported for this " + "target"); + flag_instrument_control_flow = 0; + } + if (flag_check_pointer_bounds) { if (targetm.chkp_bound_mode () == VOIDmode) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-01 8:56 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-08-15 14:08 ` Richard Biener 2017-08-18 14:01 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-08-25 21:03 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-08-25 21:05 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 1 sibling, 2 replies; 24+ messages in thread From: Richard Biener @ 2017-08-15 14:08 UTC (permalink / raw) To: Tsimbalist, Igor V; +Cc: gcc-patches 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-flow-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. +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. 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'? 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)? Thanks, Richard. ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-15 14:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener @ 2017-08-18 14:01 ` Tsimbalist, Igor V 2017-08-18 14:06 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2017-08-25 21:03 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 1 sibling, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-08-18 14:01 UTC (permalink / raw) To: 'Richard Biener'; +Cc: gcc-patches, Tsimbalist, Igor V > -----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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-18 14:01 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-08-18 14:06 ` Richard Biener 2017-08-18 14:58 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Richard Biener @ 2017-08-18 14:06 UTC (permalink / raw) To: Tsimbalist, Igor V; +Cc: gcc-patches 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-18 14:06 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener @ 2017-08-18 14:58 ` Tsimbalist, Igor V 2017-09-12 15:34 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-15 11:12 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2 siblings, 0 replies; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-08-18 14:58 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, Tsimbalist, Igor V > -----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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-18 14:06 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2017-08-18 14:58 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-12 15:34 ` Tsimbalist, Igor V 2017-09-15 11:12 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2 siblings, 0 replies; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-09-12 15:34 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, Tsimbalist, Igor V > -----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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-18 14:06 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2017-08-18 14:58 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-12 15:34 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-15 11:12 ` Tsimbalist, Igor V 2017-09-15 12:14 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2 siblings, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-09-15 11:12 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, Tsimbalist, Igor V > -----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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-15 11:12 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-15 12:14 ` Richard Biener 2017-09-19 13:39 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 0 siblings, 1 reply; 24+ messages in thread From: Richard Biener @ 2017-09-15 12:14 UTC (permalink / raw) To: Tsimbalist, Igor V; +Cc: gcc-patches 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-15 12:14 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener @ 2017-09-19 13:39 ` Tsimbalist, Igor V 2017-09-28 22:44 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 0 siblings, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-09-19 13:39 UTC (permalink / raw) To: gcc-patches; +Cc: richard.guenther, Tsimbalist, Igor V [-- Attachment #1: Type: text/plain, Size: 6387 bytes --] 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 > > [-- Attachment #2: 0001-Add-generic-part-for-Intel-CET-enabling-fcf-protecti.patch --] [-- Type: application/octet-stream, Size: 22338 bytes --] From 96237d5fad76ee60c4a7b188979bea3b47c44d9a Mon Sep 17 00:00:00 2001 From: Igor Tsimbalist <igor.v.tsimbalist@intel.com> Date: Mon, 3 Jul 2017 17:11:58 +0300 Subject: [PATCH 1/3] Add generic part for Intel CET enabling: fcf-protection, nocf_check The spec is available at https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf 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. * 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. --- gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ gcc/c-family/c-common.c | 20 ++++++++++++++++++++ gcc/c-family/c-common.h | 1 + gcc/c/c-typeck.c | 37 +++++++++++++++++++++++++++++++++++++ gcc/c/gimple-parser.c | 4 ++-- gcc/cfgexpand.c | 16 ++++++++++++++++ gcc/combine.c | 1 + gcc/common.opt | 23 +++++++++++++++++++++++ gcc/cp/typeck.c | 39 +++++++++++++++++++++++++++++++++++++++ gcc/emit-rtl.c | 1 + gcc/flag-types.h | 9 +++++++++ gcc/gimple.c | 18 +++++++++++++++++- gcc/gimple.h | 22 +++++++++++++++++++++- gcc/gimplify.c | 6 ++---- gcc/ipa-icf.c | 6 ++++++ gcc/recog.c | 1 + gcc/reg-notes.def | 7 +++++++ gcc/toplev.c | 26 ++++++++++++++++++++++++++ 18 files changed, 252 insertions(+), 8 deletions(-) --- gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ gcc/c-family/c-common.c | 20 ++++++++++++++++++++ gcc/c-family/c-common.h | 1 + gcc/c/c-typeck.c | 37 +++++++++++++++++++++++++++++++++++++ gcc/c/gimple-parser.c | 4 ++-- gcc/cfgexpand.c | 16 ++++++++++++++++ gcc/combine.c | 1 + gcc/common.opt | 23 +++++++++++++++++++++++ gcc/cp/typeck.c | 39 +++++++++++++++++++++++++++++++++++++++ gcc/emit-rtl.c | 1 + gcc/flag-types.h | 9 +++++++++ gcc/gimple.c | 18 +++++++++++++++++- gcc/gimple.h | 22 +++++++++++++++++++++- gcc/gimplify.c | 6 ++---- gcc/ipa-icf.c | 6 ++++++ gcc/recog.c | 1 + gcc/reg-notes.def | 7 +++++++ gcc/toplev.c | 26 ++++++++++++++++++++++++++ 18 files changed, 252 insertions(+), 8 deletions(-) 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) + { + warning (OPT_Wattributes, "%qE attribute ignored", name); + *no_add_attrs = true; + } + + return NULL_TREE; +} + /* Handle a "no_icf" attribute; arguments as in struct attribute_spec.handler. */ 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; +} + /* Setup a TYPE_DECL node as a typedef representation. X is a TYPE_DECL for a typedef statement. Create a brand new diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 0de549d..829ea8e 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1123,6 +1123,7 @@ extern void preprocess_file (cpp_reader *); extern void pp_file_change (const line_map_ordinary *); extern void pp_dir_change (cpp_reader *, const char *); extern bool check_missing_format_attribute (tree, tree); +extern bool check_missing_nocf_check_attribute (tree, tree); /* In c-omp.c */ struct omp_clause_mask diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index f45fd3c..59ae833 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -6826,6 +6826,43 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type, G_("return makes %q#v qualified function " "pointer from unqualified"), TYPE_QUALS (ttl) & ~TYPE_QUALS (ttr)); + + /* Check if the right-hand side and left-hand side mismatch in a + 'nocf_check' attribute. Both cases could be harmful: + NOCF = CF Implicit control-flow check from CF + is dropped as a future call is done through NOCF; + CF = NOCF Control-flow check will be done through the CF but + the check is not expected by NOCF. */ + if (warn_ignored_attributes + && check_missing_nocf_check_attribute (type, rhstype)) + { + switch (errtype) + { + case ic_argpass: + warning_at (expr_loc, OPT_Wattributes, + "nocf_check attribute mismatch for" + " argument %d of %qE", + parmnum, rname); + break; + case ic_assign: + warning_at (location, OPT_Wattributes, + "nocf_check attribute mismatch for" + " assignment"); + break; + case ic_init: + warning_at (location, OPT_Wattributes, + "nocf_check attribute mismatch for" + " initialization"); + break; + case ic_return: + warning_at (location, OPT_Wattributes, + "nocf_check attribute mismatch for" + " return type"); + break; + default: + gcc_unreachable (); + } + } } } /* Avoid warning about the volatile ObjC EH puts on decls. */ diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index 22f58f4..c2e31df 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -276,7 +276,7 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) && TREE_CODE (lhs.value) == CALL_EXPR) { gimple *call; - call = gimple_build_call_from_tree (lhs.value); + call = gimple_build_call_from_tree (lhs.value, NULL); gimple_seq_add_stmt (seq, call); gimple_set_location (call, loc); return; @@ -407,7 +407,7 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) rhs = c_parser_gimple_unary_expression (parser); if (rhs.value != error_mark_node) { - gimple *call = gimple_build_call_from_tree (rhs.value); + gimple *call = gimple_build_call_from_tree (rhs.value, NULL); gimple_call_set_lhs (call, lhs.value); gimple_seq_add_stmt (seq, call); gimple_set_location (call, loc); diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 7657a65..3bcaa4c 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -2659,12 +2659,28 @@ expand_call_stmt (gcall *stmt) } } + rtx_insn *before_call = get_last_insn (); lhs = gimple_call_lhs (stmt); if (lhs) expand_assignment (lhs, exp, false); else expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); + /* If the gimple call is an indirect call and has 'nocf_check' + attribute find a generated CALL insn to mark it as no + control-flow verification is needed. */ + if (gimple_call_nocf_check_p (stmt) + && !gimple_call_fndecl (stmt)) + { + rtx_insn *last = get_last_insn (); + while (!CALL_P (last) + && last != before_call) + last = PREV_INSN (last); + + if (last != before_call) + add_reg_note (last, REG_CALL_NOCF_CHECK, const0_rtx); + } + mark_transaction_restart_calls (stmt); } diff --git a/gcc/combine.c b/gcc/combine.c index 1832c3c..e0315fd 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -14151,6 +14151,7 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, case REG_SETJMP: case REG_TM: case REG_CALL_DECL: + case REG_CALL_NOCF_CHECK: /* These notes must remain with the call. It should not be possible for both I2 and I3 to be a call. */ if (CALL_P (i3)) diff --git a/gcc/common.opt b/gcc/common.opt index 1581ca8..b06bede 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1608,6 +1608,29 @@ finline-atomics Common Report Var(flag_inline_atomics) Init(1) Optimization Inline __atomic operations when a lock free instruction sequence is available. +fcf-protection +Common RejectNegative Alias(fcf-protection=,full) + +fcf-protection= +Common 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. + +Enum +Name(cf_protection_level) Type(enum cf_protection_level) UnknownError(unknown Cotrol-Flow Protection Level %qs) + +EnumValue +Enum(cf_protection_level) String(full) Value(CF_FULL) + +EnumValue +Enum(cf_protection_level) String(branch) Value(CF_BRANCH) + +EnumValue +Enum(cf_protection_level) String(return) Value(CF_RETURN) + +EnumValue +Enum(cf_protection_level) String(none) Value(CF_NONE) + finstrument-functions Common Report Var(flag_instrument_function_entry_exit) Instrument function entry and exit with profiling calls. diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index 0a09998..3ddb41e 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -8689,6 +8689,45 @@ convert_for_assignment (tree type, tree rhs, } } + /* Check if the right-hand side and left-hand side mismatch in a + 'nocf_check' attribute. Both cases could be harmful: + NOCF = CF Implicit control-flow check from CF + is dropped as a future call is done through NOCF; + CF = NOCF Control-flow check will be done through the CF but + the check is not expected by NOCF. */ + + if (warn_ignored_attributes) + { + const enum tree_code codel = TREE_CODE (type); + if ((codel == POINTER_TYPE || codel == REFERENCE_TYPE) + && coder == codel + && check_missing_nocf_check_attribute (type, rhstype)) + { + switch (errtype) + { + case ICR_ARGPASS: + warning (OPT_Wattributes, + "nocf_check attribute mismatch for argument %d of %qE", + parmnum, fndecl); + break; + case ICR_ASSIGN: + warning (OPT_Wattributes, + "nocf_check attribute mismatch for assignment"); + break; + case ICR_INIT: + warning (OPT_Wattributes, + "nocf_check attribute mismatch for initialization"); + break; + case ICR_RETURN: + warning (OPT_Wattributes, + "nocf_check attribute mismatch for return type"); + break; + default: + gcc_unreachable (); + } + } + } + /* If -Wparentheses, warn about a = b = c when a has type bool and b does not. */ if (warn_parentheses diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index e36a7dd..b68729a 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -3791,6 +3791,7 @@ try_split (rtx pat, rtx_insn *trial, int last) case REG_NORETURN: case REG_SETJMP: case REG_TM: + case REG_CALL_NOCF_CHECK: for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn)) { if (CALL_P (insn)) diff --git a/gcc/flag-types.h b/gcc/flag-types.h index 4938f69..c0db3c9 100644 --- a/gcc/flag-types.h +++ b/gcc/flag-types.h @@ -318,4 +318,13 @@ enum gfc_convert }; +/* Control-Flow Protection values. */ +enum cf_protection_level +{ + CF_NONE = 0, + CF_BRANCH = 1 << 0, + CF_RETURN = 1 << 1, + CF_FULL = CF_BRANCH | CF_RETURN, + CF_SET = 1 << 2 +}; #endif /* ! GCC_FLAG_TYPES_H */ diff --git a/gcc/gimple.c b/gcc/gimple.c index c4e6f81..701cb18 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -346,7 +346,7 @@ gimple_build_call_internal_vec (enum internal_fn fn, vec<tree> args) this fact. */ gcall * -gimple_build_call_from_tree (tree t) +gimple_build_call_from_tree (tree t, tree fnptrtype) { unsigned i, nargs; gcall *call; @@ -380,6 +380,22 @@ 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 (fnptrtype) + { + gimple_call_set_fntype (call, TREE_TYPE (fnptrtype)); + + /* Check if a type has the no-track attribute and propagate + it to the indirect CALL insn. */ + if (TREE_CODE (fnptrtype) != FUNCTION_DECL) + { + gcc_assert (POINTER_TYPE_P (fnptrtype)); + tree fntype = TREE_TYPE (fnptrtype); + + if (lookup_attribute ("nocf_check", TYPE_ATTRIBUTES (fntype))) + gimple_call_set_nocf_check (call, TRUE); + } + } + return call; } diff --git a/gcc/gimple.h b/gcc/gimple.h index 6213c49..5dcc03d 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -148,6 +148,7 @@ enum gf_mask { GF_CALL_WITH_BOUNDS = 1 << 8, GF_CALL_MUST_TAIL_CALL = 1 << 9, GF_CALL_BY_DESCRIPTOR = 1 << 10, + GF_CALL_NOCF_CHECK = 1 << 11, GF_OMP_PARALLEL_COMBINED = 1 << 0, GF_OMP_PARALLEL_GRID_PHONY = 1 << 1, GF_OMP_TASK_TASKLOOP = 1 << 0, @@ -1425,7 +1426,7 @@ gcall *gimple_build_call (tree, unsigned, ...); gcall *gimple_build_call_valist (tree, unsigned, va_list); gcall *gimple_build_call_internal (enum internal_fn, unsigned, ...); gcall *gimple_build_call_internal_vec (enum internal_fn, vec<tree> ); -gcall *gimple_build_call_from_tree (tree); +gcall *gimple_build_call_from_tree (tree, tree); gassign *gimple_build_assign (tree, tree CXX_MEM_STAT_INFO); gassign *gimple_build_assign (tree, enum tree_code, tree, tree, tree CXX_MEM_STAT_INFO); @@ -2893,6 +2894,25 @@ gimple_call_set_with_bounds (gimple *gs, bool with_bounds) } +/* Return true if call GS is marked as no-track. */ + +static inline bool +gimple_call_nocf_check_p (const gcall *gs) +{ + return (gs->subcode & GF_CALL_NOCF_CHECK) != 0; +} + +/* Mark statement GS as no-track call. */ + +static inline void +gimple_call_set_nocf_check (gcall *gs, bool nocf_check) +{ + if (nocf_check) + gs->subcode |= GF_CALL_NOCF_CHECK; + else + gs->subcode &= ~GF_CALL_NOCF_CHECK; +} + /* Return the target of internal call GS. */ static inline enum internal_fn diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 8b29a71..7d55625 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -3379,8 +3379,7 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value) /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we have to do is replicate it as a GIMPLE_CALL tuple. */ gimple_stmt_iterator gsi; - call = gimple_build_call_from_tree (*expr_p); - gimple_call_set_fntype (call, TREE_TYPE (fnptrtype)); + call = gimple_build_call_from_tree (*expr_p, fnptrtype); notice_special_calls (call); if (EXPR_CILK_SPAWN (*expr_p)) gimplify_cilk_detach (pre_p); @@ -5656,8 +5655,7 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, CALL_EXPR_ARG (*from_p, 2)); else { - call_stmt = gimple_build_call_from_tree (*from_p); - gimple_call_set_fntype (call_stmt, TREE_TYPE (fnptrtype)); + call_stmt = gimple_build_call_from_tree (*from_p, fnptrtype); } } notice_special_calls (call_stmt); diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c index 4d152ce..e666d5a 100644 --- a/gcc/ipa-icf.c +++ b/gcc/ipa-icf.c @@ -1422,6 +1422,7 @@ sem_function::init (void) } } + hstate.commit_flag (); gcode_hash = hstate.end (); bb_sizes.safe_push (nondbg_stmt_count); @@ -1644,6 +1645,11 @@ sem_function::hash_stmt (gimple *stmt, inchash::hash &hstate) if (gimple_op (stmt, i)) add_type (TREE_TYPE (gimple_op (stmt, i)), hstate); } + /* Consider nocf_check attribute in hash as it affects code + generation. */ + if (code == GIMPLE_CALL + && flag_cf_protection & CF_BRANCH) + hstate.add_flag (gimple_call_nocf_check_p (as_a <gcall *> (stmt))); default: break; } diff --git a/gcc/recog.c b/gcc/recog.c index 78c26d6..ec51cd4 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -3382,6 +3382,7 @@ peep2_attempt (basic_block bb, rtx_insn *insn, int match_len, rtx_insn *attempt) case REG_NORETURN: case REG_SETJMP: case REG_TM: + case REG_CALL_NOCF_CHECK: add_reg_note (new_insn, REG_NOTE_KIND (note), XEXP (note, 0)); break; diff --git a/gcc/reg-notes.def b/gcc/reg-notes.def index 943eff4..cc456a6 100644 --- a/gcc/reg-notes.def +++ b/gcc/reg-notes.def @@ -228,3 +228,10 @@ REG_NOTE (RETURNED) The decl might not be available in the call due to splitting of the call insn. This note is a SYMBOL_REF. */ REG_NOTE (CALL_DECL) + +/* Indicate that a call should not be verified for control-flow consistency. + The target address of the call is assumed as a valid address and no check + to validate a branch to the target address is needed. The call is marked + when a called function has a 'notrack' attribute. This note is used by the + compiler when the option -fcf-protection=branch is specified. */ +REG_NOTE (CALL_NOCF_CHECK) diff --git a/gcc/toplev.c b/gcc/toplev.c index 7d2b8ff..9293172 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1297,6 +1297,32 @@ process_options (void) "-floop-parallelize-all)"); #endif + if (flag_cf_protection != CF_NONE + && flag_cf_protection != CF_SET) + { + if (flag_cf_protection == CF_FULL) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=full%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + if (flag_cf_protection == CF_BRANCH) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=branch%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + if (flag_cf_protection == CF_RETURN) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=return%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + } + if (flag_check_pointer_bounds) { if (targetm.chkp_bound_mode () == VOIDmode) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-19 13:39 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-28 22:44 ` Jeff Law 2017-09-29 14:31 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 0 siblings, 1 reply; 24+ messages in thread From: Jeff Law @ 2017-09-28 22:44 UTC (permalink / raw) To: Tsimbalist, Igor V, gcc-patches; +Cc: richard.guenther 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-28 22:44 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law @ 2017-09-29 14:31 ` Tsimbalist, Igor V 2017-09-29 16:04 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 0 siblings, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-09-29 14:31 UTC (permalink / raw) To: Jeff Law, gcc-patches; +Cc: richard.guenther, Tsimbalist, Igor V > -----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 > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-29 14:31 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-29 16:04 ` Tsimbalist, Igor V 2017-10-05 10:20 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 0 siblings, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-09-29 16:04 UTC (permalink / raw) To: Jeff Law, gcc-patches; +Cc: richard.guenther, Tsimbalist, Igor V [-- Attachment #1: Type: text/plain, Size: 7172 bytes --] 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 > > > > > > [-- Attachment #2: 0001-Add-generic-part-for-Intel-CET-enabling-fcf-protecti.patch --] [-- Type: application/octet-stream, Size: 22368 bytes --] From b8a41d6ef39240cfd412cfcbb8542f9b97bf38d9 Mon Sep 17 00:00:00 2001 From: Igor Tsimbalist <igor.v.tsimbalist@intel.com> Date: Mon, 3 Jul 2017 17:11:58 +0300 Subject: [PATCH 1/3] Add generic part for Intel CET enabling: fcf-protection, nocf_check The spec is available at https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf 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. * 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. --- gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ gcc/c-family/c-common.c | 20 ++++++++++++++++++++ gcc/c-family/c-common.h | 1 + gcc/c/c-typeck.c | 37 +++++++++++++++++++++++++++++++++++++ gcc/c/gimple-parser.c | 4 ++-- gcc/cfgexpand.c | 16 ++++++++++++++++ gcc/combine.c | 1 + gcc/common.opt | 23 +++++++++++++++++++++++ gcc/cp/typeck.c | 39 +++++++++++++++++++++++++++++++++++++++ gcc/emit-rtl.c | 1 + gcc/flag-types.h | 9 +++++++++ gcc/gimple.c | 18 +++++++++++++++++- gcc/gimple.h | 22 +++++++++++++++++++++- gcc/gimplify.c | 6 ++---- gcc/ipa-icf.c | 6 ++++++ gcc/recog.c | 1 + gcc/reg-notes.def | 7 +++++++ gcc/toplev.c | 26 ++++++++++++++++++++++++++ 18 files changed, 252 insertions(+), 8 deletions(-) --- gcc/c-family/c-attribs.c | 21 +++++++++++++++++++++ gcc/c-family/c-common.c | 24 ++++++++++++++++++++++++ gcc/c-family/c-common.h | 1 + gcc/c/c-typeck.c | 37 +++++++++++++++++++++++++++++++++++++ gcc/c/gimple-parser.c | 4 ++-- gcc/cfgexpand.c | 16 ++++++++++++++++ gcc/combine.c | 1 + gcc/common.opt | 23 +++++++++++++++++++++++ gcc/cp/typeck.c | 39 +++++++++++++++++++++++++++++++++++++++ gcc/emit-rtl.c | 1 + gcc/flag-types.h | 9 +++++++++ gcc/gimple.c | 18 +++++++++++++++++- gcc/gimple.h | 22 +++++++++++++++++++++- gcc/gimplify.c | 6 ++---- gcc/ipa-icf.c | 6 ++++++ gcc/recog.c | 1 + gcc/reg-notes.def | 7 +++++++ gcc/toplev.c | 26 ++++++++++++++++++++++++++ 18 files changed, 254 insertions(+), 8 deletions(-) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 0337537..cdb9f02 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,24 @@ 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) + { + warning (OPT_Wattributes, "%qE attribute ignored", name); + *no_add_attrs = true; + } + + return NULL_TREE; +} + /* Handle a "no_icf" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index b3ec3a0..a8a4326 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -7253,6 +7253,30 @@ check_missing_format_attribute (tree ltype, tree rtype) return false; } +/* Check for missing nocf_check attributes on function pointers. + LTYPE is the left-hand side type. RTYPE is the right-hand side + type. Returns TRUE if either type has the attribute and another + does not. Returns FALSE if both types have or miss the 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; + if ((la && ra) + || (la == ra)) + return false; + else + return true; +} + /* Setup a TYPE_DECL node as a typedef representation. X is a TYPE_DECL for a typedef statement. Create a brand new diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 0de549d..829ea8e 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1123,6 +1123,7 @@ extern void preprocess_file (cpp_reader *); extern void pp_file_change (const line_map_ordinary *); extern void pp_dir_change (cpp_reader *, const char *); extern bool check_missing_format_attribute (tree, tree); +extern bool check_missing_nocf_check_attribute (tree, tree); /* In c-omp.c */ struct omp_clause_mask diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index f45fd3c..cd3db7d 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -6826,6 +6826,43 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type, G_("return makes %q#v qualified function " "pointer from unqualified"), TYPE_QUALS (ttl) & ~TYPE_QUALS (ttr)); + + /* Check if the right-hand side and left-hand side mismatch in a + 'nocf_check' attribute. Both cases could be harmful: + NOCF = CF Implicit control-flow check from CF + is dropped as a future call is done through NOCF; + CF = NOCF Control-flow check will be done through the CF but + the check is not expected by NOCF. */ + if (warn_ignored_attributes + && check_missing_nocf_check_attribute (type, rhstype)) + { + switch (errtype) + { + case ic_argpass: + warning_at (expr_loc, OPT_Wattributes, + "nocf_check attribute mismatch for" + " argument %d of %qE", + parmnum, rname); + break; + case ic_assign: + warning_at (location, OPT_Wattributes, + "nocf_check attribute mismatch for" + " assignment"); + break; + case ic_init: + warning_at (location, OPT_Wattributes, + "nocf_check attribute mismatch for" + " initialization"); + break; + case ic_return: + warning_at (location, OPT_Wattributes, + "nocf_check attribute mismatch for" + " return type"); + break; + default: + gcc_unreachable (); + } + } } } /* Avoid warning about the volatile ObjC EH puts on decls. */ diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index 22f58f4..c2e31df 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -276,7 +276,7 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) && TREE_CODE (lhs.value) == CALL_EXPR) { gimple *call; - call = gimple_build_call_from_tree (lhs.value); + call = gimple_build_call_from_tree (lhs.value, NULL); gimple_seq_add_stmt (seq, call); gimple_set_location (call, loc); return; @@ -407,7 +407,7 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) rhs = c_parser_gimple_unary_expression (parser); if (rhs.value != error_mark_node) { - gimple *call = gimple_build_call_from_tree (rhs.value); + gimple *call = gimple_build_call_from_tree (rhs.value, NULL); gimple_call_set_lhs (call, lhs.value); gimple_seq_add_stmt (seq, call); gimple_set_location (call, loc); diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 7657a65..3bcaa4c 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -2659,12 +2659,28 @@ expand_call_stmt (gcall *stmt) } } + rtx_insn *before_call = get_last_insn (); lhs = gimple_call_lhs (stmt); if (lhs) expand_assignment (lhs, exp, false); else expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); + /* If the gimple call is an indirect call and has 'nocf_check' + attribute find a generated CALL insn to mark it as no + control-flow verification is needed. */ + if (gimple_call_nocf_check_p (stmt) + && !gimple_call_fndecl (stmt)) + { + rtx_insn *last = get_last_insn (); + while (!CALL_P (last) + && last != before_call) + last = PREV_INSN (last); + + if (last != before_call) + add_reg_note (last, REG_CALL_NOCF_CHECK, const0_rtx); + } + mark_transaction_restart_calls (stmt); } diff --git a/gcc/combine.c b/gcc/combine.c index 1832c3c..e0315fd 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -14151,6 +14151,7 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, case REG_SETJMP: case REG_TM: case REG_CALL_DECL: + case REG_CALL_NOCF_CHECK: /* These notes must remain with the call. It should not be possible for both I2 and I3 to be a call. */ if (CALL_P (i3)) diff --git a/gcc/common.opt b/gcc/common.opt index 1581ca8..b06bede 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1608,6 +1608,29 @@ finline-atomics Common Report Var(flag_inline_atomics) Init(1) Optimization Inline __atomic operations when a lock free instruction sequence is available. +fcf-protection +Common RejectNegative Alias(fcf-protection=,full) + +fcf-protection= +Common 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. + +Enum +Name(cf_protection_level) Type(enum cf_protection_level) UnknownError(unknown Cotrol-Flow Protection Level %qs) + +EnumValue +Enum(cf_protection_level) String(full) Value(CF_FULL) + +EnumValue +Enum(cf_protection_level) String(branch) Value(CF_BRANCH) + +EnumValue +Enum(cf_protection_level) String(return) Value(CF_RETURN) + +EnumValue +Enum(cf_protection_level) String(none) Value(CF_NONE) + finstrument-functions Common Report Var(flag_instrument_function_entry_exit) Instrument function entry and exit with profiling calls. diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index 0a09998..3ddb41e 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -8689,6 +8689,45 @@ convert_for_assignment (tree type, tree rhs, } } + /* Check if the right-hand side and left-hand side mismatch in a + 'nocf_check' attribute. Both cases could be harmful: + NOCF = CF Implicit control-flow check from CF + is dropped as a future call is done through NOCF; + CF = NOCF Control-flow check will be done through the CF but + the check is not expected by NOCF. */ + + if (warn_ignored_attributes) + { + const enum tree_code codel = TREE_CODE (type); + if ((codel == POINTER_TYPE || codel == REFERENCE_TYPE) + && coder == codel + && check_missing_nocf_check_attribute (type, rhstype)) + { + switch (errtype) + { + case ICR_ARGPASS: + warning (OPT_Wattributes, + "nocf_check attribute mismatch for argument %d of %qE", + parmnum, fndecl); + break; + case ICR_ASSIGN: + warning (OPT_Wattributes, + "nocf_check attribute mismatch for assignment"); + break; + case ICR_INIT: + warning (OPT_Wattributes, + "nocf_check attribute mismatch for initialization"); + break; + case ICR_RETURN: + warning (OPT_Wattributes, + "nocf_check attribute mismatch for return type"); + break; + default: + gcc_unreachable (); + } + } + } + /* If -Wparentheses, warn about a = b = c when a has type bool and b does not. */ if (warn_parentheses diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index e36a7dd..b68729a 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -3791,6 +3791,7 @@ try_split (rtx pat, rtx_insn *trial, int last) case REG_NORETURN: case REG_SETJMP: case REG_TM: + case REG_CALL_NOCF_CHECK: for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn)) { if (CALL_P (insn)) diff --git a/gcc/flag-types.h b/gcc/flag-types.h index 4938f69..c0db3c9 100644 --- a/gcc/flag-types.h +++ b/gcc/flag-types.h @@ -318,4 +318,13 @@ enum gfc_convert }; +/* Control-Flow Protection values. */ +enum cf_protection_level +{ + CF_NONE = 0, + CF_BRANCH = 1 << 0, + CF_RETURN = 1 << 1, + CF_FULL = CF_BRANCH | CF_RETURN, + CF_SET = 1 << 2 +}; #endif /* ! GCC_FLAG_TYPES_H */ diff --git a/gcc/gimple.c b/gcc/gimple.c index c4e6f81..701cb18 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -346,7 +346,7 @@ gimple_build_call_internal_vec (enum internal_fn fn, vec<tree> args) this fact. */ gcall * -gimple_build_call_from_tree (tree t) +gimple_build_call_from_tree (tree t, tree fnptrtype) { unsigned i, nargs; gcall *call; @@ -380,6 +380,22 @@ 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 (fnptrtype) + { + gimple_call_set_fntype (call, TREE_TYPE (fnptrtype)); + + /* Check if a type has the no-track attribute and propagate + it to the indirect CALL insn. */ + if (TREE_CODE (fnptrtype) != FUNCTION_DECL) + { + gcc_assert (POINTER_TYPE_P (fnptrtype)); + tree fntype = TREE_TYPE (fnptrtype); + + if (lookup_attribute ("nocf_check", TYPE_ATTRIBUTES (fntype))) + gimple_call_set_nocf_check (call, TRUE); + } + } + return call; } diff --git a/gcc/gimple.h b/gcc/gimple.h index 6213c49..5dcc03d 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -148,6 +148,7 @@ enum gf_mask { GF_CALL_WITH_BOUNDS = 1 << 8, GF_CALL_MUST_TAIL_CALL = 1 << 9, GF_CALL_BY_DESCRIPTOR = 1 << 10, + GF_CALL_NOCF_CHECK = 1 << 11, GF_OMP_PARALLEL_COMBINED = 1 << 0, GF_OMP_PARALLEL_GRID_PHONY = 1 << 1, GF_OMP_TASK_TASKLOOP = 1 << 0, @@ -1425,7 +1426,7 @@ gcall *gimple_build_call (tree, unsigned, ...); gcall *gimple_build_call_valist (tree, unsigned, va_list); gcall *gimple_build_call_internal (enum internal_fn, unsigned, ...); gcall *gimple_build_call_internal_vec (enum internal_fn, vec<tree> ); -gcall *gimple_build_call_from_tree (tree); +gcall *gimple_build_call_from_tree (tree, tree); gassign *gimple_build_assign (tree, tree CXX_MEM_STAT_INFO); gassign *gimple_build_assign (tree, enum tree_code, tree, tree, tree CXX_MEM_STAT_INFO); @@ -2893,6 +2894,25 @@ gimple_call_set_with_bounds (gimple *gs, bool with_bounds) } +/* Return true if call GS is marked as no-track. */ + +static inline bool +gimple_call_nocf_check_p (const gcall *gs) +{ + return (gs->subcode & GF_CALL_NOCF_CHECK) != 0; +} + +/* Mark statement GS as no-track call. */ + +static inline void +gimple_call_set_nocf_check (gcall *gs, bool nocf_check) +{ + if (nocf_check) + gs->subcode |= GF_CALL_NOCF_CHECK; + else + gs->subcode &= ~GF_CALL_NOCF_CHECK; +} + /* Return the target of internal call GS. */ static inline enum internal_fn diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 8b29a71..7d55625 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -3379,8 +3379,7 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value) /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we have to do is replicate it as a GIMPLE_CALL tuple. */ gimple_stmt_iterator gsi; - call = gimple_build_call_from_tree (*expr_p); - gimple_call_set_fntype (call, TREE_TYPE (fnptrtype)); + call = gimple_build_call_from_tree (*expr_p, fnptrtype); notice_special_calls (call); if (EXPR_CILK_SPAWN (*expr_p)) gimplify_cilk_detach (pre_p); @@ -5656,8 +5655,7 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, CALL_EXPR_ARG (*from_p, 2)); else { - call_stmt = gimple_build_call_from_tree (*from_p); - gimple_call_set_fntype (call_stmt, TREE_TYPE (fnptrtype)); + call_stmt = gimple_build_call_from_tree (*from_p, fnptrtype); } } notice_special_calls (call_stmt); diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c index 4d152ce..e666d5a 100644 --- a/gcc/ipa-icf.c +++ b/gcc/ipa-icf.c @@ -1422,6 +1422,7 @@ sem_function::init (void) } } + hstate.commit_flag (); gcode_hash = hstate.end (); bb_sizes.safe_push (nondbg_stmt_count); @@ -1644,6 +1645,11 @@ sem_function::hash_stmt (gimple *stmt, inchash::hash &hstate) if (gimple_op (stmt, i)) add_type (TREE_TYPE (gimple_op (stmt, i)), hstate); } + /* Consider nocf_check attribute in hash as it affects code + generation. */ + if (code == GIMPLE_CALL + && flag_cf_protection & CF_BRANCH) + hstate.add_flag (gimple_call_nocf_check_p (as_a <gcall *> (stmt))); default: break; } diff --git a/gcc/recog.c b/gcc/recog.c index 78c26d6..ec51cd4 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -3382,6 +3382,7 @@ peep2_attempt (basic_block bb, rtx_insn *insn, int match_len, rtx_insn *attempt) case REG_NORETURN: case REG_SETJMP: case REG_TM: + case REG_CALL_NOCF_CHECK: add_reg_note (new_insn, REG_NOTE_KIND (note), XEXP (note, 0)); break; diff --git a/gcc/reg-notes.def b/gcc/reg-notes.def index 943eff4..cc456a6 100644 --- a/gcc/reg-notes.def +++ b/gcc/reg-notes.def @@ -228,3 +228,10 @@ REG_NOTE (RETURNED) The decl might not be available in the call due to splitting of the call insn. This note is a SYMBOL_REF. */ REG_NOTE (CALL_DECL) + +/* Indicate that a call should not be verified for control-flow consistency. + The target address of the call is assumed as a valid address and no check + to validate a branch to the target address is needed. The call is marked + when a called function has a 'notrack' attribute. This note is used by the + compiler when the option -fcf-protection=branch is specified. */ +REG_NOTE (CALL_NOCF_CHECK) diff --git a/gcc/toplev.c b/gcc/toplev.c index 7d2b8ff..3cb1124 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1297,6 +1297,32 @@ process_options (void) "-floop-parallelize-all)"); #endif + if (flag_cf_protection != CF_NONE + && !(flag_cf_protection & CF_SET)) + { + if (flag_cf_protection == CF_FULL) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=full%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + if (flag_cf_protection == CF_BRANCH) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=branch%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + if (flag_cf_protection == CF_RETURN) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=return%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + } + if (flag_check_pointer_bounds) { if (targetm.chkp_bound_mode () == VOIDmode) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-29 16:04 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-10-05 10:20 ` Tsimbalist, Igor V 2017-10-12 6:26 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 0 siblings, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-10-05 10:20 UTC (permalink / raw) To: Jeff Law, gcc-patches; +Cc: richard.guenther, Tsimbalist, Igor V 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 > > > > > > > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-10-05 10:20 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-10-12 6:26 ` Jeff Law 2017-10-12 8:33 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 0 siblings, 1 reply; 24+ messages in thread From: Jeff Law @ 2017-10-12 6:26 UTC (permalink / raw) To: Tsimbalist, Igor V, gcc-patches; +Cc: richard.guenther 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-10-12 6:26 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law @ 2017-10-12 8:33 ` Tsimbalist, Igor V 2017-10-12 15:15 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 0 siblings, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-10-12 8:33 UTC (permalink / raw) To: Jeff Law, gcc-patches; +Cc: richard.guenther, Tsimbalist, Igor V [-- Attachment #1: Type: text/plain, Size: 1988 bytes --] > 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 [-- Attachment #2: 0001-Add-generic-part-for-Intel-CET-enabling-fcf-protecti.patch --] [-- Type: application/octet-stream, Size: 17154 bytes --] From ecebc52923cddad06a2d9934996545193582bb4f Mon Sep 17 00:00:00 2001 From: Igor Tsimbalist <igor.v.tsimbalist@intel.com> Date: Mon, 3 Jul 2017 17:11:58 +0300 Subject: [PATCH 1/6] Add generic part for Intel CET enabling: fcf-protection, nocf_check The spec is available at https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf 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. --- gcc/attribs.c | 3 +++ gcc/c-family/c-attribs.c | 27 +++++++++++++++++++++++++++ gcc/c/gimple-parser.c | 4 ++-- gcc/cfgexpand.c | 16 ++++++++++++++++ gcc/combine.c | 1 + gcc/common.opt | 23 +++++++++++++++++++++++ gcc/emit-rtl.c | 1 + gcc/flag-types.h | 9 +++++++++ gcc/gimple.c | 19 ++++++++++++++++++- gcc/gimple.h | 22 +++++++++++++++++++++- gcc/gimplify.c | 6 ++---- gcc/ipa-icf.c | 6 ++++++ gcc/recog.c | 1 + gcc/reg-notes.def | 7 +++++++ gcc/toplev.c | 26 ++++++++++++++++++++++++++ 15 files changed, 163 insertions(+), 8 deletions(-) diff --git a/gcc/attribs.c b/gcc/attribs.c index 4ef35b8..ed76a8d 100644 --- a/gcc/attribs.c +++ b/gcc/attribs.c @@ -1182,6 +1182,9 @@ comp_type_attributes (const_tree type1, const_tree type2) } if (lookup_attribute ("transaction_safe", CONST_CAST_TREE (a))) return 0; + if ((lookup_attribute ("nocf_check", TYPE_ATTRIBUTES (type1)) != NULL) + ^ (lookup_attribute ("nocf_check", TYPE_ATTRIBUTES (type2)) != NULL)) + return 0; /* As some type combinations - like default calling-convention - might be compatible, we have to call the target hook to get the final result. */ return targetm.comp_type_attributes (type1, type2); diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 0337537..4f6597a 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, true }, { NULL, 0, 0, false, false, false, NULL, false } }; @@ -783,6 +786,30 @@ 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) + { + warning (OPT_Wattributes, "%qE attribute ignored", name); + *no_add_attrs = true; + } + else if (!(flag_cf_protection & CF_BRANCH)) + { + warning (OPT_Wattributes, "%qE attribute ignored. Use " + "-fcf-protection option to enable it", name); + *no_add_attrs = true; + } + + return NULL_TREE; +} + /* Handle a "no_icf" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index 22f58f4..c2e31df 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -276,7 +276,7 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) && TREE_CODE (lhs.value) == CALL_EXPR) { gimple *call; - call = gimple_build_call_from_tree (lhs.value); + call = gimple_build_call_from_tree (lhs.value, NULL); gimple_seq_add_stmt (seq, call); gimple_set_location (call, loc); return; @@ -407,7 +407,7 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) rhs = c_parser_gimple_unary_expression (parser); if (rhs.value != error_mark_node) { - gimple *call = gimple_build_call_from_tree (rhs.value); + gimple *call = gimple_build_call_from_tree (rhs.value, NULL); gimple_call_set_lhs (call, lhs.value); gimple_seq_add_stmt (seq, call); gimple_set_location (call, loc); diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 7657a65..3bcaa4c 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -2659,12 +2659,28 @@ expand_call_stmt (gcall *stmt) } } + rtx_insn *before_call = get_last_insn (); lhs = gimple_call_lhs (stmt); if (lhs) expand_assignment (lhs, exp, false); else expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); + /* If the gimple call is an indirect call and has 'nocf_check' + attribute find a generated CALL insn to mark it as no + control-flow verification is needed. */ + if (gimple_call_nocf_check_p (stmt) + && !gimple_call_fndecl (stmt)) + { + rtx_insn *last = get_last_insn (); + while (!CALL_P (last) + && last != before_call) + last = PREV_INSN (last); + + if (last != before_call) + add_reg_note (last, REG_CALL_NOCF_CHECK, const0_rtx); + } + mark_transaction_restart_calls (stmt); } diff --git a/gcc/combine.c b/gcc/combine.c index 1832c3c..e0315fd 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -14151,6 +14151,7 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, case REG_SETJMP: case REG_TM: case REG_CALL_DECL: + case REG_CALL_NOCF_CHECK: /* These notes must remain with the call. It should not be possible for both I2 and I3 to be a call. */ if (CALL_P (i3)) diff --git a/gcc/common.opt b/gcc/common.opt index 1581ca8..b06bede 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1608,6 +1608,29 @@ finline-atomics Common Report Var(flag_inline_atomics) Init(1) Optimization Inline __atomic operations when a lock free instruction sequence is available. +fcf-protection +Common RejectNegative Alias(fcf-protection=,full) + +fcf-protection= +Common 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. + +Enum +Name(cf_protection_level) Type(enum cf_protection_level) UnknownError(unknown Cotrol-Flow Protection Level %qs) + +EnumValue +Enum(cf_protection_level) String(full) Value(CF_FULL) + +EnumValue +Enum(cf_protection_level) String(branch) Value(CF_BRANCH) + +EnumValue +Enum(cf_protection_level) String(return) Value(CF_RETURN) + +EnumValue +Enum(cf_protection_level) String(none) Value(CF_NONE) + finstrument-functions Common Report Var(flag_instrument_function_entry_exit) Instrument function entry and exit with profiling calls. diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index e36a7dd..b68729a 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -3791,6 +3791,7 @@ try_split (rtx pat, rtx_insn *trial, int last) case REG_NORETURN: case REG_SETJMP: case REG_TM: + case REG_CALL_NOCF_CHECK: for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn)) { if (CALL_P (insn)) diff --git a/gcc/flag-types.h b/gcc/flag-types.h index 4938f69..c0db3c9 100644 --- a/gcc/flag-types.h +++ b/gcc/flag-types.h @@ -318,4 +318,13 @@ enum gfc_convert }; +/* Control-Flow Protection values. */ +enum cf_protection_level +{ + CF_NONE = 0, + CF_BRANCH = 1 << 0, + CF_RETURN = 1 << 1, + CF_FULL = CF_BRANCH | CF_RETURN, + CF_SET = 1 << 2 +}; #endif /* ! GCC_FLAG_TYPES_H */ diff --git a/gcc/gimple.c b/gcc/gimple.c index c4e6f81..b01aec5 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -346,7 +346,7 @@ gimple_build_call_internal_vec (enum internal_fn fn, vec<tree> args) this fact. */ gcall * -gimple_build_call_from_tree (tree t) +gimple_build_call_from_tree (tree t, tree fnptrtype) { unsigned i, nargs; gcall *call; @@ -380,6 +380,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 (fnptrtype) + { + gimple_call_set_fntype (call, TREE_TYPE (fnptrtype)); + + /* Check if it's an indirect CALL and the type has the + nocf_check attribute. In that case propagate the information + to the gimple CALL insn. */ + if (!fndecl) + { + gcc_assert (POINTER_TYPE_P (fnptrtype)); + tree fntype = TREE_TYPE (fnptrtype); + + if (lookup_attribute ("nocf_check", TYPE_ATTRIBUTES (fntype))) + gimple_call_set_nocf_check (call, TRUE); + } + } + return call; } diff --git a/gcc/gimple.h b/gcc/gimple.h index 6213c49..334def8 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -148,6 +148,7 @@ enum gf_mask { GF_CALL_WITH_BOUNDS = 1 << 8, GF_CALL_MUST_TAIL_CALL = 1 << 9, GF_CALL_BY_DESCRIPTOR = 1 << 10, + GF_CALL_NOCF_CHECK = 1 << 11, GF_OMP_PARALLEL_COMBINED = 1 << 0, GF_OMP_PARALLEL_GRID_PHONY = 1 << 1, GF_OMP_TASK_TASKLOOP = 1 << 0, @@ -1425,7 +1426,7 @@ gcall *gimple_build_call (tree, unsigned, ...); gcall *gimple_build_call_valist (tree, unsigned, va_list); gcall *gimple_build_call_internal (enum internal_fn, unsigned, ...); gcall *gimple_build_call_internal_vec (enum internal_fn, vec<tree> ); -gcall *gimple_build_call_from_tree (tree); +gcall *gimple_build_call_from_tree (tree, tree); gassign *gimple_build_assign (tree, tree CXX_MEM_STAT_INFO); gassign *gimple_build_assign (tree, enum tree_code, tree, tree, tree CXX_MEM_STAT_INFO); @@ -2893,6 +2894,25 @@ gimple_call_set_with_bounds (gimple *gs, bool with_bounds) } +/* Return true if call GS is marked as nocf_check. */ + +static inline bool +gimple_call_nocf_check_p (const gcall *gs) +{ + return (gs->subcode & GF_CALL_NOCF_CHECK) != 0; +} + +/* Mark statement GS as nocf_check call. */ + +static inline void +gimple_call_set_nocf_check (gcall *gs, bool nocf_check) +{ + if (nocf_check) + gs->subcode |= GF_CALL_NOCF_CHECK; + else + gs->subcode &= ~GF_CALL_NOCF_CHECK; +} + /* Return the target of internal call GS. */ static inline enum internal_fn diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 8b29a71..7d55625 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -3379,8 +3379,7 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value) /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we have to do is replicate it as a GIMPLE_CALL tuple. */ gimple_stmt_iterator gsi; - call = gimple_build_call_from_tree (*expr_p); - gimple_call_set_fntype (call, TREE_TYPE (fnptrtype)); + call = gimple_build_call_from_tree (*expr_p, fnptrtype); notice_special_calls (call); if (EXPR_CILK_SPAWN (*expr_p)) gimplify_cilk_detach (pre_p); @@ -5656,8 +5655,7 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, CALL_EXPR_ARG (*from_p, 2)); else { - call_stmt = gimple_build_call_from_tree (*from_p); - gimple_call_set_fntype (call_stmt, TREE_TYPE (fnptrtype)); + call_stmt = gimple_build_call_from_tree (*from_p, fnptrtype); } } notice_special_calls (call_stmt); diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c index 4d152ce..e666d5a 100644 --- a/gcc/ipa-icf.c +++ b/gcc/ipa-icf.c @@ -1422,6 +1422,7 @@ sem_function::init (void) } } + hstate.commit_flag (); gcode_hash = hstate.end (); bb_sizes.safe_push (nondbg_stmt_count); @@ -1644,6 +1645,11 @@ sem_function::hash_stmt (gimple *stmt, inchash::hash &hstate) if (gimple_op (stmt, i)) add_type (TREE_TYPE (gimple_op (stmt, i)), hstate); } + /* Consider nocf_check attribute in hash as it affects code + generation. */ + if (code == GIMPLE_CALL + && flag_cf_protection & CF_BRANCH) + hstate.add_flag (gimple_call_nocf_check_p (as_a <gcall *> (stmt))); default: break; } diff --git a/gcc/recog.c b/gcc/recog.c index 78c26d6..ec51cd4 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -3382,6 +3382,7 @@ peep2_attempt (basic_block bb, rtx_insn *insn, int match_len, rtx_insn *attempt) case REG_NORETURN: case REG_SETJMP: case REG_TM: + case REG_CALL_NOCF_CHECK: add_reg_note (new_insn, REG_NOTE_KIND (note), XEXP (note, 0)); break; diff --git a/gcc/reg-notes.def b/gcc/reg-notes.def index 943eff4..cc456a6 100644 --- a/gcc/reg-notes.def +++ b/gcc/reg-notes.def @@ -228,3 +228,10 @@ REG_NOTE (RETURNED) The decl might not be available in the call due to splitting of the call insn. This note is a SYMBOL_REF. */ REG_NOTE (CALL_DECL) + +/* Indicate that a call should not be verified for control-flow consistency. + The target address of the call is assumed as a valid address and no check + to validate a branch to the target address is needed. The call is marked + when a called function has a 'notrack' attribute. This note is used by the + compiler when the option -fcf-protection=branch is specified. */ +REG_NOTE (CALL_NOCF_CHECK) diff --git a/gcc/toplev.c b/gcc/toplev.c index 7d2b8ff..3cb1124 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1297,6 +1297,32 @@ process_options (void) "-floop-parallelize-all)"); #endif + if (flag_cf_protection != CF_NONE + && !(flag_cf_protection & CF_SET)) + { + if (flag_cf_protection == CF_FULL) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=full%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + if (flag_cf_protection == CF_BRANCH) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=branch%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + if (flag_cf_protection == CF_RETURN) + { + error_at (UNKNOWN_LOCATION, + "%<-fcf-protection=return%> is not supported for this " + "target"); + flag_cf_protection = CF_NONE; + } + } + if (flag_check_pointer_bounds) { if (targetm.chkp_bound_mode () == VOIDmode) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-10-12 8:33 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-10-12 15:15 ` Jeff Law 0 siblings, 0 replies; 24+ messages in thread From: Jeff Law @ 2017-10-12 15:15 UTC (permalink / raw) To: Tsimbalist, Igor V, gcc-patches; +Cc: richard.guenther 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-15 14:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2017-08-18 14:01 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-08-25 21:03 ` Jeff Law 2017-09-12 15:40 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 1 sibling, 1 reply; 24+ messages in thread From: Jeff Law @ 2017-08-25 21:03 UTC (permalink / raw) To: Richard Biener, Tsimbalist, Igor V; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-25 21:03 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law @ 2017-09-12 15:40 ` Tsimbalist, Igor V 2017-09-13 19:05 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 0 siblings, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-09-12 15:40 UTC (permalink / raw) To: Jeff Law, Richard Biener; +Cc: gcc-patches, Tsimbalist, Igor V > -----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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-12 15:40 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-13 19:05 ` Jeff Law 0 siblings, 0 replies; 24+ messages in thread From: Jeff Law @ 2017-09-13 19:05 UTC (permalink / raw) To: Tsimbalist, Igor V, Richard Biener; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-01 8:56 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-08-15 14:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener @ 2017-08-25 21:05 ` Jeff Law 2017-09-12 15:59 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-13 17:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 1 sibling, 2 replies; 24+ messages in thread From: Jeff Law @ 2017-08-25 21:05 UTC (permalink / raw) To: Tsimbalist, Igor V, 'gcc-patches@gcc.gnu.org' On 08/01/2017 02:56 AM, Tsimbalist, Igor V 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-flow-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). > > > 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling.patch > > > From 403fc8239fb1f690cc378287b4def57dcc9d25bf Mon Sep 17 00:00:00 2001 > From: Igor Tsimbalist <igor.v.tsimbalist@intel.com> > Date: Mon, 3 Jul 2017 17:11:58 +0300 > Subject: [PATCH 1/9] 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-flow-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). > > gcc/c-family/ > > * c-attribs.c (handle_notrack_attribute): New function. > (c_common_attribute_table): Add 'notrack' handling. > > gcc/ > > * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOTRACK. > * combine.c (distribute_notes): Add REG_CALL_NOTRACK handling. > * common.opt: Add finstrument-control-flow flag. > * emit-rtl.c (try_split): Add REG_CALL_NOTRACK handling. > * gimple.c (gimple_build_call_from_tree): Add 'notrack' > propogation. > * gimple.h (gf_mask): Add GF_CALL_WITH_NOTRACK. > (gimple_call_with_notrack_p): function. > (gimple_call_set_with_notrack): Likewise. > * recog.c (peep2_attempt): Add REG_CALL_NOTRACK handling. > * reg-notes.def: Add REG_NOTE (CALL_NOTRACK). > * toplev.c (process_options): Add flag_instrument_control_flow > handling. > --- > gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ > gcc/cfgexpand.c | 11 +++++++++++ > gcc/combine.c | 1 + > gcc/common.opt | 5 +++++ > gcc/emit-rtl.c | 1 + > gcc/gimple.c | 17 +++++++++++++++++ > gcc/gimple.h | 34 ++++++++++++++++++++++++++++++++++ > gcc/recog.c | 1 + > gcc/reg-notes.def | 7 +++++++ > gcc/toplev.c | 11 +++++++++++ > 10 files changed, 111 insertions(+) > --- > gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ > gcc/cfgexpand.c | 11 +++++++++++ > gcc/combine.c | 1 + > gcc/common.opt | 5 +++++ > gcc/emit-rtl.c | 1 + > gcc/gimple.c | 17 +++++++++++++++++ > gcc/gimple.h | 34 ++++++++++++++++++++++++++++++++++ > gcc/recog.c | 1 + > gcc/reg-notes.def | 7 +++++++ > gcc/toplev.c | 11 +++++++++++ > 10 files changed, 111 insertions(+) > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > index 0d9ab2d..4d62f7c 100644 > --- a/gcc/c-family/c-attribs.c > +++ b/gcc/c-family/c-attribs.c > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c > index c9d8118..ed344c5 100644 > --- a/gcc/cfgexpand.c > +++ b/gcc/cfgexpand.c > @@ -2657,12 +2657,23 @@ expand_call_stmt (gcall *stmt) > } > } > > + rtx_insn *before_call = get_last_insn (); > lhs = gimple_call_lhs (stmt); > if (lhs) > expand_assignment (lhs, exp, false); > else > expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); > > + /* Find a generated CALL insn to propagate a 'notrack' attribute. */ > + rtx_insn *last = get_last_insn (); > + while (!CALL_P (last) > + && (last != before_call)) > + last = PREV_INSN (last); > + > + if (last != before_call > + && gimple_call_with_notrack_p (stmt)) > + add_reg_note (last, REG_CALL_NOTRACK, const0_rtx); > + > mark_transaction_restart_calls (stmt); > } Just to be sure -- this is only searching through the currently open sequence to find the RTL call emitted into the sequence to implement the gimple call we're expanding, right? ISTM that you don't need to do the search if !gimple_call_with_notrack_p, right? Which would imply this code should look something like if (gimple_call_with_notrack_p (stmt)) { rtx insn *last = get_last_insn (); while (!CALL_P (last) && last != before_call) last = PREV_INSN (last); add_reg_note (last, REG_CALL_NOTRACK, const0_rtx) } Or something close to that? I'd be slightly concerned if expansion of an argument resulted in a call. I'm not sure if that can happen anymore, but if it does, then you could end up marking the wrong CALL_INSN. THe only cases where I think that might be possible would be if we needed to emit a libcall or memcpy. So perhaps you want to verify that last is a call *and* that it's an indirect call. > 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); > + } > + } > + > return call; As Richi noted, this deserves a better comment. It may even be advisable to build a predicate to test if a given gimple statement is an indirect call and use that rather than fndecl == NULL_TREE. That would help make this code clearer as well. Q. Do we need to do anything with ICF (identical code folding) and CFE? Given two functions which have the same implementation in gimple, except that one has a notrack indirect call and the other has a tracked indirect call, what is proper behavior? I think we'd keep them separate which implies we need to make sure the notrack attribute is part of the ICF hashing implementation. It'd probably even be worth building a test for this :-) > } > > > +/* Return true if call GS is marked as no-track. */ > + > +static inline bool > +gimple_call_with_notrack_p (const gcall *gs) > +{ > + return (gs->subcode & GF_CALL_WITH_NOTRACK) != 0; > +} > + > +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); > +} Agree with Richi WRT avoiding gimple * overloads. Jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-25 21:05 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law @ 2017-09-12 15:59 ` Tsimbalist, Igor V 2017-09-13 18:56 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-09-13 17:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 1 sibling, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-09-12 15:59 UTC (permalink / raw) To: Jeff Law, 'gcc-patches@gcc.gnu.org'; +Cc: Tsimbalist, Igor V > -----Original Message----- > From: Jeff Law [mailto:law@redhat.com] > Sent: Friday, August 25, 2017 10:50 PM > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; 'gcc- > patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org> > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On 08/01/2017 02:56 AM, Tsimbalist, Igor V 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). > > > > > > 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling.patch > > > > > > From 403fc8239fb1f690cc378287b4def57dcc9d25bf Mon Sep 17 00:00:00 > 2001 > > From: Igor Tsimbalist <igor.v.tsimbalist@intel.com> > > Date: Mon, 3 Jul 2017 17:11:58 +0300 > > Subject: [PATCH 1/9] 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). > > > > gcc/c-family/ > > > > * c-attribs.c (handle_notrack_attribute): New function. > > (c_common_attribute_table): Add 'notrack' handling. > > > > gcc/ > > > > * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOTRACK. > > * combine.c (distribute_notes): Add REG_CALL_NOTRACK handling. > > * common.opt: Add finstrument-control-flow flag. > > * emit-rtl.c (try_split): Add REG_CALL_NOTRACK handling. > > * gimple.c (gimple_build_call_from_tree): Add 'notrack' > > propogation. > > * gimple.h (gf_mask): Add GF_CALL_WITH_NOTRACK. > > (gimple_call_with_notrack_p): function. > > (gimple_call_set_with_notrack): Likewise. > > * recog.c (peep2_attempt): Add REG_CALL_NOTRACK handling. > > * reg-notes.def: Add REG_NOTE (CALL_NOTRACK). > > * toplev.c (process_options): Add flag_instrument_control_flow > > handling. > > --- > > gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ > > gcc/cfgexpand.c | 11 +++++++++++ > > gcc/combine.c | 1 + > > gcc/common.opt | 5 +++++ > > gcc/emit-rtl.c | 1 + > > gcc/gimple.c | 17 +++++++++++++++++ > > gcc/gimple.h | 34 ++++++++++++++++++++++++++++++++++ > > gcc/recog.c | 1 + > > gcc/reg-notes.def | 7 +++++++ > > gcc/toplev.c | 11 +++++++++++ > > 10 files changed, 111 insertions(+) > > --- > > gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++ > > gcc/cfgexpand.c | 11 +++++++++++ > > gcc/combine.c | 1 + > > gcc/common.opt | 5 +++++ > > gcc/emit-rtl.c | 1 + > > gcc/gimple.c | 17 +++++++++++++++++ > > gcc/gimple.h | 34 ++++++++++++++++++++++++++++++++++ > > gcc/recog.c | 1 + > > gcc/reg-notes.def | 7 +++++++ > > gcc/toplev.c | 11 +++++++++++ > > 10 files changed, 111 insertions(+) > > > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index > > 0d9ab2d..4d62f7c 100644 > > --- a/gcc/c-family/c-attribs.c > > +++ b/gcc/c-family/c-attribs.c > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index c9d8118..ed344c5 > > 100644 > > --- a/gcc/cfgexpand.c > > +++ b/gcc/cfgexpand.c > > @@ -2657,12 +2657,23 @@ expand_call_stmt (gcall *stmt) > > } > > } > > > > + rtx_insn *before_call = get_last_insn (); > > lhs = gimple_call_lhs (stmt); > > if (lhs) > > expand_assignment (lhs, exp, false); > > else > > expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); > > > > + /* Find a generated CALL insn to propagate a 'notrack' attribute. > > +*/ > > + rtx_insn *last = get_last_insn (); > > + while (!CALL_P (last) > > + && (last != before_call)) > > + last = PREV_INSN (last); > > + > > + if (last != before_call > > + && gimple_call_with_notrack_p (stmt)) > > + add_reg_note (last, REG_CALL_NOTRACK, const0_rtx); > > + > > mark_transaction_restart_calls (stmt); } > Just to be sure -- this is only searching through the currently open sequence > to find the RTL call emitted into the sequence to implement the gimple call > we're expanding, right? > > ISTM that you don't need to do the search if !gimple_call_with_notrack_p, > right? Which would imply this code should look something like > > if (gimple_call_with_notrack_p (stmt)) > { > rtx insn *last = get_last_insn (); > while (!CALL_P (last) > && last != before_call) > last = PREV_INSN (last); > add_reg_note (last, REG_CALL_NOTRACK, const0_rtx) > } > > Or something close to that? > > > I'd be slightly concerned if expansion of an argument resulted in a call. I'm > not sure if that can happen anymore, but if it does, then you could end up > marking the wrong CALL_INSN. > > THe only cases where I think that might be possible would be if we needed > to emit a libcall or memcpy. So perhaps you want to verify that last is a call > *and* that it's an indirect call. I've rewrote the code as you suggested to check if gimple call is an indirect call and is 'noverify_cf' call. > > > 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); > > + } > > + } > > + > > return call; > As Richi noted, this deserves a better comment. > > It may even be advisable to build a predicate to test if a given gimple > statement is an indirect call and use that rather than fndecl == NULL_TREE. > That would help make this code clearer as well. I've rewrote the code above according to Richard's suggestion: second parameter was introduced and a proper type is passed to the function to look if it has the required attribute. > Q. Do we need to do anything with ICF (identical code folding) and CFE? > Given two functions which have the same implementation in gimple, except > that one has a notrack indirect call and the other has a tracked indirect call, > what is proper behavior? I think we'd keep them separate which implies we > need to make sure the notrack attribute is part of the ICF hashing > implementation. It'd probably even be worth building a test for this :-) Are you talking about a case when such two functions are inlined? Or there is a possibility to merge function bodies if they are identical? I agree with you that the functions should be kept separate. I haven't looked into such optimization in gcc so I need to learn it. > > } > > > > > > +/* Return true if call GS is marked as no-track. */ > > + > > +static inline bool > > +gimple_call_with_notrack_p (const gcall *gs) { > > + return (gs->subcode & GF_CALL_WITH_NOTRACK) != 0; } > > + > > +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); } > Agree with Richi WRT avoiding gimple * overloads. Fixed. Thanks, Igor > > > Jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-12 15:59 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-13 18:56 ` Jeff Law 0 siblings, 0 replies; 24+ messages in thread From: Jeff Law @ 2017-09-13 18:56 UTC (permalink / raw) To: Tsimbalist, Igor V, 'gcc-patches@gcc.gnu.org' On 09/12/2017 09:59 AM, Tsimbalist, Igor V wrote: > >> Q. Do we need to do anything with ICF (identical code folding) and CFE? >> Given two functions which have the same implementation in gimple, except >> that one has a notrack indirect call and the other has a tracked indirect call, >> what is proper behavior? I think we'd keep them separate which implies we >> need to make sure the notrack attribute is part of the ICF hashing >> implementation. It'd probably even be worth building a test for this :-) > Are you talking about a case when such two functions are inlined? Or there is a possibility to merge > function bodies if they are identical? The latter. The compiler has a couple strategies when it finds identical bodies. I'm over-simplifying, but given two functions, A and B. If ICF hashing finds they are identical, then only one function definition would be emitted. So given two functions A & B. They are identical except that A has an indirect call and the signature of the call target has the notrack attribute while B has an indirect call and the signature of the call target does not have the notrack attribute. A & B would be subject to ICF, but I don't think that's the right/safe thing to do. Or am I missing something? jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-08-25 21:05 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-09-12 15:59 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-13 17:08 ` Tsimbalist, Igor V 2017-09-13 19:01 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 1 sibling, 1 reply; 24+ messages in thread From: Tsimbalist, Igor V @ 2017-09-13 17:08 UTC (permalink / raw) To: Jeff Law, 'gcc-patches@gcc.gnu.org'; +Cc: Tsimbalist, Igor V > -----Original Message----- > From: Tsimbalist, Igor V > Sent: Tuesday, September 12, 2017 5:59 PM > To: 'Jeff Law' <law@redhat.com>; 'gcc-patches@gcc.gnu.org' <gcc- > patches@gcc.gnu.org> > Cc: 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, August 25, 2017 10:50 PM > > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; 'gcc- > > patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org> > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote: > > > Part#1. Add generic part for Intel CET enabling. > > > > > > Q. Do we need to do anything with ICF (identical code folding) and CFE? > > Given two functions which have the same implementation in gimple, > > except that one has a notrack indirect call and the other has a > > tracked indirect call, what is proper behavior? I think we'd keep > > them separate which implies we need to make sure the notrack attribute > > is part of the ICF hashing implementation. It'd probably even be > > worth building a test for this :-) > Are you talking about a case when such two functions are inlined? Or there is > a possibility to merge function bodies if they are identical? > > I agree with you that the functions should be kept separate. I haven't looked > into such optimization in gcc so I need to learn it. I thought over this case and my conclusion is that nothing has to be done regarding ICF. First of all let's sync on a case we are talking about. A code template could look like fn1 definition { <some code> } fn2 definition with notrack attribute { <the same code as in fn1> } func definition { <calling fn1>...<calling fn2> } Is it the case you are talking about? Let's consider different scenarios: 1) calls to fn1 and fn2 are direct calls. In that case 'notrack' has no effect on direct calls as they are assumed to be save (it applies to indirect calls only). ICF can be done here; 2) one of calls is an indirect call or both calls are indirect calls. If compiler can prove what exact functions are called then indirect call(s) can be replaced by direct call(s) and that gives us the case 1); 3) if compiler cannot prove what function is called it will keep the indirect call and so there is nothing to do for ICF here. Thanks, Igor > > > > } > > > > > > > > > +/* Return true if call GS is marked as no-track. */ > > > + > > > +static inline bool > > > +gimple_call_with_notrack_p (const gcall *gs) { > > > + return (gs->subcode & GF_CALL_WITH_NOTRACK) != 0; } > > > + > > > +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); } > > Agree with Richi WRT avoiding gimple * overloads. > Fixed. > > Thanks, > Igor > > > > > > > Jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling 2017-09-13 17:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V @ 2017-09-13 19:01 ` Jeff Law 0 siblings, 0 replies; 24+ messages in thread From: Jeff Law @ 2017-09-13 19:01 UTC (permalink / raw) To: Tsimbalist, Igor V, 'gcc-patches@gcc.gnu.org' On 09/13/2017 11:07 AM, Tsimbalist, Igor V wrote: >> -----Original Message----- >> From: Tsimbalist, Igor V >> Sent: Tuesday, September 12, 2017 5:59 PM >> To: 'Jeff Law' <law@redhat.com>; 'gcc-patches@gcc.gnu.org' <gcc- >> patches@gcc.gnu.org> >> Cc: 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, August 25, 2017 10:50 PM >>> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; 'gcc- >>> patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org> >>> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling >>> >>> On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote: >>>> Part#1. Add generic part for Intel CET enabling. >>>> >> >>> Q. Do we need to do anything with ICF (identical code folding) and CFE? >>> Given two functions which have the same implementation in gimple, >>> except that one has a notrack indirect call and the other has a >>> tracked indirect call, what is proper behavior? I think we'd keep >>> them separate which implies we need to make sure the notrack attribute >>> is part of the ICF hashing implementation. It'd probably even be >>> worth building a test for this :-) >> Are you talking about a case when such two functions are inlined? Or there is >> a possibility to merge function bodies if they are identical? >> >> I agree with you that the functions should be kept separate. I haven't looked >> into such optimization in gcc so I need to learn it. > I thought over this case and my conclusion is that nothing has to be done regarding ICF. > > First of all let's sync on a case we are talking about. A code template could look like > > fn1 definition > { > <some code> > } > > fn2 definition with notrack attribute > { > <the same code as in fn1> > } > > func definition > { > <calling fn1>...<calling fn2> > } > > Is it the case you are talking about? Let's consider different scenarios: > > 1) calls to fn1 and fn2 are direct calls. In that case 'notrack' has no effect on direct calls as they are > assumed to be save (it applies to indirect calls only). ICF can be done here; > 2) one of calls is an indirect call or both calls are indirect calls. If compiler can prove what exact functions > are called then indirect call(s) can be replaced by direct call(s) and that gives us the case 1); > 3) if compiler cannot prove what function is called it will keep the indirect call and so there is nothing > to do for ICF here. No, not the case I'm worried about. Instead fn1() { indirect call where the signature is marked with notrack } fn2() { indirect call where the signature is not marked with notrack } fn1 and fn2 would be subject to ICF which I think is wrong. Essentially we're carrying semantic information in attributes that are part of the type of the function pointer. I think we need to include those attributes when we hash and compare two objects for equality within ICF. Jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-10-12 15:10 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-01 8:56 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-08-15 14:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2017-08-18 14:01 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-08-18 14:06 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2017-08-18 14:58 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-12 15:34 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-15 11:12 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-15 12:14 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Richard Biener 2017-09-19 13:39 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-28 22:44 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-09-29 14:31 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-29 16:04 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-10-05 10:20 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-10-12 6:26 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-10-12 8:33 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-10-12 15:15 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-08-25 21:03 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-09-12 15:40 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-13 19:05 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-08-25 21:05 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-09-12 15:59 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-13 18:56 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law 2017-09-13 17:08 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Tsimbalist, Igor V 2017-09-13 19:01 ` 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).