Re: [PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow stack instructions

[Richard Henderson]

On 8/8/24 07:23, Deepak Gupta wrote:
> On Wed, Aug 07, 2024 at 12:43:31PM +1000, Richard Henderson wrote:
> > On 8/7/24 10:06, Deepak Gupta wrote:
> > > Shadow stack instructions shadow stack mmu index for load/stores.
> > > `MMU_IDX_SS_ACCESS` at bit positon 3 is used as shadow stack index.
> > > Shadow stack mmu index depend on privilege and SUM bit. If shadow stack
> > > accesses happening in user mode, shadow stack mmu index = 0b1000. If
> > > shaodw stack access happening in supervisor mode mmu index = 0b1001. If
> > > shadow stack access happening in supervisor mode with SUM=1 then mmu
> > > index = 0b1010
> > >
> > > Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> > > ---
> > >  target/riscv/cpu.h                            | 13 ++++++++++
> > >  target/riscv/cpu_helper.c                     |  3 +++
> > >  target/riscv/insn_trans/trans_rva.c.inc       |  8 ++++++
> > >  target/riscv/insn_trans/trans_rvzicfiss.c.inc |  6 +++++
> > >  target/riscv/internals.h                      |  1 +
> > >  target/riscv/translate.c                      | 25 +++++++++++++++++++
> > >  6 files changed, 56 insertions(+)
> > >
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 6da94c417c..3ad220a9fe 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -615,6 +615,19 @@ FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
> > >  FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
> > >  /* zicfiss needs a TB flag so that correct TB is located based on tb flags */
> > >  FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
> > > +/*
> > > + * zicfiss shadow stack is special memory on which regular stores aren't
> > > + * allowed but shadow stack stores are allowed. Shadow stack stores can
> > > + * happen as `sspush` or `ssamoswap` instructions. `sspush` implicitly
> > > + * takes shadow stack address from CSR_SSP. But `ssamoswap` takes address
> > > + * from encoded input register and it will be used by supervisor software
> > > + * to access (read/write) user shadow stack for setting up rt_frame during
> > > + * signal delivery. Supervisor software will do so by setting SUM=1. Thus
> > > + * a TB flag is needed if SUM was 1 during TB generation to correctly
> > > + * reflect memory permissions to access shadow stack user memory from
> > > + * supervisor mode.
> > > + */
> > > +FIELD(TB_FLAGS, SUM, 31, 1)
> >
> > This is already encoded into the mmu_idx as MMUIdx_S_SUM.
>
> This is where I need some help / suggestion and clarifications.
>
> `riscv_env_mmu_index` is the which does mode --> mmu index translation and 
> that's
> where `MMUIdx_S_SUM` is set.
>
> Although above function assumes following things
>     -- Only loads ands stores are supposed to do read and write.
>     -- Translates env/priv --> mmu index
>
> In case of shadow stack, we need to hold following true:
> Shadow stack are not writeable via regular stores but are allowed to be 
> readable.
> Shadow stack are writeable only via shadow stack instruction.
> Shadow stack instructions can't operate on non-shadow stack memory.
>
> This let me to create a new mmu index (as you saw in patches). This mmu index 
> is only
> setup by shadow stack instruction and thus has to be known at translation time

All good so far.

> There is no way of telling in `riscv_env_mmu_index` about whether mmu index is 
> requested
> for regular load/store or some other instruction (in this case shadow stack 
> instruction).
> If that is available then I think I can use `riscv_env_mmu_index`.

What you miss is that the result of riscv_env_mmu_index is stored

  ctx->mem_idx

So that takes care of U, S, SUM, M, VS, VU, etc.  All you need at
this point is to or in your shadow stack bit:

  ctx->mem_idx | MMU_IDX_SS_ACCESS

(Perhaps SS_WRITE is a better name, since read access is never prohibited?)

Note that you can do this without ifdefs -- user-only will happily accept and ignore the 
mmu index.  Also note that user-only will *not* be able to restrict access to the shadow 
stack pages in the way the spec describes.  We rely on the host mmu for read/write 
permission for user-only.  For now -- changing that is a long term goal.


> Question:
> I see that `riscv_env_mmu_index` could be called from a bunch of places in (like
> `accel/tcg/ldst_common.c.inc` as well. Does it exclude loads, stores which 
> calculate mmu
> indexes during translation (like shadow stack load, stores) ?

It means you cannot use the legacy interfaces for the shadow stack.
The current interfaces:

 *        cpu_ld{sign}{size}{end}_mmuidx_ra(env, ptr, mmu_idx, retaddr)
 *        cpu_ld{sign}{size}{end}_mmu(env, ptr, oi, retaddr)
...
 *        cpu_st{size}{end}_mmuidx_ra(env, ptr, val, mmu_idx, retaddr)
 *        cpu_st{size}{end}_mmu(env, ptr, val, oi, retaddr)

take the mmu_idx as a parameter.

But as it happens, the shadow stack instructions are simple enough to implement all 
inline, so you won't need to call the out-of-line load/store functions from cpu helpers.


r~