Richard, Thanks a lot for your comments.
> On Oct 20, 2020, at 1:12 PM, Richard Sandiford <richard.sandif...@arm.com> > wrote: > >> >> + >> + if ((strcmp (TREE_STRING_POINTER (id), "skip") != 0) >> + && (strcmp (TREE_STRING_POINTER (id), "used-gpr-arg") != 0) >> + && (strcmp (TREE_STRING_POINTER (id), "used-arg") != 0) >> + && (strcmp (TREE_STRING_POINTER (id), "all-arg") != 0) >> + && (strcmp (TREE_STRING_POINTER (id), "used-gpr") != 0) >> + && (strcmp (TREE_STRING_POINTER (id), "all-gpr") != 0) >> + && (strcmp (TREE_STRING_POINTER (id), "used") != 0) >> + && (strcmp (TREE_STRING_POINTER (id), "all") != 0)) > > Any reason we don't support all-gpr-arg? Seems to be the only > “missing” combination. Will add this one. > > Would be good to have a single piece of code that parses these > arguments into a set of flags, rather than have one list here > and one get_call_used_regs_seq. > > Maybe we could do something similar to sanitizer_opts, but that > might not be necessary. Okay, will do that. > >> + { >> + error ("attribute %qE argument must be one of %qs, %qs, %qs, %qs," >> + "%qs, %qs, %qs, or %qs", >> + name, "skip", "used-gpr-arg", "used-arg", "all-arg", >> + "used-gpr", "all-gpr", "used", "all"); >> + *no_add_attris = true; >> + return NULL_TREE; >> + } >> + >> + return NULL_TREE; >> +} >> + >> /* Handle a "returns_nonnull" attribute; arguments as in >> struct attribute_spec.handler. */ >> >> diff --git a/gcc/coretypes.h b/gcc/coretypes.h >> index 6b6cfcd..0ce5eb4 100644 >> --- a/gcc/coretypes.h >> +++ b/gcc/coretypes.h >> @@ -418,6 +418,19 @@ enum symbol_visibility >> VISIBILITY_INTERNAL >> }; >> >> +/* Zero call-used registers type. */ >> +enum zero_call_used_regs { >> + zero_call_used_regs_unset = 0, >> + zero_call_used_regs_skip, >> + zero_call_used_regs_used_gpr_arg, >> + zero_call_used_regs_used_arg, >> + zero_call_used_regs_all_arg, >> + zero_call_used_regs_used_gpr, >> + zero_call_used_regs_all_gpr, >> + zero_call_used_regs_used, >> + zero_call_used_regs_all >> +}; > > I think a bitmask would be easier to use: > > SKIP > ONLY_USED > ONLY_GPR > ONLY_ARG > > Should probably be a class enum given that we're C++11. Good suggestion. > >> +pass parameters. @samp{used-arg} zeros used call-used registers that >> +pass parameters. @samp{arg} zeros all call-used registers that pass >> +parameters. These 3 choices are used for ROP mitigation. >> + >> +@samp{used-gpr} zeros call-used general purpose registers >> +which are used in function. @samp{all-gpr} zeros all >> +call-used registers. @samp{used} zeros call-used registers which >> +are used in function. @samp{all} zeros all call-used registers. >> +These 4 choices are used for preventing information leak through >> +registers. > > The description for all-gpr doesn't look right. Oops. Will fix it. > I think it would > be easier to describe (and hopefully to follow) if we start with > the three basic choices: “skip”, “used” and “all”. Then describe > how “used” and “all” can be modified by adding “-gpr” to limit the > clearing to general-purpose registers and “-arg” to limit the > clearing to argument registers. > > We need to say what “call-used” and “used” mean in this context. > In particular, “call-used” is also known as “call-clobbered”, > “caller-saved“ and “volatile”, so it would be good to list those > as alternatives. We need to say what “used” registers are. Okay. >> >> +@item -fzero-call-used-regs=@var{choice} >> +@opindex fzero-call-used-regs >> +Zero call-used registers at function return to increase the program >> +security by either mitigating Return-Oriented Programming (ROP) or >> +preventing information leak through registers. >> + >> +@samp{skip}, which is the default, doesn't zero call-used registers. >> + >> +@samp{used-gpr-arg} zeros used call-used general purpose registers that >> +pass parameters. @samp{used-arg} zeros used call-used registers that >> +pass parameters. @samp{all-arg} zeros all call-used registers that pass >> +parameters. These 3 choices are used for ROP mitigation. >> + >> +@samp{used-gpr} zeros call-used general purpose registers >> +which are used in function. @samp{all-gpr} zeros all >> +call-used registers. @samp{used} zeros call-used registers which >> +are used in function. @samp{all} zeros all call-used registers. >> +These 4 choices are used for preventing information leak through >> +registers. > > Same comment here. Okay. > >> @@ -310,6 +310,9 @@ struct GTY(()) rtl_data { >> sets them. */ >> HARD_REG_SET asm_clobbers; >> >> + /* All hard registers that are zeroed at the return of the routine. */ >> + HARD_REG_SET zeroed_reg_set; > > How about “must_be_zero_on_return“? “zeroed_reg_set” isn't very > specific about where the zeroing happens or is needed. E.g. we also > zero uninitialised registers. okay. > >> +{ >> + basic_block bb = BLOCK_FOR_INSN (ret); >> + auto_bitmap live_out; >> + bitmap_copy (live_out, df_get_live_out (bb)); > > Sorry, forgot that here we should do: > > df_simulate_initialize_backwards (bb, live_out); > > But we should calculate this set once per return instruction rather > than repeat the calculation for every register. Okay. > >> >> + /* If gpr_only is true, only zero call-used-registers that are >> + general-purpose registers; if used_only is true, only zero >> + call-used-registers that are used in the current function. */ >> + >> + switch (zero_regs_type) >> + { >> + case zero_call_used_regs_used_arg: >> + gpr_only = false; >> + break; >> + case zero_call_used_regs_all_arg: >> + gpr_only = false; >> + used_only = false; >> + break; >> + case zero_call_used_regs_used_gpr: >> + arg_only = false; >> + break; >> + case zero_call_used_regs_all_gpr: >> + used_only = false; >> + arg_only = false; >> + break; >> + case zero_call_used_regs_used: >> + gpr_only = false; >> + arg_only = false; >> + break; >> + case zero_call_used_regs_all: >> + gpr_only = false; >> + used_only = false; >> + arg_only = false; >> + break; >> + default: >> + break; >> + } > > Using a bitmask would also simplify this. agreed. > >> + >> + /* For each of the hard registers, check to see whether we should zero it >> if: >> + 1. it is a call-used-registers; >> + and 2. it is not a fixed-registers; >> + and 3. it is not live at the return of the routine; >> + and 4. it is general registor if gpr_only is true; >> + and 5. it is used in the routine if used_only is true; >> + and 6. it is a register that passes parameter if arg_only is true; >> + */ >> + >> + HARD_REG_SET need_zeroed_hardregs; >> + CLEAR_HARD_REG_SET (need_zeroed_hardregs); >> + for (unsigned int regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++) >> + { >> + if (!this_target_hard_regs->x_call_used_regs[regno]) >> + continue; > > This should use crtl->abi instead. The set of call-used registers > can vary from function to function. You mean to use: If (!crtl->abi->clobbers_full_reg_p(regno)) ? > >> +static unsigned int >> +rest_of_zero_call_used_regs (void) >> +{ >> + basic_block bb; >> + rtx_insn *insn; >> + >> + /* This pass needs data flow information. */ >> + df_analyze (); >> + >> + /* Search all the "return"s in the routine, and insert instruction >> sequence to >> + zero the call used registers. */ >> + FOR_EACH_BB_REVERSE_FN (bb, cfun) >> + if (bb == EXIT_BLOCK_PTR_FOR_FN (cfun) >> + || (single_succ_p (bb) >> + && single_succ (bb) == EXIT_BLOCK_PTR_FOR_FN (cfun))) >> + FOR_BB_INSNS_REVERSE (bb, insn) >> + if (JUMP_P (insn) && ANY_RETURN_P (JUMP_LABEL (insn))) >> + { >> + /* Now we can insert the instruction sequence to zero the call used >> + registers before this insn. */ >> + gen_call_used_regs_seq (insn); >> + break; >> + } > > The exit block never has instructions, so it's only necessary to process > predecessors. A simpler way to do that is to iterate over the edges in: > > EXIT_BLOCK_PTR_FOR_FN (cfun)->preds > > You shouldn't need to use FOR_BB_INSNS_REVERSE: it should be enough > to check only BB_END (bb), since returns always end blocks. Something like the following? FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds) { insn = BB_END (e->src); If (JUMP_P (insn) && ANY_RETURN_P (JUMP_LABEL (insn))) { /* Now we can insert the instruction sequence to zero the call used registers before this insn. */ gen_call_used_regs_seq (insn); break; } } > >> + >> + return 0; >> +} >> + >> +namespace { >> + >> +const pass_data pass_data_zero_call_used_regs = >> +{ >> + RTL_PASS, /* type */ >> + "zero_call_used_regs", /* name */ >> + OPTGROUP_NONE, /* optinfo_flags */ >> + TV_NONE, /* tv_id */ >> + 0, /* properties_required */ >> + 0, /* properties_provided */ >> + 0, /* properties_destroyed */ >> + 0, /* todo_flags_start */ >> + 0, /* todo_flags_finish */ >> +}; >> + >> +class pass_zero_call_used_regs: public rtl_opt_pass >> +{ >> +public: >> + pass_zero_call_used_regs (gcc::context *ctxt) >> + : rtl_opt_pass (pass_data_zero_call_used_regs, ctxt) >> + {} >> + >> + /* opt_pass methods: */ >> + virtual bool gate (function *) >> + { >> + return flag_zero_call_used_regs > zero_call_used_regs_unset; > > I think this also needs to check the function attributes. Okay. > >> + } >> + virtual unsigned int execute (function *) >> + { >> + return rest_of_zero_call_used_regs (); >> + } >> + >> +}; // class pass_zero_call_used_regs >> + >> +} // anon namespace >> + >> +rtl_opt_pass * >> +make_pass_zero_call_used_regs (gcc::context *ctxt) >> +{ >> + return new pass_zero_call_used_regs (ctxt); >> +} >> >> /* If CONSTRAINT is a matching constraint, then return its number. >> Otherwise, return -1. */ >> diff --git a/gcc/optabs.c b/gcc/optabs.c >> index 8ad7f4b..57e5c5d 100644 >> --- a/gcc/optabs.c >> +++ b/gcc/optabs.c >> @@ -6484,6 +6484,49 @@ expand_memory_blockage (void) >> expand_asm_memory_blockage (); >> } >> >> +/* Generate asm volatile("" : : : "memory") as a memory blockage, at the >> + same time clobbering the register set specified by ZEROED_REGS. */ >> + >> +void >> +expand_asm_reg_clobber_mem_blockage (HARD_REG_SET zeroed_regs) > > Just “regs”: the interface is more general than registers that are being > zeroed. Okay. > >> +{ >> + rtx asm_op, clob_mem, clob_reg; >> + >> + unsigned int num_of_regs = 0; >> + for (unsigned int i = 0; i < FIRST_PSEUDO_REGISTER; i++) >> + if (TEST_HARD_REG_BIT (zeroed_regs, i)) >> + num_of_regs++; >> + >> + if (num_of_regs == 0) >> + return; > > For this interface, I think we should continue and just include > a memory clobber. Right. > >> + RTVEC_ELT (v,j) = clob_reg; > > …IMO it would be more readable as just: > > RTVEC_ELT (v, j) = gen_rtx_CLOBBER (VOIDmode, regno_reg_rtx[i]); Okay. > >> diff --git a/gcc/recog.c b/gcc/recog.c >> index ce83b7f..472c2dc 100644 >> --- a/gcc/recog.c >> +++ b/gcc/recog.c >> @@ -923,6 +923,21 @@ validate_simplify_insn (rtx_insn *insn) >> return ((num_changes_pending () > 0) && (apply_change_group () > 0)); >> } >> >> >> + >> +bool >> +valid_insn_p (rtx_insn *insn) > > This should have a function comment. E.g.: > > /* Check whether INSN matches a specific alternative of an .md pattern. */ Okay. > >> >> +/* Generate instruction sequence to zero call used registers. */ >> +DEFHOOK >> +(zero_call_used_regs, >> + "This target hook emits instructions to zero registers specified\n\ >> +by @var{need_zeroed_hardregs} at function return, at the same time\n\ >> +return the hard register set that are actually zeroed by the hook\n\ >> +Define this hook if the target has more effecient instructions to\n\ >> +zero call-used registers, or if the target only tries to zero a subset\n\ >> +of @var{need_zeroed_hardregs}.\n\ >> +If the hook is not defined, the default_zero_call_used_reg will be used.", >> + HARD_REG_SET, (HARD_REG_SET need_zeroed_hardregs), > > I'd suggest: > > "Emit instructions to zero the subset of @var{selected_regs} that\n\ > could conceivably contain values that are useful to an attacker.\n\ > Return the set of registers that were actually cleared.\n\ > \n\ > The default implementation uses normal move instructions to zero\n\ > all the registers in @var{selected_regs}. Define this hook if the\n\ > target has more efficient ways of zeroing certain registers,\n\ > or if you believe that certain registers would never contain\n\ > values that are useful to an attacker." > > with the parameter called “selected_regs” instead of > “need_zeroed_hardregs”. (“need” suggests that the target > doesn't have the option of not zeroing.) Okay. > >> >> +/* The default hook for TARGET_ZERO_CALL_USED_REGS. */ >> + >> +HARD_REG_SET >> +default_zero_call_used_regs (HARD_REG_SET need_zeroed_hardregs) >> +{ >> + HARD_REG_SET zeroed_hardregs; >> + gcc_assert (!hard_reg_set_empty_p (need_zeroed_hardregs)); >> + >> + CLEAR_HARD_REG_SET (zeroed_hardregs); >> + for (unsigned int regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++) >> + if (TEST_HARD_REG_BIT (need_zeroed_hardregs, regno)) >> + { >> + rtx_insn *last_insn = get_last_insn (); >> + machine_mode mode = GET_MODE (regno_reg_rtx[regno]); >> + rtx zero = CONST0_RTX (mode); >> + rtx_insn *insn = emit_move_insn (regno_reg_rtx[regno], zero); >> + if (!valid_insn_p (insn)) >> + { >> + static bool issued_error; >> + if (!issued_error) >> + { >> + issued_error = true; >> + sorry ("-fzero-call-used-regs not supported on this target"); > > Should be "%qs not supported on this target", with the option name > as a second argument. Okay. > >> + } >> + delete_insns_since (last_insn); >> + } >> + else >> + SET_HARD_REG_BIT (zeroed_hardregs, regno); >> + } >> + return zeroed_hardregs; > > I don't think it's worth building up a different return set. The only > time it's different from need_zeroed_hardregs is if we emit the sorry, > which will cause compilation to fail anyway. Okay. > > Sorry, I ran out of time to review the tests, but the code part > otherwise looks good. thanks. Qing > > Thanks, > Richard