qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 RFC Zisslpcfi 7/9] target/riscv: Tracking indirect branche

From: LIU Zhiwei
Subject: Re: [PATCH v1 RFC Zisslpcfi 7/9] target/riscv: Tracking indirect branches (fcfi) using TCG
Date: Thu, 16 Feb 2023 10:43:54 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 

On 2023/2/16 8:02, Deepak Gupta wrote:

On Wed, Feb 15, 2023 at 12:55 AM LIU Zhiwei
<zhiwei_liu@linux.alibaba.com> wrote:


On 2023/2/9 14:24, Deepak Gupta wrote:

zisslpcfi protects forward control flow (if enabled) by enforcing all
indirect call and jmp must land on a landing pad instruction `lpcll`
short for landing pad and check lower label value. If target of an
indirect call or jmp is not `lpcll` then cpu/hart must raise an illegal
instruction exception.

This patch implements the mechanism using TCG. Target architecture branch
instruction must define the end of a TB. Using this property, during
translation of branch instruction, TB flag = FCFI_LP_EXPECTED can be set.
Translation of target TB can check if FCFI_LP_EXPECTED flag is set and a
flag (fcfi_lp_expected) can be set in DisasContext. If `lpcll` gets
translated, fcfi_lp_expected flag in DisasContext can be cleared. Else
it'll fault.

This patch also also adds flag for forward and backward cfi in
DisasContext.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
Signed-off-by: Kip Walker  <kip@rivosinc.com>
---
   target/riscv/cpu.h        |  3 +++
   target/riscv/cpu_helper.c | 12 +++++++++
   target/riscv/translate.c  | 52 +++++++++++++++++++++++++++++++++++++++
   3 files changed, 67 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 8803ea6426..98b272bcad 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -644,6 +644,9 @@ FIELD(TB_FLAGS, VMA, 25, 1)
   /* Native debug itrigger */
   FIELD(TB_FLAGS, ITRIGGER, 26, 1)

+/* Zisslpcfi needs a TB flag to track indirect branches */
+FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 27, 1)
+
   #ifdef TARGET_RISCV32
   #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
   #else
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 63377abc2f..d15918f534 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -129,6 +129,18 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong 
*pc,
           flags = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
       }

+    if (cpu->cfg.ext_cfi) {
+        /*
+         * For Forward CFI, only the expectation of a lpcll at
+         * the start of the block is tracked (which can only happen
+         * when FCFI is enabled for the current processor mode). A jump
+         * or call at the end of the previous TB will have updated
+         * env->elp to indicate the expectation.
+         */
+        flags = FIELD_DP32(flags, TB_FLAGS, FCFI_LP_EXPECTED,
+                           env->elp != NO_LP_EXPECTED);
+    }
+
   #ifdef CONFIG_USER_ONLY
       flags |= TB_FLAGS_MSTATUS_FS;
       flags |= TB_FLAGS_MSTATUS_VS;
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index df38db7553..7d43d20fc3 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -41,6 +41,7 @@ static TCGv load_val;
   /* globals for PM CSRs */
   static TCGv pm_mask;
   static TCGv pm_base;
+static TCGOp *cfi_lp_check;

   #include "exec/gen-icount.h"

@@ -116,6 +117,10 @@ typedef struct DisasContext {
       bool itrigger;
       /* TCG of the current insn_start */
       TCGOp *insn_start;
+    /* CFI extension */
+    bool bcfi_enabled;
+    bool fcfi_enabled;
+    bool fcfi_lp_expected;
   } DisasContext;

   static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -1166,11 +1171,44 @@ static void 
riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
       ctx->pm_mask_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_MASK_ENABLED);
       ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
       ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
+    ctx->bcfi_enabled = cpu_get_bcfien(env);
+    ctx->fcfi_enabled = cpu_get_fcfien(env);

This is wrong.  If you ctx->bcfi_enabled in the translation and don't
put it in a tb flags field, the translated tb will
be misused.

TLB for shadow stack index is flushed on privilege transfers.
All TLBs is flushed whenever enable/disable bits for shadow stack are toggled.
Can you elaborate a bit more how this can be misused?
As Richard has pointed out, you need put this fields into tb flags if you want to use them in translation process as constants. 
Nothing to do with TLB.
Tb flags will always be calculated (Except the direct block chain I think, is it right? @Richard) before using the translated tb. So if your translated tb depend on some machine states, you should put it in tb flags. Otherwise, the translated tb will be misused, as its execution environment varies from its translation machine states. 

Zhiwei





Zhiwei


+    ctx->fcfi_lp_expected = FIELD_EX32(tb_flags, TB_FLAGS, FCFI_LP_EXPECTED);
       ctx->zero = tcg_constant_tl(0);
   }

   static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
   {
+    DisasContext *ctx = container_of(db, DisasContext, base);
+
+    if (ctx->fcfi_lp_expected) {
+        /*
+         * Since we can't look ahead to confirm that the first
+         * instruction is a legal landing pad instruction, emit
+         * compare-and-branch sequence that will be fixed-up in
+         * riscv_tr_tb_stop() to either statically hit or skip an
+         * illegal instruction exception depending on whether the
+         * flag was lowered by translation of a CJLP or JLP as
+         * the first instruction in the block.
+         */
+        TCGv_i32 immediate;
+        TCGLabel *l;
+        l = gen_new_label();
+        immediate = tcg_temp_local_new_i32();
+        tcg_gen_movi_i32(immediate, 0);
+        cfi_lp_check = tcg_last_op();
+        tcg_gen_brcondi_i32(TCG_COND_EQ, immediate, 0, l);
+        tcg_temp_free_i32(immediate);
+        gen_exception_illegal(ctx);
+        gen_set_label(l);
+        /*
+         * Despite the use of gen_exception_illegal(), the rest of
+         * the TB needs to be generated. The TCG optimizer will
+         * clean things up depending on which path ends up being
+         * active.
+         */
+        ctx->base.is_jmp = DISAS_NEXT;
+    }
   }

   static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
@@ -1225,6 +1263,7 @@ static void riscv_tr_translate_insn(DisasContextBase 
*dcbase, CPUState *cpu)
   static void riscv_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
   {
       DisasContext *ctx = container_of(dcbase, DisasContext, base);
+    CPURISCVState *env = cpu->env_ptr;

       switch (ctx->base.is_jmp) {
       case DISAS_TOO_MANY:
@@ -1235,6 +1274,19 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, 
CPUState *cpu)
       default:
           g_assert_not_reached();
       }
+
+    if (ctx->fcfi_lp_expected) {
+        /*
+         * If the "lp expected" flag is still up, the block needs to take an
+         * illegal instruction exception.
+         */
+        tcg_set_insn_param(cfi_lp_check, 1, tcgv_i32_arg(tcg_constant_i32(1)));
+    } else {
+        /*
+        * LP instruction requirement was met, clear up LP expected
+        */
+        env->elp = NO_LP_EXPECTED;
+    }
   }

   static void riscv_tr_disas_log(const DisasContextBase *dcbase,
reply via email to
[Prev in Thread] Current Thread [Next in Thread]