| Message ID | 20250218025446.2452254-2-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] target/riscv: fix access permission checks for CSR_SSP | expand |
On Tue, Feb 18, 2025 at 12:57 PM Deepak Gupta <debug@rivosinc.com> wrote: > > Commit f06bfe3dc38c ("target/riscv: implement zicfiss instructions") adds > `ssamoswap` instruction. `ssamoswap` takes the code-point from existing > reserved encoding (and not a zimop like other shadow stack instructions). > If shadow stack is not enabled (via xenvcfg.SSE), then `ssamoswap` must > result in an illegal instruction exception. However there is a slightly > modified behavior for M-mode. > > Shadow stack are not available in M-mode and all shadow stack instructions > in M-mode exhibit zimop behavior. However, `ssamoswap` can still succeed > if MPRV=1 and MPP is non-zero (see section 2.7 of zicfiss specification). > This patch corrects that behavior for `ssamoswap`. Section "22.2.3. Shadow Stack Memory Protection " of the latest priv spec [1] seems to say: "When the effective privilege mode is M, any memory access by an SSAMOSWAP.W/D instruction will result in a store/AMO access-fault exception." 1: https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-isa-release-9cfaf37-2025-03-06 Alistair > > Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions") > > Reported-by: Ved Shanbhogue <ved@rivosinc.com> > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > --- > target/riscv/insn_trans/trans_rvzicfiss.c.inc | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc > index e3ebc4977c..ec016cd70f 100644 > --- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc > +++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc > @@ -15,6 +15,13 @@ > * You should have received a copy of the GNU General Public License along with > * this program. If not, see <http://www.gnu.org/licenses/>. > */ > + > + #define REQUIRE_ZICFISS(ctx) do { \ > + if (!ctx->cfg_ptr->ext_zicfiss) { \ > + return false; \ > + } \ > +} while (0) > + > static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a) > { > if (!ctx->bcfi_enabled) { > @@ -77,7 +84,8 @@ static bool trans_ssrdp(DisasContext *ctx, arg_ssrdp *a) > static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a) > { > REQUIRE_A_OR_ZAAMO(ctx); > - if (!ctx->bcfi_enabled) { > + REQUIRE_ZICFISS(ctx); > + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { > return false; > } > > @@ -97,7 +105,8 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a) > { > REQUIRE_64BIT(ctx); > REQUIRE_A_OR_ZAAMO(ctx); > - if (!ctx->bcfi_enabled) { > + REQUIRE_ZICFISS(ctx); > + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { > return false; > } > > -- > 2.34.1 > >
On Thu, Mar 06, 2025 at 03:29:00PM +1000, Alistair Francis wrote: >On Tue, Feb 18, 2025 at 12:57 PM Deepak Gupta <debug@rivosinc.com> wrote: >> >> Commit f06bfe3dc38c ("target/riscv: implement zicfiss instructions") adds >> `ssamoswap` instruction. `ssamoswap` takes the code-point from existing >> reserved encoding (and not a zimop like other shadow stack instructions). >> If shadow stack is not enabled (via xenvcfg.SSE), then `ssamoswap` must >> result in an illegal instruction exception. However there is a slightly >> modified behavior for M-mode. >> >> Shadow stack are not available in M-mode and all shadow stack instructions >> in M-mode exhibit zimop behavior. However, `ssamoswap` can still succeed >> if MPRV=1 and MPP is non-zero (see section 2.7 of zicfiss specification). >> This patch corrects that behavior for `ssamoswap`. > >Section "22.2.3. Shadow Stack Memory Protection " of the latest priv >spec [1] seems to say: "When the effective privilege mode is M, any >memory access by an SSAMOSWAP.W/D >instruction will result in a store/AMO access-fault exception." Hmm I didn't look at priv spec. Let me fix this one. > >1: https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-isa-release-9cfaf37-2025-03-06 > >Alistair > >> >> Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions") >> >> Reported-by: Ved Shanbhogue <ved@rivosinc.com> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com> >> --- >> target/riscv/insn_trans/trans_rvzicfiss.c.inc | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc >> index e3ebc4977c..ec016cd70f 100644 >> --- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc >> +++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc >> @@ -15,6 +15,13 @@ >> * You should have received a copy of the GNU General Public License along with >> * this program. If not, see <http://www.gnu.org/licenses/>. >> */ >> + >> + #define REQUIRE_ZICFISS(ctx) do { \ >> + if (!ctx->cfg_ptr->ext_zicfiss) { \ >> + return false; \ >> + } \ >> +} while (0) >> + >> static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a) >> { >> if (!ctx->bcfi_enabled) { >> @@ -77,7 +84,8 @@ static bool trans_ssrdp(DisasContext *ctx, arg_ssrdp *a) >> static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a) >> { >> REQUIRE_A_OR_ZAAMO(ctx); >> - if (!ctx->bcfi_enabled) { >> + REQUIRE_ZICFISS(ctx); >> + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { >> return false; >> } >> >> @@ -97,7 +105,8 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a) >> { >> REQUIRE_64BIT(ctx); >> REQUIRE_A_OR_ZAAMO(ctx); >> - if (!ctx->bcfi_enabled) { >> + REQUIRE_ZICFISS(ctx); >> + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { >> return false; >> } >> >> -- >> 2.34.1 >> >>
On Thu, Mar 6, 2025 at 4:13 PM Deepak Gupta <debug@rivosinc.com> wrote: > > On Thu, Mar 06, 2025 at 03:29:00PM +1000, Alistair Francis wrote: > >On Tue, Feb 18, 2025 at 12:57 PM Deepak Gupta <debug@rivosinc.com> wrote: > >> > >> Commit f06bfe3dc38c ("target/riscv: implement zicfiss instructions") adds > >> `ssamoswap` instruction. `ssamoswap` takes the code-point from existing > >> reserved encoding (and not a zimop like other shadow stack instructions). > >> If shadow stack is not enabled (via xenvcfg.SSE), then `ssamoswap` must > >> result in an illegal instruction exception. However there is a slightly > >> modified behavior for M-mode. > >> > >> Shadow stack are not available in M-mode and all shadow stack instructions > >> in M-mode exhibit zimop behavior. However, `ssamoswap` can still succeed > >> if MPRV=1 and MPP is non-zero (see section 2.7 of zicfiss specification). > >> This patch corrects that behavior for `ssamoswap`. > > > >Section "22.2.3. Shadow Stack Memory Protection " of the latest priv > >spec [1] seems to say: "When the effective privilege mode is M, any > >memory access by an SSAMOSWAP.W/D > >instruction will result in a store/AMO access-fault exception." > > Hmm I didn't look at priv spec. Let me fix this one. I hope the two don't conflict, that will be a nightmare Alistair > > > > >1: https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-isa-release-9cfaf37-2025-03-06 > > > >Alistair > > > >> > >> Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions") > >> > >> Reported-by: Ved Shanbhogue <ved@rivosinc.com> > >> Signed-off-by: Deepak Gupta <debug@rivosinc.com> > >> --- > >> target/riscv/insn_trans/trans_rvzicfiss.c.inc | 13 +++++++++++-- > >> 1 file changed, 11 insertions(+), 2 deletions(-) > >> > >> diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc > >> index e3ebc4977c..ec016cd70f 100644 > >> --- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc > >> +++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc > >> @@ -15,6 +15,13 @@ > >> * You should have received a copy of the GNU General Public License along with > >> * this program. If not, see <http://www.gnu.org/licenses/>. > >> */ > >> + > >> + #define REQUIRE_ZICFISS(ctx) do { \ > >> + if (!ctx->cfg_ptr->ext_zicfiss) { \ > >> + return false; \ > >> + } \ > >> +} while (0) > >> + > >> static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a) > >> { > >> if (!ctx->bcfi_enabled) { > >> @@ -77,7 +84,8 @@ static bool trans_ssrdp(DisasContext *ctx, arg_ssrdp *a) > >> static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a) > >> { > >> REQUIRE_A_OR_ZAAMO(ctx); > >> - if (!ctx->bcfi_enabled) { > >> + REQUIRE_ZICFISS(ctx); > >> + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { > >> return false; > >> } > >> > >> @@ -97,7 +105,8 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a) > >> { > >> REQUIRE_64BIT(ctx); > >> REQUIRE_A_OR_ZAAMO(ctx); > >> - if (!ctx->bcfi_enabled) { > >> + REQUIRE_ZICFISS(ctx); > >> + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { > >> return false; > >> } > >> > >> -- > >> 2.34.1 > >> > >>
On Thu, Mar 06, 2025 at 04:22:52PM +1000, Alistair Francis wrote: >On Thu, Mar 6, 2025 at 4:13 PM Deepak Gupta <debug@rivosinc.com> wrote: >> >> On Thu, Mar 06, 2025 at 03:29:00PM +1000, Alistair Francis wrote: >> >On Tue, Feb 18, 2025 at 12:57 PM Deepak Gupta <debug@rivosinc.com> wrote: >> >> >> >> Commit f06bfe3dc38c ("target/riscv: implement zicfiss instructions") adds >> >> `ssamoswap` instruction. `ssamoswap` takes the code-point from existing >> >> reserved encoding (and not a zimop like other shadow stack instructions). >> >> If shadow stack is not enabled (via xenvcfg.SSE), then `ssamoswap` must >> >> result in an illegal instruction exception. However there is a slightly >> >> modified behavior for M-mode. >> >> >> >> Shadow stack are not available in M-mode and all shadow stack instructions >> >> in M-mode exhibit zimop behavior. However, `ssamoswap` can still succeed >> >> if MPRV=1 and MPP is non-zero (see section 2.7 of zicfiss specification). >> >> This patch corrects that behavior for `ssamoswap`. >> > >> >Section "22.2.3. Shadow Stack Memory Protection " of the latest priv >> >spec [1] seems to say: "When the effective privilege mode is M, any >> >memory access by an SSAMOSWAP.W/D >> >instruction will result in a store/AMO access-fault exception." >> >> Hmm I didn't look at priv spec. Let me fix this one. > >I hope the two don't conflict, that will be a nightmare No they don't conflict. Last "else" block below basically means that it should be store/AMO access fault because there is no shadow stack page. """ if privilege_mode != M && menvcfg.SSE == 0 raise illegal-instruction exception if S-mode not implemented raise illegal-instruction exception else if privilege_mode == U && senvcfg.SSE == 0 raise illegal-instruction exception else if privilege_mode == VS && henvcfg.SSE == 0 raise virtual instruction exception else if privilege_mode == VU && senvcfg.SSE == 0 raise virtual instruction exception else temp[31:0] = mem[X(rs1)] X(rd) = SignExtend(temp[31:0]) mem[X(rs1)] = X(rs2)[31:0] endif """ > >Alistair > >> >> > >> >1: https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-isa-release-9cfaf37-2025-03-06 >> > >> >Alistair >> > >> >> >> >> Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions") >> >> >> >> Reported-by: Ved Shanbhogue <ved@rivosinc.com> >> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com> >> >> --- >> >> target/riscv/insn_trans/trans_rvzicfiss.c.inc | 13 +++++++++++-- >> >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc >> >> index e3ebc4977c..ec016cd70f 100644 >> >> --- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc >> >> +++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc >> >> @@ -15,6 +15,13 @@ >> >> * You should have received a copy of the GNU General Public License along with >> >> * this program. If not, see <http://www.gnu.org/licenses/>. >> >> */ >> >> + >> >> + #define REQUIRE_ZICFISS(ctx) do { \ >> >> + if (!ctx->cfg_ptr->ext_zicfiss) { \ >> >> + return false; \ >> >> + } \ >> >> +} while (0) >> >> + >> >> static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a) >> >> { >> >> if (!ctx->bcfi_enabled) { >> >> @@ -77,7 +84,8 @@ static bool trans_ssrdp(DisasContext *ctx, arg_ssrdp *a) >> >> static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a) >> >> { >> >> REQUIRE_A_OR_ZAAMO(ctx); >> >> - if (!ctx->bcfi_enabled) { >> >> + REQUIRE_ZICFISS(ctx); >> >> + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { >> >> return false; >> >> } >> >> >> >> @@ -97,7 +105,8 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a) >> >> { >> >> REQUIRE_64BIT(ctx); >> >> REQUIRE_A_OR_ZAAMO(ctx); >> >> - if (!ctx->bcfi_enabled) { >> >> + REQUIRE_ZICFISS(ctx); >> >> + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { >> >> return false; >> >> } >> >> >> >> -- >> >> 2.34.1 >> >> >> >>
diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc index e3ebc4977c..ec016cd70f 100644 --- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc +++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc @@ -15,6 +15,13 @@ * You should have received a copy of the GNU General Public License along with * this program. If not, see <http://www.gnu.org/licenses/>. */ + + #define REQUIRE_ZICFISS(ctx) do { \ + if (!ctx->cfg_ptr->ext_zicfiss) { \ + return false; \ + } \ +} while (0) + static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a) { if (!ctx->bcfi_enabled) { @@ -77,7 +84,8 @@ static bool trans_ssrdp(DisasContext *ctx, arg_ssrdp *a) static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a) { REQUIRE_A_OR_ZAAMO(ctx); - if (!ctx->bcfi_enabled) { + REQUIRE_ZICFISS(ctx); + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { return false; } @@ -97,7 +105,8 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a) { REQUIRE_64BIT(ctx); REQUIRE_A_OR_ZAAMO(ctx); - if (!ctx->bcfi_enabled) { + REQUIRE_ZICFISS(ctx); + if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) { return false; }
Commit f06bfe3dc38c ("target/riscv: implement zicfiss instructions") adds `ssamoswap` instruction. `ssamoswap` takes the code-point from existing reserved encoding (and not a zimop like other shadow stack instructions). If shadow stack is not enabled (via xenvcfg.SSE), then `ssamoswap` must result in an illegal instruction exception. However there is a slightly modified behavior for M-mode. Shadow stack are not available in M-mode and all shadow stack instructions in M-mode exhibit zimop behavior. However, `ssamoswap` can still succeed if MPRV=1 and MPP is non-zero (see section 2.7 of zicfiss specification). This patch corrects that behavior for `ssamoswap`. Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions") Reported-by: Ved Shanbhogue <ved@rivosinc.com> Signed-off-by: Deepak Gupta <debug@rivosinc.com> --- target/riscv/insn_trans/trans_rvzicfiss.c.inc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)