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

[Deepak Gupta <debug@rivosinc.com>]

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.

>
>Alistair