| Message ID | 20240807000652.1417776-19-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv support for control flow integrity extensions | expand |
On 8/7/24 10:06, Deepak Gupta wrote: > Violations to control flow rules setup by zicfilp and zicfiss lead to > software check exceptions. To debug and fix such sw check issues in guest > , add trace-hooks for each case. > > Signed-off-by: Jim Shu <jim.shu@sifive.com> > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > --- > target/riscv/insn_trans/trans_rvi.c.inc | 6 ++++-- > target/riscv/op_helper.c | 24 ++++++++++++++++++++++++ > target/riscv/trace-events | 6 ++++++ > target/riscv/translate.c | 2 +- > 4 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc > index cbd7d5c395..0f5d5def60 100644 > --- a/target/riscv/insn_trans/trans_rvi.c.inc > +++ b/target/riscv/insn_trans/trans_rvi.c.inc > @@ -65,7 +65,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a) > */ > gen_helper_raise_sw_check_excep(tcg_env, > tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), > - tcg_constant_tl(MISALIGNED_LPAD), tcg_constant_tl(0)); > + tcg_constant_tl(MISALIGNED_LPAD), > + tcg_constant_tl(ctx->base.pc_next)); > return true; > } > } > @@ -81,7 +82,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a) > tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip); > gen_helper_raise_sw_check_excep(tcg_env, > tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), > - tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0)); > + tcg_constant_tl(LABEL_MISMATCH_LPAD), > + tcg_constant_tl(a->imm_cfi20)); > gen_set_label(skip); > } > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index 3b47fb34ea..07990e6589 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -24,6 +24,7 @@ > #include "exec/exec-all.h" > #include "exec/cpu_ldst.h" > #include "exec/helper-proto.h" > +#include "trace.h" > > /* Exceptions processing helpers */ > G_NORETURN void riscv_raise_exception(CPURISCVState *env, > @@ -262,6 +263,29 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address) > void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong swcheck_code, > target_ulong arg1, target_ulong arg2) > { > + switch (swcheck_code) { > + case RISCV_EXCP_SW_CHECK_FCFI_TVAL: > + switch (arg1) { > + case MISSING_LPAD: > + trace_zicfilp_missing_lpad_instr(arg2); > + break; > + case MISALIGNED_LPAD: > + trace_zicfilp_unaligned_lpad_instr(arg2); > + break; > + case LABEL_MISMATCH_LPAD: > + trace_zicfilp_lpad_reg_mismatch(arg2); > + break; > + } > + break; > + case RISCV_EXCP_SW_CHECK_BCFI_TVAL: > + trace_zicfiss_sspopchk_reg_mismatch(arg1, arg2); > + break; > + default: > + /* any other value of swcheck_code is asserted */ > + assert(swcheck_code || (swcheck_code == 0)); > + break; > + } I think this is a mistake. Better to have 4 different helpers and eliminate MISSING_LPAD, etc entirely. > @@ -1302,7 +1302,7 @@ static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu) > tcg_gen_brcondi_i32(TCG_COND_EQ, immediate, 0, l); > gen_helper_raise_sw_check_excep(tcg_env, > tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), > - tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0)); > + tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(ctx->base.pc_first)); You certainly don't need to pass pc_first, since if lp_expected is set that is always env->pc in the helper. r~
On Wed, Aug 07, 2024 at 01:27:22PM +1000, Richard Henderson wrote: >On 8/7/24 10:06, Deepak Gupta wrote: >>Violations to control flow rules setup by zicfilp and zicfiss lead to >>software check exceptions. To debug and fix such sw check issues in guest >>, add trace-hooks for each case. >> >>Signed-off-by: Jim Shu <jim.shu@sifive.com> >>Signed-off-by: Deepak Gupta <debug@rivosinc.com> >>--- >> target/riscv/insn_trans/trans_rvi.c.inc | 6 ++++-- >> target/riscv/op_helper.c | 24 ++++++++++++++++++++++++ >> target/riscv/trace-events | 6 ++++++ >> target/riscv/translate.c | 2 +- >> 4 files changed, 35 insertions(+), 3 deletions(-) >> >>diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc >>index cbd7d5c395..0f5d5def60 100644 >>--- a/target/riscv/insn_trans/trans_rvi.c.inc >>+++ b/target/riscv/insn_trans/trans_rvi.c.inc >>@@ -65,7 +65,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a) >> */ >> gen_helper_raise_sw_check_excep(tcg_env, >> tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), >>- tcg_constant_tl(MISALIGNED_LPAD), tcg_constant_tl(0)); >>+ tcg_constant_tl(MISALIGNED_LPAD), >>+ tcg_constant_tl(ctx->base.pc_next)); >> return true; >> } >> } >>@@ -81,7 +82,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a) >> tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip); >> gen_helper_raise_sw_check_excep(tcg_env, >> tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), >>- tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0)); >>+ tcg_constant_tl(LABEL_MISMATCH_LPAD), >>+ tcg_constant_tl(a->imm_cfi20)); >> gen_set_label(skip); >> } >>diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c >>index 3b47fb34ea..07990e6589 100644 >>--- a/target/riscv/op_helper.c >>+++ b/target/riscv/op_helper.c >>@@ -24,6 +24,7 @@ >> #include "exec/exec-all.h" >> #include "exec/cpu_ldst.h" >> #include "exec/helper-proto.h" >>+#include "trace.h" >> /* Exceptions processing helpers */ >> G_NORETURN void riscv_raise_exception(CPURISCVState *env, >>@@ -262,6 +263,29 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address) >> void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong swcheck_code, >> target_ulong arg1, target_ulong arg2) >> { >>+ switch (swcheck_code) { >>+ case RISCV_EXCP_SW_CHECK_FCFI_TVAL: >>+ switch (arg1) { >>+ case MISSING_LPAD: >>+ trace_zicfilp_missing_lpad_instr(arg2); >>+ break; >>+ case MISALIGNED_LPAD: >>+ trace_zicfilp_unaligned_lpad_instr(arg2); >>+ break; >>+ case LABEL_MISMATCH_LPAD: >>+ trace_zicfilp_lpad_reg_mismatch(arg2); >>+ break; >>+ } >>+ break; >>+ case RISCV_EXCP_SW_CHECK_BCFI_TVAL: >>+ trace_zicfiss_sspopchk_reg_mismatch(arg1, arg2); >>+ break; >>+ default: >>+ /* any other value of swcheck_code is asserted */ >>+ assert(swcheck_code || (swcheck_code == 0)); >>+ break; >>+ } > >I think this is a mistake. >Better to have 4 different helpers and eliminate MISSING_LPAD, etc entirely. Honestly for raising sw check exception, helper is not needed. I added a helper for raising sw check exception, because needed to add trace capability on violations. Helps locate issues in cfi compiled binaries faster. If you prefer 4 different helpers for 4 different trace needs, I will do that. > >>@@ -1302,7 +1302,7 @@ static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu) >> tcg_gen_brcondi_i32(TCG_COND_EQ, immediate, 0, l); >> gen_helper_raise_sw_check_excep(tcg_env, >> tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), >>- tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0)); >>+ tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(ctx->base.pc_first)); > >You certainly don't need to pass pc_first, since if lp_expected is set >that is always env->pc in the helper. noted. > > >r~
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc index cbd7d5c395..0f5d5def60 100644 --- a/target/riscv/insn_trans/trans_rvi.c.inc +++ b/target/riscv/insn_trans/trans_rvi.c.inc @@ -65,7 +65,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a) */ gen_helper_raise_sw_check_excep(tcg_env, tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), - tcg_constant_tl(MISALIGNED_LPAD), tcg_constant_tl(0)); + tcg_constant_tl(MISALIGNED_LPAD), + tcg_constant_tl(ctx->base.pc_next)); return true; } } @@ -81,7 +82,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a) tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip); gen_helper_raise_sw_check_excep(tcg_env, tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), - tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0)); + tcg_constant_tl(LABEL_MISMATCH_LPAD), + tcg_constant_tl(a->imm_cfi20)); gen_set_label(skip); } diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index 3b47fb34ea..07990e6589 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -24,6 +24,7 @@ #include "exec/exec-all.h" #include "exec/cpu_ldst.h" #include "exec/helper-proto.h" +#include "trace.h" /* Exceptions processing helpers */ G_NORETURN void riscv_raise_exception(CPURISCVState *env, @@ -262,6 +263,29 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address) void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong swcheck_code, target_ulong arg1, target_ulong arg2) { + switch (swcheck_code) { + case RISCV_EXCP_SW_CHECK_FCFI_TVAL: + switch (arg1) { + case MISSING_LPAD: + trace_zicfilp_missing_lpad_instr(arg2); + break; + case MISALIGNED_LPAD: + trace_zicfilp_unaligned_lpad_instr(arg2); + break; + case LABEL_MISMATCH_LPAD: + trace_zicfilp_lpad_reg_mismatch(arg2); + break; + } + break; + case RISCV_EXCP_SW_CHECK_BCFI_TVAL: + trace_zicfiss_sspopchk_reg_mismatch(arg1, arg2); + break; + default: + /* any other value of swcheck_code is asserted */ + assert(swcheck_code || (swcheck_code == 0)); + break; + } + env->sw_check_code = swcheck_code; riscv_raise_exception(env, RISCV_EXCP_SW_CHECK, GETPC()); } diff --git a/target/riscv/trace-events b/target/riscv/trace-events index 49ec4d3b7d..0e8807f0d4 100644 --- a/target/riscv/trace-events +++ b/target/riscv/trace-events @@ -9,3 +9,9 @@ pmpaddr_csr_write(uint64_t mhartid, uint32_t addr_index, uint64_t val) "hart %" mseccfg_csr_read(uint64_t mhartid, uint64_t val) "hart %" PRIu64 ": read mseccfg, val: 0x%" PRIx64 mseccfg_csr_write(uint64_t mhartid, uint64_t val) "hart %" PRIu64 ": write mseccfg, val: 0x%" PRIx64 + +# zicfiss/lp +zicfiss_sspopchk_reg_mismatch(uint64_t ssra, uint64_t rs1) "shadow_stack_ra: 0x%" PRIx64 ", rs1: 0x%" PRIx64 +zicfilp_missing_lpad_instr(uint64_t pc_first) "pc_first: 0x%" PRIx64 +zicfilp_unaligned_lpad_instr(uint64_t pc_next) "pc_next: 0x%" PRIx64 +zicfilp_lpad_reg_mismatch(int lpad_label) "lpad_label: 0x%x" diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 4772191bd8..9ef1f220e0 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -1302,7 +1302,7 @@ static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu) tcg_gen_brcondi_i32(TCG_COND_EQ, immediate, 0, l); gen_helper_raise_sw_check_excep(tcg_env, tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL), - tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0)); + tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(ctx->base.pc_first)); gen_set_label(l); /* * Despite the use of gen_exception_illegal(), the rest of