Re: [PATCH v5 11/15] target/riscv: mmu changes for zicfiss shadow stack protection

[Deepak Gupta]

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~