| Message ID | 20240820000129.3522346-12-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv support for control flow integrity extensions | expand |
On Mon, Aug 19, 2024 at 05:01:25PM -0700, Deepak Gupta wrote: >zicfiss protects shadow stack using new page table encodings PTE.W=0, >PTE.R=0 and PTE.X=0. This encoding is reserved if zicfiss is not >implemented or if shadow stack are not enabled. >Loads on shadow stack memory are allowed while stores to shadow stack >memory leads to access faults. Shadow stack accesses to RO memory >leads to store page fault. > >To implement special nature of shadow stack memory where only selected >stores (shadow stack stores from sspush) have to be allowed while rest >of regular stores disallowed, new MMU TLB index is created for shadow >stack. > >Signed-off-by: Deepak Gupta <debug@rivosinc.com> >Suggested-by: Richard Henderson <richard.henderson@linaro.org> >--- >@@ -1406,6 +1431,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n", > __func__, address, access_type, mmu_idx); > >+ /* If shadow stack instruction initiated this access, treat it as store */ >+ if (mmu_idx & MMU_IDX_SS_WRITE) { >+ access_type = MMU_DATA_STORE; >+ } >+ I think I forgot to address this. Do you still want me to fix this up like you had suggested? IIRC, you mentioned to use TARGET_INSN_START_EXTRA_WORDS=2. Honestly I don't know what it means and how its used. Based on git grep and some readup, are you expecting something along the below lines? """ diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index fee31b8037..dfd2efa941 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -47,7 +47,7 @@ typedef struct CPUArchState CPURISCVState; * RISC-V-specific extra insn start words: * 1: Original instruction opcode */ -#define TARGET_INSN_START_EXTRA_WORDS 1 +#define TARGET_INSN_START_EXTRA_WORDS 2 #define RV(x) ((target_ulong)1 << (x - 'A')) diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index f74a1216b1..b266177e46 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -1271,6 +1271,11 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong address, { CPUState *cs = env_cpu(env); + if (!pmp_violation && + tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] & 1) { + tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] &= ~1; + access_type = MMU_DATA_STORE; + } + switch (access_type) { case MMU_INST_FETCH: if (pmp_violation) { @@ -1433,7 +1438,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, /* If shadow stack instruction initiated this access, treat it as store */ if (mmu_idx & MMU_IDX_SS_WRITE) { - access_type = MMU_DATA_STORE; + tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] |= 1; } pmu_tlb_fill_incr_ctr(cpu, access_type); @@ -1529,6 +1534,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, if (ret == TRANSLATE_SUCCESS) { tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1), prot, mmu_idx, tlb_size); + tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] &= ~1; return true; } else if (probe) { """ > pmu_tlb_fill_incr_ctr(cpu, access_type); > if (two_stage_lookup) { > /* Two stage lookup */ >diff --git a/target/riscv/internals.h b/target/riscv/internals.h >index 0ac17bc5ad..ddbdee885b 100644 >--- a/target/riscv/internals.h >+++ b/target/riscv/internals.h >@@ -30,12 +30,15 @@ > * - U+2STAGE 0b100 > * - S+2STAGE 0b101 > * - S+SUM+2STAGE 0b110 >+ * - Shadow stack+U 0b1000 >+ * - Shadow stack+S 0b1001 > */ > #define MMUIdx_U 0 > #define MMUIdx_S 1 > #define MMUIdx_S_SUM 2 > #define MMUIdx_M 3 > #define MMU_2STAGE_BIT (1 << 2) >+#define MMU_IDX_SS_WRITE (1 << 3) > > static inline int mmuidx_priv(int mmu_idx) > { >-- >2.44.0 >
On 8/20/24 17:35, Deepak Gupta wrote: >> + /* If shadow stack instruction initiated this access, treat it as store */ >> + if (mmu_idx & MMU_IDX_SS_WRITE) { >> + access_type = MMU_DATA_STORE; >> + } >> + > > I think I forgot to address this. Do you still want me to fix this up like you > had suggested? Yes, this still needs fixing. > IIRC, you mentioned to use TARGET_INSN_START_EXTRA_WORDS=2. Honestly I don't know > what it means and how its used. Based on git grep and some readup, are you expecting > something > along the below lines? > > > """ > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index fee31b8037..dfd2efa941 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -47,7 +47,7 @@ typedef struct CPUArchState CPURISCVState; > * RISC-V-specific extra insn start words: > * 1: Original instruction opcode > */ > -#define TARGET_INSN_START_EXTRA_WORDS 1 > +#define TARGET_INSN_START_EXTRA_WORDS 2 > > #define RV(x) ((target_ulong)1 << (x - 'A')) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index f74a1216b1..b266177e46 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -1271,6 +1271,11 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong > address, > { > CPUState *cs = env_cpu(env); > > + if (!pmp_violation && > + tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] & 1) { > + tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] &= ~1; > + access_type = MMU_DATA_STORE; > + } The first thing to understand is that the unwind data is stored by the compiler and recovered by the unwinder. The unwind data is exposed to the target via one of two methods: (1) TCGCPUOps.restore_state_to_opc, i.e. riscv_restore_state_to_opc. The data[] argument contains the extra words. With this method, the extra words are restored to env and are available in a later call to riscv_cpu_do_interrupt. Compare env->bins from the first extra word, which is used exactly so. This is probably the easiest and best option. You'd promote LOAD* to STORE_AMO* while dispatching the interrupt. (2) cpu_unwind_state_data() With this method, you have immediate access to the extra words, and don't need to store them anywhere else. This is supposed to be used when we are *not* going to raise an exception, merely look something up and continue execution. Otherwise, we'd be performing the unwind operation twice, and it's not cheap. So, tcg_ctx->gen_insn_data[] is not something you'd ever touch, and this is the wrong spot to do anything. r~
On Tue, Aug 20, 2024 at 07:20:48PM +1000, Richard Henderson wrote: >On 8/20/24 17:35, Deepak Gupta wrote: >>>+ /* If shadow stack instruction initiated this access, treat it as store */ >>>+ if (mmu_idx & MMU_IDX_SS_WRITE) { >>>+ access_type = MMU_DATA_STORE; >>>+ } >>>+ >> >>I think I forgot to address this. Do you still want me to fix this up like you >>had suggested? > >Yes, this still needs fixing. > > >>IIRC, you mentioned to use TARGET_INSN_START_EXTRA_WORDS=2. Honestly I don't know >>what it means and how its used. Based on git grep and some readup, >>are you expecting something >>along the below lines? >> >> >>""" >> >>diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>index fee31b8037..dfd2efa941 100644 >>--- a/target/riscv/cpu.h >>+++ b/target/riscv/cpu.h >>@@ -47,7 +47,7 @@ typedef struct CPUArchState CPURISCVState; >> * RISC-V-specific extra insn start words: >> * 1: Original instruction opcode >> */ >>-#define TARGET_INSN_START_EXTRA_WORDS 1 >>+#define TARGET_INSN_START_EXTRA_WORDS 2 >> >> #define RV(x) ((target_ulong)1 << (x - 'A')) >> >>diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>index f74a1216b1..b266177e46 100644 >>--- a/target/riscv/cpu_helper.c >>+++ b/target/riscv/cpu_helper.c >>@@ -1271,6 +1271,11 @@ static void raise_mmu_exception(CPURISCVState >>*env, target_ulong address, >> { >> CPUState *cs = env_cpu(env); >> >>+ if (!pmp_violation && >>+ tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] & 1) { >>+ tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] &= ~1; >>+ access_type = MMU_DATA_STORE; >>+ } > >The first thing to understand is that the unwind data is stored by the >compiler and recovered by the unwinder. > >The unwind data is exposed to the target via one of two methods: > >(1) TCGCPUOps.restore_state_to_opc, i.e. riscv_restore_state_to_opc. > The data[] argument contains the extra words. > > With this method, the extra words are restored to env and are > available in a later call to riscv_cpu_do_interrupt. > Compare env->bins from the first extra word, which is used exactly so. > > This is probably the easiest and best option. > You'd promote LOAD* to STORE_AMO* while dispatching the interrupt. > >(2) cpu_unwind_state_data() > > With this method, you have immediate access to the extra words, > and don't need to store them anywhere else. > > This is supposed to be used when we are *not* going to raise > an exception, merely look something up and continue execution. > Otherwise, we'd be performing the unwind operation twice, > and it's not cheap. > >So, tcg_ctx->gen_insn_data[] is not something you'd ever touch, >and this is the wrong spot to do anything. Thanks for more information and guiding me through this. Not going to say that I still understand everything. But I looked at one arm example. Before I do something more with it. I wanted to run it by you. Something on the below lines? I've one question as well for you in comment. """" diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index fee31b8037..b4e04fe849 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -46,8 +46,14 @@ typedef struct CPUArchState CPURISCVState; /* * RISC-V-specific extra insn start words: * 1: Original instruction opcode + * 2: more information about instruction */ -#define TARGET_INSN_START_EXTRA_WORDS 1 +#define TARGET_INSN_START_EXTRA_WORDS 2 + +/* + * b0: Whether a shadow stack operation/instruction or not. + */ +#define RISCV_INSN_START_WORD2_SS_OP 1 #define RV(x) ((target_ulong)1 << (x - 'A')) @@ -226,6 +232,7 @@ struct CPUArchState { bool elp; /* shadow stack register for zicfiss extension */ target_ulong ssp; + bool ss_op; /* sw check code for sw check exception */ target_ulong sw_check_code; #ifdef CONFIG_USER_ONLY diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index f74a1216b1..c28b13d42c 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -1777,6 +1777,10 @@ void riscv_cpu_do_interrupt(CPUState *cs) target_ulong mtval2 = 0; if (!async) { + /* shadow stack op, promote load page fault to store page fault */ + if (env->ss_op && cause == RISCV_EXCP_LOAD_PAGE_FAULT) { + cause = RISCV_EXCP_STORE_PAGE_FAULT; + } /* set tval to badaddr for traps with address information */ switch (cause) { case RISCV_EXCP_SEMIHOST: diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 4da26cb926..c0f21fe3db 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -129,6 +129,7 @@ static void riscv_restore_state_to_opc(CPUState *cs, env->pc = pc; } env->bins = data[1]; + env->ss_op = data[2] & RISCV_INSN_START_WORD2_SS_OP; } static const TCGCPUOps riscv_tcg_ops = { diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 580aa23c5b..6f952db823 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -1271,7 +1271,7 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu) pc_next &= ~TARGET_PAGE_MASK; } - tcg_gen_insn_start(pc_next, 0); + tcg_gen_insn_start(pc_next, 0, 0); ctx->insn_start_updated = false; } @@ -1301,6 +1301,14 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) ctx->base.is_jmp = DISAS_NORETURN; } + /* shadow stack index means shadow stack instruction is translated */ + if (ctx->mem_idx & MMU_IDX_SS_WRITE) { + /* Is this needed to set true? */ + ctx->insn_start_updated = true; + tcg_set_insn_start_param(ctx->base.insn_start, 2, + RISCV_INSN_START_WORD2_SS_OP); + } + /* Only the first insn within a TB is allowed to cross a page boundary. */ if (ctx->base.is_jmp == DISAS_NEXT) { if (ctx->itrigger || !is_same_page(&ctx->base, ctx->base.pc_next)) { """ > > >r~
On Tue, Aug 20, 2024 at 11:55 AM Deepak Gupta <debug@rivosinc.com> wrote: > > On Tue, Aug 20, 2024 at 07:20:48PM +1000, Richard Henderson wrote: > >On 8/20/24 17:35, Deepak Gupta wrote: > >>>+ /* If shadow stack instruction initiated this access, treat it as store */ > >>>+ if (mmu_idx & MMU_IDX_SS_WRITE) { > >>>+ access_type = MMU_DATA_STORE; > >>>+ } > >>>+ > >> > >>I think I forgot to address this. Do you still want me to fix this up like you > >>had suggested? > > > >Yes, this still needs fixing. > > > > > >>IIRC, you mentioned to use TARGET_INSN_START_EXTRA_WORDS=2. Honestly I don't know > >>what it means and how its used. Based on git grep and some readup, > >>are you expecting something > >>along the below lines? > >> > >> > >>""" > >> > >>diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > >>index fee31b8037..dfd2efa941 100644 > >>--- a/target/riscv/cpu.h > >>+++ b/target/riscv/cpu.h > >>@@ -47,7 +47,7 @@ typedef struct CPUArchState CPURISCVState; > >> * RISC-V-specific extra insn start words: > >> * 1: Original instruction opcode > >> */ > >>-#define TARGET_INSN_START_EXTRA_WORDS 1 > >>+#define TARGET_INSN_START_EXTRA_WORDS 2 > >> > >> #define RV(x) ((target_ulong)1 << (x - 'A')) > >> > >>diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > >>index f74a1216b1..b266177e46 100644 > >>--- a/target/riscv/cpu_helper.c > >>+++ b/target/riscv/cpu_helper.c > >>@@ -1271,6 +1271,11 @@ static void raise_mmu_exception(CPURISCVState > >>*env, target_ulong address, > >> { > >> CPUState *cs = env_cpu(env); > >> > >>+ if (!pmp_violation && > >>+ tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] & 1) { > >>+ tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] &= ~1; > >>+ access_type = MMU_DATA_STORE; > >>+ } > > > >The first thing to understand is that the unwind data is stored by the > >compiler and recovered by the unwinder. > > > >The unwind data is exposed to the target via one of two methods: > > > >(1) TCGCPUOps.restore_state_to_opc, i.e. riscv_restore_state_to_opc. > > The data[] argument contains the extra words. > > > > With this method, the extra words are restored to env and are > > available in a later call to riscv_cpu_do_interrupt. > > Compare env->bins from the first extra word, which is used exactly so. > > > > This is probably the easiest and best option. > > You'd promote LOAD* to STORE_AMO* while dispatching the interrupt. > > > >(2) cpu_unwind_state_data() > > > > With this method, you have immediate access to the extra words, > > and don't need to store them anywhere else. > > > > This is supposed to be used when we are *not* going to raise > > an exception, merely look something up and continue execution. > > Otherwise, we'd be performing the unwind operation twice, > > and it's not cheap. > > > >So, tcg_ctx->gen_insn_data[] is not something you'd ever touch, > >and this is the wrong spot to do anything. > > Thanks for more information and guiding me through this. > > Not going to say that I still understand everything. But I looked > at one arm example. Before I do something more with it. I wanted to run > it by you. > > Something on the below lines? I've one question as well for you in comment. > > """" > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index fee31b8037..b4e04fe849 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -46,8 +46,14 @@ typedef struct CPUArchState CPURISCVState; > /* > * RISC-V-specific extra insn start words: > * 1: Original instruction opcode > + * 2: more information about instruction > */ > -#define TARGET_INSN_START_EXTRA_WORDS 1 > +#define TARGET_INSN_START_EXTRA_WORDS 2 > + > +/* > + * b0: Whether a shadow stack operation/instruction or not. > + */ > +#define RISCV_INSN_START_WORD2_SS_OP 1 > > #define RV(x) ((target_ulong)1 << (x - 'A')) > > @@ -226,6 +232,7 @@ struct CPUArchState { > bool elp; > /* shadow stack register for zicfiss extension */ > target_ulong ssp; > + bool ss_op; > /* sw check code for sw check exception */ > target_ulong sw_check_code; > #ifdef CONFIG_USER_ONLY > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index f74a1216b1..c28b13d42c 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -1777,6 +1777,10 @@ void riscv_cpu_do_interrupt(CPUState *cs) > target_ulong mtval2 = 0; > > if (!async) { > + /* shadow stack op, promote load page fault to store page fault */ > + if (env->ss_op && cause == RISCV_EXCP_LOAD_PAGE_FAULT) { > + cause = RISCV_EXCP_STORE_PAGE_FAULT; > + } > /* set tval to badaddr for traps with address information */ > switch (cause) { > case RISCV_EXCP_SEMIHOST: > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 4da26cb926..c0f21fe3db 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -129,6 +129,7 @@ static void riscv_restore_state_to_opc(CPUState *cs, > env->pc = pc; > } > env->bins = data[1]; > + env->ss_op = data[2] & RISCV_INSN_START_WORD2_SS_OP; > } > > static const TCGCPUOps riscv_tcg_ops = { > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 580aa23c5b..6f952db823 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -1271,7 +1271,7 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu) > pc_next &= ~TARGET_PAGE_MASK; > } > > - tcg_gen_insn_start(pc_next, 0); > + tcg_gen_insn_start(pc_next, 0, 0); > ctx->insn_start_updated = false; > } > > @@ -1301,6 +1301,14 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) > ctx->base.is_jmp = DISAS_NORETURN; > } > > + /* shadow stack index means shadow stack instruction is translated */ > + if (ctx->mem_idx & MMU_IDX_SS_WRITE) { This is a wrong check because we never store MMU_IDX_SS_WRITE in `ctx->mem_idx` But I can place an indicator in `DisasContext` and set it up in `trans_xxx` for shadow stack instr. > + /* Is this needed to set true? */ > + ctx->insn_start_updated = true; > + tcg_set_insn_start_param(ctx->base.insn_start, 2, > + RISCV_INSN_START_WORD2_SS_OP); > + } > + > /* Only the first insn within a TB is allowed to cross a page boundary. */ > if (ctx->base.is_jmp == DISAS_NEXT) { > if (ctx->itrigger || !is_same_page(&ctx->base, ctx->base.pc_next)) { > > """ > > > > > >r~
On 8/21/24 04:55, Deepak Gupta wrote: > Something on the below lines? I've one question as well for you in comment. > > """" > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index fee31b8037..b4e04fe849 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -46,8 +46,14 @@ typedef struct CPUArchState CPURISCVState; > /* > * RISC-V-specific extra insn start words: > * 1: Original instruction opcode > + * 2: more information about instruction > */ > -#define TARGET_INSN_START_EXTRA_WORDS 1 > +#define TARGET_INSN_START_EXTRA_WORDS 2 > + > +/* > + * b0: Whether a shadow stack operation/instruction or not. > + */ > +#define RISCV_INSN_START_WORD2_SS_OP 1 Ah, here: not shadow-stack specific. Set for any insn which should always generate STORE_AMO, including the actual AMO instructions. It's a current emulation error, IIRC. > @@ -226,6 +232,7 @@ struct CPUArchState { > bool elp; > /* shadow stack register for zicfiss extension */ > target_ulong ssp; > + bool ss_op; For generality, maybe just store the whole word as excp_uw2? > if (!async) { > + /* shadow stack op, promote load page fault to store page fault */ > + if (env->ss_op && cause == RISCV_EXCP_LOAD_PAGE_FAULT) { > + cause = RISCV_EXCP_STORE_PAGE_FAULT; > + } > /* set tval to badaddr for traps with address information */ > switch (cause) { > case RISCV_EXCP_SEMIHOST: case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: if (env->excp_uw2 & RISCV_UW2_ALWAYS_STORE_AMO) { cause = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT; } goto load_store_fault; case RISCV_EXCP_LOAD_ACCESS_FAULT: ... case RISCV_EXCP_LOAD_PAGE_FAULT: ... case RISCV_EXCP_STORE_PAGE_FAULT: load_store_fault: > @@ -1301,6 +1301,14 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, > CPUState *cpu) > ctx->base.is_jmp = DISAS_NORETURN; > } > > + /* shadow stack index means shadow stack instruction is translated */ > + if (ctx->mem_idx & MMU_IDX_SS_WRITE) { > + /* Is this needed to set true? */ > + ctx->insn_start_updated = true; > + tcg_set_insn_start_param(ctx->base.insn_start, 2, > + RISCV_INSN_START_WORD2_SS_OP); > + } No, SS_WRITE is never part of mem_idx, and setting insn_start_updated here would break things. You'll want to change decode_save_opcode() to take the second parameter (or introduce a new helper for the second parameter, leaving decode_save_opcode alone). But you do have to handle the update on a per-insn basis. r~
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index d3115da28d..f74a1216b1 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -894,6 +894,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical, hwaddr ppn; int napot_bits = 0; target_ulong napot_mask; + bool is_sstack_idx = ((mmu_idx & MMU_IDX_SS_WRITE) == MMU_IDX_SS_WRITE); + bool sstack_page = false; /* * Check if we should use the background registers for the two @@ -1102,21 +1104,36 @@ restart: return TRANSLATE_FAIL; } + target_ulong rwx = pte & (PTE_R | PTE_W | PTE_X); /* Check for reserved combinations of RWX flags. */ - switch (pte & (PTE_R | PTE_W | PTE_X)) { - case PTE_W: + switch (rwx) { case PTE_W | PTE_X: return TRANSLATE_FAIL; + case PTE_W: + /* if bcfi enabled, PTE_W is not reserved and shadow stack page */ + if (cpu_get_bcfien(env) && first_stage) { + sstack_page = true; + /* if ss index, read and write allowed. else only read allowed */ + rwx = is_sstack_idx ? PTE_R | PTE_W : PTE_R; + break; + } + return TRANSLATE_FAIL; + case PTE_R: + /* shadow stack writes to readonly memory are page faults */ + if (is_sstack_idx && access_type == MMU_DATA_STORE) { + return TRANSLATE_FAIL; + } + break; } int prot = 0; - if (pte & PTE_R) { + if (rwx & PTE_R) { prot |= PAGE_READ; } - if (pte & PTE_W) { + if (rwx & PTE_W) { prot |= PAGE_WRITE; } - if (pte & PTE_X) { + if (rwx & PTE_X) { bool mxr = false; /* @@ -1161,7 +1178,7 @@ restart: if (!((prot >> access_type) & 1)) { /* Access check failed */ - return TRANSLATE_FAIL; + return sstack_page ? TRANSLATE_PMP_FAIL : TRANSLATE_FAIL; } target_ulong updated_pte = pte; @@ -1348,9 +1365,17 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr, break; case MMU_DATA_LOAD: cs->exception_index = RISCV_EXCP_LOAD_ADDR_MIS; + /* shadow stack mis aligned accesses are access faults */ + if (mmu_idx & MMU_IDX_SS_WRITE) { + cs->exception_index = RISCV_EXCP_LOAD_ACCESS_FAULT; + } break; case MMU_DATA_STORE: cs->exception_index = RISCV_EXCP_STORE_AMO_ADDR_MIS; + /* shadow stack mis aligned accesses are access faults */ + if (mmu_idx & MMU_IDX_SS_WRITE) { + cs->exception_index = RISCV_EXCP_STORE_AMO_ACCESS_FAULT; + } break; default: g_assert_not_reached(); @@ -1406,6 +1431,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n", __func__, address, access_type, mmu_idx); + /* If shadow stack instruction initiated this access, treat it as store */ + if (mmu_idx & MMU_IDX_SS_WRITE) { + access_type = MMU_DATA_STORE; + } + pmu_tlb_fill_incr_ctr(cpu, access_type); if (two_stage_lookup) { /* Two stage lookup */ diff --git a/target/riscv/internals.h b/target/riscv/internals.h index 0ac17bc5ad..ddbdee885b 100644 --- a/target/riscv/internals.h +++ b/target/riscv/internals.h @@ -30,12 +30,15 @@ * - U+2STAGE 0b100 * - S+2STAGE 0b101 * - S+SUM+2STAGE 0b110 + * - Shadow stack+U 0b1000 + * - Shadow stack+S 0b1001 */ #define MMUIdx_U 0 #define MMUIdx_S 1 #define MMUIdx_S_SUM 2 #define MMUIdx_M 3 #define MMU_2STAGE_BIT (1 << 2) +#define MMU_IDX_SS_WRITE (1 << 3) static inline int mmuidx_priv(int mmu_idx) {
zicfiss protects shadow stack using new page table encodings PTE.W=0, PTE.R=0 and PTE.X=0. This encoding is reserved if zicfiss is not implemented or if shadow stack are not enabled. Loads on shadow stack memory are allowed while stores to shadow stack memory leads to access faults. Shadow stack accesses to RO memory leads to store page fault. To implement special nature of shadow stack memory where only selected stores (shadow stack stores from sspush) have to be allowed while rest of regular stores disallowed, new MMU TLB index is created for shadow stack. Signed-off-by: Deepak Gupta <debug@rivosinc.com> Suggested-by: Richard Henderson <richard.henderson@linaro.org> --- target/riscv/cpu_helper.c | 42 +++++++++++++++++++++++++++++++++------ target/riscv/internals.h | 3 +++ 2 files changed, 39 insertions(+), 6 deletions(-)