| Message ID | 20240807000652.1417776-5-debug@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv support for control flow integrity extensions | expand |
On 8/7/24 10:06, Deepak Gupta wrote: > elp state is recorded in *status on trap entry (less privilege to higher > privilege) and restored in elp from *status on trap exit (higher to less > privilege). > > Additionally this patch introduces a forward cfi helper function to > determine if current privilege has forward cfi is enabled or not based on > *envcfg (for U, VU, S, VU, HS) or mseccfg csr (for M). For qemu-user, a > new field `ufcfien` is introduced which is by default set to false and > helper function returns value deposited in `ufcfien` for qemu-user. > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > Co-developed-by: Jim Shu <jim.shu@sifive.com> > Co-developed-by: Andy Chiu <andy.chiu@sifive.com> > --- > target/riscv/cpu.c | 5 ++++ > target/riscv/cpu.h | 2 ++ > target/riscv/cpu_helper.c | 58 +++++++++++++++++++++++++++++++++++++++ > target/riscv/op_helper.c | 18 ++++++++++++ > 4 files changed, 83 insertions(+) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 82fa85a8d6..e1526c7ab5 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1022,6 +1022,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type) > env->load_res = -1; > set_default_nan_mode(1, &env->fp_status); > > +#ifdef CONFIG_USER_ONLY > + /* qemu-user for riscv, fcfi is off by default */ > + env->ufcfien = false; > +#endif > + > #ifndef CONFIG_USER_ONLY > if (cpu->cfg.debug) { > riscv_trigger_reset_hold(env); > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index ae436a3179..8c7841fc08 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -226,6 +226,7 @@ struct CPUArchState { > cfi_elp elp; > #ifdef CONFIG_USER_ONLY > uint32_t elf_flags; > + bool ufcfien; > #endif > > #ifndef CONFIG_USER_ONLY > @@ -530,6 +531,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen); > bool riscv_cpu_vector_enabled(CPURISCVState *env); > void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable); > int riscv_env_mmu_index(CPURISCVState *env, bool ifetch); > +bool cpu_get_fcfien(CPURISCVState *env); > G_NORETURN void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr, > MMUAccessType access_type, > int mmu_idx, uintptr_t retaddr); > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 6709622dd3..8c69c55576 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -33,6 +33,7 @@ > #include "cpu_bits.h" > #include "debug.h" > #include "tcg/oversized-guest.h" > +#include "pmp.h" > > int riscv_env_mmu_index(CPURISCVState *env, bool ifetch) > { > @@ -63,6 +64,35 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch) > #endif > } > > +bool cpu_get_fcfien(CPURISCVState *env) > +{ > +#ifdef CONFIG_USER_ONLY > + return env->ufcfien; > +#else > + /* no cfi extension, return false */ > + if (!env_archcpu(env)->cfg.ext_zicfilp) { > + return false; > + } Keep extension check common between user/system. Recall that you can choose -cpu from user as well. > + /* > + * Interrupt/exception/trap delivery is asynchronous event and as per > + * Zisslpcfi spec CPU should clear up the ELP state. If cfi extension is > + * available, clear ELP state. > + */ > + > + if (cpu->cfg.ext_zicfilp) { > + env->elp = NO_LP_EXPECTED; > + } If extension is not available, elp isn't a thing. You can just as easily make the store unconditional and save the test. > > + /* > + * If forward cfi enabled for new priv, restore elp status > + * and clear spelp in mstatus > + */ > + if (cpu_get_fcfien(env)) { > + env->elp = get_field(env->mstatus, MSTATUS_SPELP); > + env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0); > + } The spec is perhaps poorly written here. I read ... if xPP holds the value y, then ELP is set to the value of xPELP if yLPE is 1; otherwise, it is set to NO_LP_EXPECTED; xPELP is set to NO_LP_EXPECTED. as xPELP always being cleared, regardless of yLPE. r~
On Wed, Aug 07, 2024 at 11:06:49AM +1000, Richard Henderson wrote: >On 8/7/24 10:06, Deepak Gupta wrote: >>elp state is recorded in *status on trap entry (less privilege to higher >>privilege) and restored in elp from *status on trap exit (higher to less >>privilege). >> >>Additionally this patch introduces a forward cfi helper function to >>determine if current privilege has forward cfi is enabled or not based on >>*envcfg (for U, VU, S, VU, HS) or mseccfg csr (for M). For qemu-user, a >>new field `ufcfien` is introduced which is by default set to false and >>helper function returns value deposited in `ufcfien` for qemu-user. >> >>Signed-off-by: Deepak Gupta <debug@rivosinc.com> >>Co-developed-by: Jim Shu <jim.shu@sifive.com> >>Co-developed-by: Andy Chiu <andy.chiu@sifive.com> >>--- >> target/riscv/cpu.c | 5 ++++ >> target/riscv/cpu.h | 2 ++ >> target/riscv/cpu_helper.c | 58 +++++++++++++++++++++++++++++++++++++++ >> target/riscv/op_helper.c | 18 ++++++++++++ >> 4 files changed, 83 insertions(+) >> >>diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>index 82fa85a8d6..e1526c7ab5 100644 >>--- a/target/riscv/cpu.c >>+++ b/target/riscv/cpu.c >>@@ -1022,6 +1022,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type) >> env->load_res = -1; >> set_default_nan_mode(1, &env->fp_status); >>+#ifdef CONFIG_USER_ONLY >>+ /* qemu-user for riscv, fcfi is off by default */ >>+ env->ufcfien = false; >>+#endif >>+ >> #ifndef CONFIG_USER_ONLY >> if (cpu->cfg.debug) { >> riscv_trigger_reset_hold(env); >>diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>index ae436a3179..8c7841fc08 100644 >>--- a/target/riscv/cpu.h >>+++ b/target/riscv/cpu.h >>@@ -226,6 +226,7 @@ struct CPUArchState { >> cfi_elp elp; >> #ifdef CONFIG_USER_ONLY >> uint32_t elf_flags; >>+ bool ufcfien; >> #endif >> #ifndef CONFIG_USER_ONLY >>@@ -530,6 +531,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen); >> bool riscv_cpu_vector_enabled(CPURISCVState *env); >> void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable); >> int riscv_env_mmu_index(CPURISCVState *env, bool ifetch); >>+bool cpu_get_fcfien(CPURISCVState *env); >> G_NORETURN void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr, >> MMUAccessType access_type, >> int mmu_idx, uintptr_t retaddr); >>diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>index 6709622dd3..8c69c55576 100644 >>--- a/target/riscv/cpu_helper.c >>+++ b/target/riscv/cpu_helper.c >>@@ -33,6 +33,7 @@ >> #include "cpu_bits.h" >> #include "debug.h" >> #include "tcg/oversized-guest.h" >>+#include "pmp.h" >> int riscv_env_mmu_index(CPURISCVState *env, bool ifetch) >> { >>@@ -63,6 +64,35 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch) >> #endif >> } >>+bool cpu_get_fcfien(CPURISCVState *env) >>+{ >>+#ifdef CONFIG_USER_ONLY >>+ return env->ufcfien; >>+#else >>+ /* no cfi extension, return false */ >>+ if (!env_archcpu(env)->cfg.ext_zicfilp) { >>+ return false; >>+ } > >Keep extension check common between user/system. >Recall that you can choose -cpu from user as well. Ack. will put a check (for both extensions) Side note: ufcfien (or ubcfien) will get set only via syscall prctls which does check for extension. > >>+ /* >>+ * Interrupt/exception/trap delivery is asynchronous event and as per >>+ * Zisslpcfi spec CPU should clear up the ELP state. If cfi extension is >>+ * available, clear ELP state. >>+ */ >>+ >>+ if (cpu->cfg.ext_zicfilp) { >>+ env->elp = NO_LP_EXPECTED; >>+ } > >If extension is not available, elp isn't a thing. >You can just as easily make the store unconditional and save the test. Yes noted. make sense. > >> >>+ /* >>+ * If forward cfi enabled for new priv, restore elp status >>+ * and clear spelp in mstatus >>+ */ >>+ if (cpu_get_fcfien(env)) { >>+ env->elp = get_field(env->mstatus, MSTATUS_SPELP); >>+ env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0); >>+ } > >The spec is perhaps poorly written here. I read > > ... if xPP holds the value y, then ELP is set to the value of xPELP if yLPE is 1; > otherwise, it is set to NO_LP_EXPECTED; xPELP is set to NO_LP_EXPECTED. > >as xPELP always being cleared, regardless of yLPE. Yes that's what code above is also doing. restore elp status from SPELP field and clear it at SPELP. On `mret` same logic but for MPELP bitposition. > > >r~
On 8/8/24 06:11, Deepak Gupta wrote: >>> + /* >>> + * If forward cfi enabled for new priv, restore elp status >>> + * and clear spelp in mstatus >>> + */ >>> + if (cpu_get_fcfien(env)) { >>> + env->elp = get_field(env->mstatus, MSTATUS_SPELP); >>> + env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0); >>> + } >> >> The spec is perhaps poorly written here. I read >> >> ... if xPP holds the value y, then ELP is set to the value of xPELP if yLPE is 1; >> otherwise, it is set to NO_LP_EXPECTED; xPELP is set to NO_LP_EXPECTED. >> >> as xPELP always being cleared, regardless of yLPE. > > Yes that's what code above is also doing. restore elp status from SPELP field and clear > it at SPELP. No, my point is that the text doesn't seem to be if (enabled) { restore elp clear pelp } but if (enabled) { restore elp } clear pelp I.e. the clear is unconditional. r~
On Thu, Aug 08, 2024 at 08:40:08AM +1000, Richard Henderson wrote: >On 8/8/24 06:11, Deepak Gupta wrote: >>>>+ /* >>>>+ * If forward cfi enabled for new priv, restore elp status >>>>+ * and clear spelp in mstatus >>>>+ */ >>>>+ if (cpu_get_fcfien(env)) { >>>>+ env->elp = get_field(env->mstatus, MSTATUS_SPELP); >>>>+ env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0); >>>>+ } >>> >>>The spec is perhaps poorly written here. I read >>> >>> ... if xPP holds the value y, then ELP is set to the value of xPELP if yLPE is 1; >>> otherwise, it is set to NO_LP_EXPECTED; xPELP is set to NO_LP_EXPECTED. >>> >>>as xPELP always being cleared, regardless of yLPE. >> >>Yes that's what code above is also doing. restore elp status from SPELP field and clear >>it at SPELP. > >No, my point is that the text doesn't seem to be > > if (enabled) { > restore elp > clear pelp > } > >but > > if (enabled) { > restore elp > } > clear pelp > >I.e. the clear is unconditional. hmm. that's right. good catch here. > > >r~
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 82fa85a8d6..e1526c7ab5 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1022,6 +1022,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type) env->load_res = -1; set_default_nan_mode(1, &env->fp_status); +#ifdef CONFIG_USER_ONLY + /* qemu-user for riscv, fcfi is off by default */ + env->ufcfien = false; +#endif + #ifndef CONFIG_USER_ONLY if (cpu->cfg.debug) { riscv_trigger_reset_hold(env); diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index ae436a3179..8c7841fc08 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -226,6 +226,7 @@ struct CPUArchState { cfi_elp elp; #ifdef CONFIG_USER_ONLY uint32_t elf_flags; + bool ufcfien; #endif #ifndef CONFIG_USER_ONLY @@ -530,6 +531,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen); bool riscv_cpu_vector_enabled(CPURISCVState *env); void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable); int riscv_env_mmu_index(CPURISCVState *env, bool ifetch); +bool cpu_get_fcfien(CPURISCVState *env); G_NORETURN void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr, MMUAccessType access_type, int mmu_idx, uintptr_t retaddr); diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 6709622dd3..8c69c55576 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -33,6 +33,7 @@ #include "cpu_bits.h" #include "debug.h" #include "tcg/oversized-guest.h" +#include "pmp.h" int riscv_env_mmu_index(CPURISCVState *env, bool ifetch) { @@ -63,6 +64,35 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch) #endif } +bool cpu_get_fcfien(CPURISCVState *env) +{ +#ifdef CONFIG_USER_ONLY + return env->ufcfien; +#else + /* no cfi extension, return false */ + if (!env_archcpu(env)->cfg.ext_zicfilp) { + return false; + } + + switch (env->priv) { + case PRV_U: + if (riscv_has_ext(env, RVS)) { + return env->senvcfg & MENVCFG_LPE; + } + return env->menvcfg & MENVCFG_LPE; + case PRV_S: + if (env->virt_enabled) { + return env->henvcfg & HENVCFG_LPE; + } + return env->menvcfg & MENVCFG_LPE; + case PRV_M: + return env->mseccfg & MSECCFG_MLPE; + default: + g_assert_not_reached(); + } +#endif +} + void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc, uint64_t *cs_base, uint32_t *pflags) { @@ -546,6 +576,15 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env) } bool current_virt = env->virt_enabled; + /* + * If zicfilp extension available and henvcfg.LPE = 1, + * then apply SPELP mask on mstatus + */ + if (env_archcpu(env)->cfg.ext_zicfilp && + get_field(env->henvcfg, HENVCFG_LPE)) { + mstatus_mask |= SSTATUS_SPELP; + } + g_assert(riscv_has_ext(env, RVH)); if (current_virt) { @@ -1754,6 +1793,11 @@ void riscv_cpu_do_interrupt(CPUState *cs) if (env->priv <= PRV_S && cause < 64 && (((deleg >> cause) & 1) || s_injected || vs_injected)) { /* handle the trap in S-mode */ + /* save elp status */ + if (cpu_get_fcfien(env)) { + env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, env->elp); + } + if (riscv_has_ext(env, RVH)) { uint64_t hdeleg = async ? env->hideleg : env->hedeleg; @@ -1802,6 +1846,11 @@ void riscv_cpu_do_interrupt(CPUState *cs) riscv_cpu_set_mode(env, PRV_S); } else { /* handle the trap in M-mode */ + /* save elp status */ + if (cpu_get_fcfien(env)) { + env->mstatus = set_field(env->mstatus, MSTATUS_MPELP, env->elp); + } + if (riscv_has_ext(env, RVH)) { if (env->virt_enabled) { riscv_cpu_swap_hypervisor_regs(env); @@ -1833,6 +1882,15 @@ void riscv_cpu_do_interrupt(CPUState *cs) riscv_cpu_set_mode(env, PRV_M); } + /* + * Interrupt/exception/trap delivery is asynchronous event and as per + * Zisslpcfi spec CPU should clear up the ELP state. If cfi extension is + * available, clear ELP state. + */ + + if (cpu->cfg.ext_zicfilp) { + env->elp = NO_LP_EXPECTED; + } /* * NOTE: it is not necessary to yield load reservations here. It is only * necessary for an SC from "another hart" to cause a load reservation diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index 2baf5bc3ca..488116cc2e 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -313,6 +313,15 @@ target_ulong helper_sret(CPURISCVState *env) riscv_cpu_set_mode(env, prev_priv); + /* + * If forward cfi enabled for new priv, restore elp status + * and clear spelp in mstatus + */ + if (cpu_get_fcfien(env)) { + env->elp = get_field(env->mstatus, MSTATUS_SPELP); + env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0); + } + return retpc; } @@ -357,6 +366,15 @@ target_ulong helper_mret(CPURISCVState *env) riscv_cpu_set_virt_enabled(env, prev_virt); } + /* + * If forward cfi enabled for new priv, restore elp status + * and clear mpelp in mstatus + */ + if (cpu_get_fcfien(env)) { + env->elp = get_field(env->mstatus, MSTATUS_MPELP); + env->mstatus = set_field(env->mstatus, MSTATUS_MPELP, 0); + } + return retpc; }
elp state is recorded in *status on trap entry (less privilege to higher privilege) and restored in elp from *status on trap exit (higher to less privilege). Additionally this patch introduces a forward cfi helper function to determine if current privilege has forward cfi is enabled or not based on *envcfg (for U, VU, S, VU, HS) or mseccfg csr (for M). For qemu-user, a new field `ufcfien` is introduced which is by default set to false and helper function returns value deposited in `ufcfien` for qemu-user. Signed-off-by: Deepak Gupta <debug@rivosinc.com> Co-developed-by: Jim Shu <jim.shu@sifive.com> Co-developed-by: Andy Chiu <andy.chiu@sifive.com> --- target/riscv/cpu.c | 5 ++++ target/riscv/cpu.h | 2 ++ target/riscv/cpu_helper.c | 58 +++++++++++++++++++++++++++++++++++++++ target/riscv/op_helper.c | 18 ++++++++++++ 4 files changed, 83 insertions(+)