Re: [PATCH v11 13/20] target/riscv: mmu changes for zicfiss shadow stack protection

[Alistair Francis]

On Thu, Aug 29, 2024 at 9:45 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> On Thu, Aug 29, 2024 at 09:29:49AM +1000, Alistair Francis wrote:
> >On Thu, Aug 29, 2024 at 3:49 AM Deepak Gupta <debug@rivosinc.com> wrote:
> >>
> >> zicfiss protects shadow stack using new page table encodings PTE.W=1,
> >> 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>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>  target/riscv/cpu_helper.c | 37 +++++++++++++++++++++++++++++++------
> >>  target/riscv/internals.h  |  3 +++
> >>  2 files changed, 34 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index be4ac3d54e..39544cade6 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -893,6 +893,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
> >> @@ -1101,21 +1103,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) {
>
> While responding to your question, I noticed there is a bug here. Its a 
> leftover from
> previous patches where I was promoting shadow stack loads to stores. No need 
> to check
> `access_type == MMU_DATA_STORE` because we store unwind information as part 
> of tcg
> compile.
>
> Will fix it.
>
> >> +            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;
> >>
> >>          /*
> >> @@ -1160,7 +1177,7 @@ restart:
> >>
> >>      if (!((prot >> access_type) & 1)) {
> >>          /* Access check failed */
> >> -        return TRANSLATE_FAIL;
> >> +        return sstack_page ? TRANSLATE_PMP_FAIL : TRANSLATE_FAIL;
> >
> >Why is it a PMP error if it's a shadow stack page?
>
> A shadow stack page is readable by regular loads.
> We are making sure of that in `case PTE_W` in above switch case.
> But shadow stack page is not writeable via regular stores. And must raise
> access fault. return code `TRANSLATE_PMP_FAIL` is translated to access fault
> while raising fault.

Ah, ok. It's worth commenting that we are returning TRANSLATE_PMP_FAIL
as that will be translated to an access fault

Alistair

>
> >
> >Alistair