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

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> @@ -1105,15 +1119,45 @@ restart:
>           return TRANSLATE_FAIL;
>       }
>   
> +    /*
> +     * When backward CFI is enabled, the R=0, W=1, X=0 reserved encoding
> +     * is used to mark Shadow Stack (SS) pages. If back CFI enabled, allow
> +     * normal loads on SS pages, regular stores raise store access fault
> +     * and avoid hitting the reserved-encoding case. Only shadow stack
> +     * stores are allowed on SS pages. Shadow stack loads and stores on
> +     * regular memory (non-SS) raise load and store/AMO access fault.
> +     * Second stage translations don't participate in Shadow Stack.
> +     */
> +    sstack_page = (cpu_get_bcfien(env) && first_stage &&
> +                  ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_W));
> +
>       /* Check for reserved combinations of RWX flags. */
>       switch (pte & (PTE_R | PTE_W | PTE_X)) {
> -    case PTE_W:
>       case PTE_W | PTE_X:
> +    case PTE_W:
> +        if (sstack_page) { /* if shadow stack page, PTE_W is not reserved */
> +            break;

This is oddly written, and duplicates checks.  Better as

    switch (pte & RWX) {
    case W | X:
        return TRANSLATE_FAIL;
    case W:
        if (bcfi && first_stage) {
            sstack_page = true;
            break;
        }
        return TRANSLATE_FAIL;
    }


> +    /* 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;
> +    }
> +
>       int prot = 0;
> -    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) {
>           prot |= PAGE_READ;
>       }

I feel like this logic could be simplified somehow.
I'll think about it.

> @@ -1409,6 +1461,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
> size,
>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>                     __func__, address, access_type, mmu_idx);
>   
> +    /* If shadow stack instruction initiated this access, treat it as store */
> +    if (mmu_idx & MMU_IDX_SS_ACCESS) {
> +        access_type = MMU_DATA_STORE;
> +    }

I know you're trying to massage the fault type, but I think this is the wrong 
place.


r~