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

[Richard Henderson]

On 8/16/24 11:07, 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>
> ---
>   target/riscv/cpu_helper.c | 52 +++++++++++++++++++++++++++++++++++++--
>   target/riscv/internals.h  |  3 +++
>   2 files changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d3115da28d..4d282fd9ed 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -817,6 +817,18 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong 
> newpriv)
>       env->load_res = -1;
>   }
>   
> +static bool legal_sstack_access(int access_type, bool sstack_inst,
> +                                bool sstack_attribute)
> +{
> +    /*
> +     * Read/write/execution permissions are checked as usual. Shadow
> +     * stack enforcement is just that (1) instruction type must match
> +     * the attribute unless (2) a non-SS load to an SS region.
> +     */
> +    return (sstack_inst == sstack_attribute) ||
> +        ((access_type == MMU_DATA_LOAD) && sstack_attribute);
> +}

While helpers are usually helpful, the invocation and logic of this one isn't.

> @@ -894,6 +906,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr 
> *physical,
>       hwaddr ppn;
>       int napot_bits = 0;
>       target_ulong napot_mask;
> +    bool is_sstack_insn = ((mmu_idx & MMU_IDX_SS_WRITE) == MMU_IDX_SS_WRITE);

is_sstack_idx is a better name, because we are testing the idx.
The fact that this idx is intended to be used only by sstack insns is beside 
the point.

> +    bool sstack_page = false;
>   
>       /*
>        * Check if we should use the background registers for the two
> @@ -1104,13 +1118,34 @@ restart:
>   
>       /* Check for reserved combinations of RWX flags. */
>       switch (pte & (PTE_R | PTE_W | PTE_X)) {

Perhaps

    rwx = ptw & ...;
    switch (rwx)

> -    case PTE_W:
>       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;

Extra space.

Perhaps, at this point,

    /* Shadow stack pages are globally readable,
       but only writable from the sstack idx. */
    rwx = is_sstack_idx ? RWX_R | RWX_W : RWX_R;

> +    /* Illegal combo of instruction type and page attribute */
> +    if (!legal_sstack_access(access_type, is_sstack_insn,
> +                            sstack_page)) {
> +        /* shadow stack instruction and RO page then it's a page fault */
> +        if (is_sstack_insn && ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_R)) {
> +                return TRANSLATE_FAIL;
> +        }
> +        /* In all other cases it's an access fault, so raise PMP_FAIL */
> +        return TRANSLATE_PMP_FAIL;

Continuing the above switch,

    case PTE_R:
        /* Shadow stack writes to read-only pages are page faults. */
        if (is_sstack_idx && access_type == MMU_DATA_STORE) {
            return TRANSLATE_FAIL;
        }
        break;

> -    if (pte & PTE_R) {
> +    /*
> +     * If PTE has read bit in it or it's shadow stack page,
> +     * then reads allowed
> +     */
> +    if ((pte & PTE_R) || sstack_page) {

Using rwx, no longer need to special case sstack_page.

The second half of leval_sstack_access can be handled by adjusting

    if (!((prot >> access_type) & 1)) {
        /* Access check failed */
        return sstack_page ? TRANSLATE_PMP_FAIL : TRANSLATE_FAIL;
    }

I don't think you need or want to move this ahead of the user/supervisor check.  I see "U 
and SUM bit enforcement is performed normally", which makes sense.  Differing behavior for 
user/supervisor could allow a timing attack against kernel pages to identify where the 
kernel shadow stack is located.  (Not an observation on qemu, but an observation on the 
architecture spec.)


r~