| Message ID | 20241003183342.679249-15-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv support for control flow integrity extensions | expand |
On Thu, Oct 03, 2024 at 11:33:35AM -0700, Deepak Gupta wrote: >`check_zicbom_access` (`cbo.clean/flush/inval`) may probe shadow stack >memory and must always raise store/AMO access fault because it has store >semantics. > >For non-shadow stack memory even though `cbo.clean/flush/inval` have >store semantics, it will not fault if read is allowed (probably to follow >`clflush` on x86). Although if read is not allowed, eventually >`probe_write` will do store page (or access) fault (if permissions don't >allow it). > >cbo operations on shadow stack memory must always raise store access fault. >Thus extending `get_physical_address` to recieve `probe` parameter as well. > >Signed-off-by: Deepak Gupta <debug@rivosinc.com> >--- > target/riscv/cpu_helper.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > >diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >index 5580f5f3f3..ab46f694b5 100644 >--- a/target/riscv/cpu_helper.c >+++ b/target/riscv/cpu_helper.c >@@ -884,7 +884,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical, > target_ulong *fault_pte_addr, > int access_type, int mmu_idx, > bool first_stage, bool two_stage, >- bool is_debug) >+ bool is_debug, bool is_probe) > { > /* > * NOTE: the env->pc value visible here will not be >@@ -1030,7 +1030,7 @@ restart: > int vbase_ret = get_physical_address(env, &vbase, &vbase_prot, > base, NULL, MMU_DATA_LOAD, > MMUIdx_U, false, true, >- is_debug); >+ is_debug, false); > > if (vbase_ret != TRANSLATE_SUCCESS) { > if (fault_pte_addr) { >@@ -1117,8 +1117,11 @@ restart: > /* 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; >+ /* >+ * if ss index, read and write allowed. else if not a probe >+ * then only read allowed >+ */ >+ rwx = is_sstack_idx ? (PTE_R | PTE_W) : (is_probe ? rwx : PTE_R); hmm... there is a bug here. Above would allow writes to go through if probed. It should be rwx = is_sstack_idx ? (PTE_R | PTE_W) : (is_probe ? 0 : PTE_R); I'll fix it in next version. Still need higher level feedback on patch that if it's okay to extend `get_physical_address` to recieve `probe` parameter. > break; > } > return TRANSLATE_FAIL; >@@ -1327,13 +1330,13 @@ hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) > int mmu_idx = riscv_env_mmu_index(&cpu->env, false); > > if (get_physical_address(env, &phys_addr, &prot, addr, NULL, 0, mmu_idx, >- true, env->virt_enabled, true)) { >+ true, env->virt_enabled, true, false)) { > return -1; > } > > if (env->virt_enabled) { > if (get_physical_address(env, &phys_addr, &prot, phys_addr, NULL, >- 0, MMUIdx_U, false, true, true)) { >+ 0, MMUIdx_U, false, true, true, false)) { > return -1; > } > } >@@ -1447,7 +1450,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > /* Two stage lookup */ > ret = get_physical_address(env, &pa, &prot, address, > &env->guest_phys_fault_addr, access_type, >- mmu_idx, true, true, false); >+ mmu_idx, true, true, false, probe); > > /* > * A G-stage exception may be triggered during two state lookup. >@@ -1470,7 +1473,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > > ret = get_physical_address(env, &pa, &prot2, im_address, NULL, > access_type, MMUIdx_U, false, true, >- false); >+ false, probe); > > qemu_log_mask(CPU_LOG_MMU, > "%s 2nd-stage address=%" VADDR_PRIx >@@ -1507,7 +1510,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > } else { > /* Single stage lookup */ > ret = get_physical_address(env, &pa, &prot, address, NULL, >- access_type, mmu_idx, true, false, false); >+ access_type, mmu_idx, true, false, false, >+ probe); > > qemu_log_mask(CPU_LOG_MMU, > "%s address=%" VADDR_PRIx " ret %d physical " >-- >2.45.0 >
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 5580f5f3f3..ab46f694b5 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -884,7 +884,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical, target_ulong *fault_pte_addr, int access_type, int mmu_idx, bool first_stage, bool two_stage, - bool is_debug) + bool is_debug, bool is_probe) { /* * NOTE: the env->pc value visible here will not be @@ -1030,7 +1030,7 @@ restart: int vbase_ret = get_physical_address(env, &vbase, &vbase_prot, base, NULL, MMU_DATA_LOAD, MMUIdx_U, false, true, - is_debug); + is_debug, false); if (vbase_ret != TRANSLATE_SUCCESS) { if (fault_pte_addr) { @@ -1117,8 +1117,11 @@ restart: /* 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; + /* + * if ss index, read and write allowed. else if not a probe + * then only read allowed + */ + rwx = is_sstack_idx ? (PTE_R | PTE_W) : (is_probe ? rwx : PTE_R); break; } return TRANSLATE_FAIL; @@ -1327,13 +1330,13 @@ hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) int mmu_idx = riscv_env_mmu_index(&cpu->env, false); if (get_physical_address(env, &phys_addr, &prot, addr, NULL, 0, mmu_idx, - true, env->virt_enabled, true)) { + true, env->virt_enabled, true, false)) { return -1; } if (env->virt_enabled) { if (get_physical_address(env, &phys_addr, &prot, phys_addr, NULL, - 0, MMUIdx_U, false, true, true)) { + 0, MMUIdx_U, false, true, true, false)) { return -1; } } @@ -1447,7 +1450,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, /* Two stage lookup */ ret = get_physical_address(env, &pa, &prot, address, &env->guest_phys_fault_addr, access_type, - mmu_idx, true, true, false); + mmu_idx, true, true, false, probe); /* * A G-stage exception may be triggered during two state lookup. @@ -1470,7 +1473,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, ret = get_physical_address(env, &pa, &prot2, im_address, NULL, access_type, MMUIdx_U, false, true, - false); + false, probe); qemu_log_mask(CPU_LOG_MMU, "%s 2nd-stage address=%" VADDR_PRIx @@ -1507,7 +1510,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, } else { /* Single stage lookup */ ret = get_physical_address(env, &pa, &prot, address, NULL, - access_type, mmu_idx, true, false, false); + access_type, mmu_idx, true, false, false, + probe); qemu_log_mask(CPU_LOG_MMU, "%s address=%" VADDR_PRIx " ret %d physical "
`check_zicbom_access` (`cbo.clean/flush/inval`) may probe shadow stack memory and must always raise store/AMO access fault because it has store semantics. For non-shadow stack memory even though `cbo.clean/flush/inval` have store semantics, it will not fault if read is allowed (probably to follow `clflush` on x86). Although if read is not allowed, eventually `probe_write` will do store page (or access) fault (if permissions don't allow it). cbo operations on shadow stack memory must always raise store access fault. Thus extending `get_physical_address` to recieve `probe` parameter as well. Signed-off-by: Deepak Gupta <debug@rivosinc.com> --- target/riscv/cpu_helper.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)