[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow sta
|
From: |
Deepak Gupta |
|
Subject: |
Re: [PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow stack instructions |
|
Date: |
Wed, 7 Aug 2024 16:13:28 -0700 |
On Thu, Aug 08, 2024 at 08:57:47AM +1000, Richard Henderson wrote:
On 8/8/24 07:23, Deepak Gupta wrote:
On Wed, Aug 07, 2024 at 12:43:31PM +1000, Richard Henderson wrote:
On 8/7/24 10:06, Deepak Gupta wrote:
Shadow stack instructions shadow stack mmu index for load/stores.
`MMU_IDX_SS_ACCESS` at bit positon 3 is used as shadow stack index.
Shadow stack mmu index depend on privilege and SUM bit. If shadow stack
accesses happening in user mode, shadow stack mmu index = 0b1000. If
shaodw stack access happening in supervisor mode mmu index = 0b1001. If
shadow stack access happening in supervisor mode with SUM=1 then mmu
index = 0b1010
Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
target/riscv/cpu.h | 13 ++++++++++
target/riscv/cpu_helper.c | 3 +++
target/riscv/insn_trans/trans_rva.c.inc | 8 ++++++
target/riscv/insn_trans/trans_rvzicfiss.c.inc | 6 +++++
target/riscv/internals.h | 1 +
target/riscv/translate.c | 25 +++++++++++++++++++
6 files changed, 56 insertions(+)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 6da94c417c..3ad220a9fe 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -615,6 +615,19 @@ FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
/* zicfiss needs a TB flag so that correct TB is located based on tb flags */
FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
+/*
+ * zicfiss shadow stack is special memory on which regular stores aren't
+ * allowed but shadow stack stores are allowed. Shadow stack stores can
+ * happen as `sspush` or `ssamoswap` instructions. `sspush` implicitly
+ * takes shadow stack address from CSR_SSP. But `ssamoswap` takes address
+ * from encoded input register and it will be used by supervisor software
+ * to access (read/write) user shadow stack for setting up rt_frame during
+ * signal delivery. Supervisor software will do so by setting SUM=1. Thus
+ * a TB flag is needed if SUM was 1 during TB generation to correctly
+ * reflect memory permissions to access shadow stack user memory from
+ * supervisor mode.
+ */
+FIELD(TB_FLAGS, SUM, 31, 1)
This is already encoded into the mmu_idx as MMUIdx_S_SUM.
This is where I need some help / suggestion and clarifications.
`riscv_env_mmu_index` is the which does mode --> mmu index translation and
that's
where `MMUIdx_S_SUM` is set.
Although above function assumes following things
-- Only loads ands stores are supposed to do read and write.
-- Translates env/priv --> mmu index
In case of shadow stack, we need to hold following true:
Shadow stack are not writeable via regular stores but are allowed to be
readable.
Shadow stack are writeable only via shadow stack instruction.
Shadow stack instructions can't operate on non-shadow stack memory.
This let me to create a new mmu index (as you saw in patches). This mmu index
is only
setup by shadow stack instruction and thus has to be known at translation time
All good so far.
There is no way of telling in `riscv_env_mmu_index` about whether mmu index is
requested
for regular load/store or some other instruction (in this case shadow stack
instruction).
If that is available then I think I can use `riscv_env_mmu_index`.
What you miss is that the result of riscv_env_mmu_index is stored
ctx->mem_idx
Yeah, that's right. thanks.
So that takes care of U, S, SUM, M, VS, VU, etc. All you need at
this point is to or in your shadow stack bit:
ctx->mem_idx | MMU_IDX_SS_ACCESS
(Perhaps SS_WRITE is a better name, since read access is never prohibited?)
Yeah MMU_IDX_SS_WRITE is probably more self-explainatory.
Note that you can do this without ifdefs -- user-only will happily
accept and ignore the mmu index. Also note that user-only will *not*
be able to restrict access to the shadow stack pages in the way the
spec describes. We rely on the host mmu for read/write permission for
user-only. For now -- changing that is a long term goal.
Ok, didn't know user-only will ignore.
I'll remove ifdef and simply do
ctx->mem_idx =| MMU_IDX_SS_WRITE
Question:
I see that `riscv_env_mmu_index` could be called from a bunch of places in (like
`accel/tcg/ldst_common.c.inc` as well. Does it exclude loads, stores which
calculate mmu
indexes during translation (like shadow stack load, stores) ?
It means you cannot use the legacy interfaces for the shadow stack.
The current interfaces:
* cpu_ld{sign}{size}{end}_mmuidx_ra(env, ptr, mmu_idx, retaddr)
* cpu_ld{sign}{size}{end}_mmu(env, ptr, oi, retaddr)
...
* cpu_st{size}{end}_mmuidx_ra(env, ptr, val, mmu_idx, retaddr)
* cpu_st{size}{end}_mmu(env, ptr, val, oi, retaddr)
take the mmu_idx as a parameter.
But as it happens, the shadow stack instructions are simple enough to
implement all inline, so you won't need to call the out-of-line
load/store functions from cpu helpers.
Thanks for explaining this.
r~
[PATCH v3 18/20] target/riscv: add trace-hooks for each case of sw-check exception, Deepak Gupta, 2024/08/06