[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack
From: |
Richard Henderson |
Subject: |
Re: [PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack protection |
Date: |
Wed, 7 Aug 2024 13:19:55 +1000 |
User-agent: |
Mozilla Thunderbird |
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~
- Re: [PATCH v3 04/20] target/riscv: save and restore elp state on priv transitions, (continued)
[PATCH v3 07/20] target/riscv: zicfilp `lpad` impl and branch tracking, Deepak Gupta, 2024/08/06
[PATCH v3 05/20] target/riscv: additional code information for sw check, Deepak Gupta, 2024/08/06
[PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack protection, Deepak Gupta, 2024/08/06
- Re: [PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack protection,
Richard Henderson <=
[PATCH v3 12/20] target/riscv: implement zicfiss instructions, Deepak Gupta, 2024/08/06
[PATCH v3 17/20] disas/riscv: enable disassembly for compressed sspush/sspopchk, Deepak Gupta, 2024/08/06
[PATCH v3 09/20] target/riscv: Add zicfiss extension, Deepak Gupta, 2024/08/06
[PATCH v3 11/20] target/riscv: tb flag for shadow stack instructions, Deepak Gupta, 2024/08/06