* [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP
@ 2025-02-18 2:54 Deepak Gupta
2025-02-18 2:54 ` [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode Deepak Gupta
2025-03-06 5:20 ` [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP Alistair Francis
0 siblings, 2 replies; 10+ messages in thread
From: Deepak Gupta @ 2025-02-18 2:54 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bmeng.cn, liwei1518, dbarboza,
zhiwei_liu, Deepak Gupta, Adam Zabrocki
Commit:8205bc1 ("target/riscv: introduce ssp and enabling controls for
zicfiss") introduced CSR_SSP but it mis-interpreted the spec on access
to CSR_SSP in M-mode. Gated to CSR_SSP is not gated via `xSSE`. But
rather rules clearly specified in section "2.2.4. Shadow Stack Pointer"
of `zicfiss` specification. Thanks to Adam Zabrocki for bringing this
to attention.
Fixes: 8205bc127a83 ("target/riscv: introduce ssp and enabling controls
for zicfiss"
Reported-by: Adam Zabrocki <azabrocki@nvidia.com>
Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
target/riscv/csr.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index afb7544f07..75c661d2a1 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -191,6 +191,11 @@ static RISCVException cfi_ss(CPURISCVState *env, int csrno)
return RISCV_EXCP_ILLEGAL_INST;
}
+ /* If ext implemented, M-mode always have access to SSP CSR */
+ if (env->priv == PRV_M) {
+ return RISCV_EXCP_NONE;
+ }
+
/* if bcfi not active for current env, access to csr is illegal */
if (!cpu_get_bcfien(env)) {
#if !defined(CONFIG_USER_ONLY)
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode
2025-02-18 2:54 [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP Deepak Gupta
@ 2025-02-18 2:54 ` Deepak Gupta
2025-03-06 5:29 ` Alistair Francis
2025-03-06 5:20 ` [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP Alistair Francis
1 sibling, 1 reply; 10+ messages in thread
From: Deepak Gupta @ 2025-02-18 2:54 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bmeng.cn, liwei1518, dbarboza,
zhiwei_liu, Deepak Gupta, Ved Shanbhogue
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(-)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP
2025-02-18 2:54 [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP Deepak Gupta
2025-02-18 2:54 ` [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode Deepak Gupta
@ 2025-03-06 5:20 ` Alistair Francis
2025-03-06 6:12 ` Deepak Gupta
1 sibling, 1 reply; 10+ messages in thread
From: Alistair Francis @ 2025-03-06 5:20 UTC (permalink / raw)
To: Deepak Gupta
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
liwei1518, dbarboza, zhiwei_liu, Adam Zabrocki
On Tue, Feb 18, 2025 at 12:56 PM Deepak Gupta <debug@rivosinc.com> wrote:
>
> Commit:8205bc1 ("target/riscv: introduce ssp and enabling controls for
> zicfiss") introduced CSR_SSP but it mis-interpreted the spec on access
> to CSR_SSP in M-mode. Gated to CSR_SSP is not gated via `xSSE`. But
> rather rules clearly specified in section "2.2.4. Shadow Stack Pointer"
Do you mean "22.2.1. Shadow Stack Pointer (ssp) CSR access contr" in
the priv spec?
> of `zicfiss` specification. Thanks to Adam Zabrocki for bringing this
> to attention.
The thanks should probably be below the line
>
> Fixes: 8205bc127a83 ("target/riscv: introduce ssp and enabling controls
> for zicfiss"
>
> Reported-by: Adam Zabrocki <azabrocki@nvidia.com>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
The actual change looks good:
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/csr.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index afb7544f07..75c661d2a1 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -191,6 +191,11 @@ static RISCVException cfi_ss(CPURISCVState *env, int csrno)
> return RISCV_EXCP_ILLEGAL_INST;
> }
>
> + /* If ext implemented, M-mode always have access to SSP CSR */
> + if (env->priv == PRV_M) {
> + return RISCV_EXCP_NONE;
> + }
> +
> /* if bcfi not active for current env, access to csr is illegal */
> if (!cpu_get_bcfien(env)) {
> #if !defined(CONFIG_USER_ONLY)
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode
2025-02-18 2:54 ` [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode Deepak Gupta
@ 2025-03-06 5:29 ` Alistair Francis
2025-03-06 6:13 ` Deepak Gupta
0 siblings, 1 reply; 10+ messages in thread
From: Alistair Francis @ 2025-03-06 5:29 UTC (permalink / raw)
To: Deepak Gupta
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
liwei1518, dbarboza, zhiwei_liu, Ved Shanbhogue
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP
2025-03-06 5:20 ` [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP Alistair Francis
@ 2025-03-06 6:12 ` Deepak Gupta
2025-03-06 6:20 ` Alistair Francis
0 siblings, 1 reply; 10+ messages in thread
From: Deepak Gupta @ 2025-03-06 6:12 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
liwei1518, dbarboza, zhiwei_liu, Adam Zabrocki
On Thu, Mar 06, 2025 at 03:20:55PM +1000, Alistair Francis wrote:
>On Tue, Feb 18, 2025 at 12:56 PM Deepak Gupta <debug@rivosinc.com> wrote:
>>
>> Commit:8205bc1 ("target/riscv: introduce ssp and enabling controls for
>> zicfiss") introduced CSR_SSP but it mis-interpreted the spec on access
>> to CSR_SSP in M-mode. Gated to CSR_SSP is not gated via `xSSE`. But
>> rather rules clearly specified in section "2.2.4. Shadow Stack Pointer"
>
>Do you mean "22.2.1. Shadow Stack Pointer (ssp) CSR access contr" in
>the priv spec?
No I meant 2.2.4 of zicfiss specification. Section 22.2.1 of priv spec
says same.
>
>> of `zicfiss` specification. Thanks to Adam Zabrocki for bringing this
>> to attention.
>
>The thanks should probably be below the line
Sure
>
>>
>> Fixes: 8205bc127a83 ("target/riscv: introduce ssp and enabling controls
>> for zicfiss"
>>
>> Reported-by: Adam Zabrocki <azabrocki@nvidia.com>
>> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>
>The actual change looks good:
>
>Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
>Alistair
>
>> ---
>> target/riscv/csr.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index afb7544f07..75c661d2a1 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -191,6 +191,11 @@ static RISCVException cfi_ss(CPURISCVState *env, int csrno)
>> return RISCV_EXCP_ILLEGAL_INST;
>> }
>>
>> + /* If ext implemented, M-mode always have access to SSP CSR */
>> + if (env->priv == PRV_M) {
>> + return RISCV_EXCP_NONE;
>> + }
>> +
>> /* if bcfi not active for current env, access to csr is illegal */
>> if (!cpu_get_bcfien(env)) {
>> #if !defined(CONFIG_USER_ONLY)
>> --
>> 2.34.1
>>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode
2025-03-06 5:29 ` Alistair Francis
@ 2025-03-06 6:13 ` Deepak Gupta
2025-03-06 6:22 ` Alistair Francis
0 siblings, 1 reply; 10+ messages in thread
From: Deepak Gupta @ 2025-03-06 6:13 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
liwei1518, dbarboza, zhiwei_liu, Ved Shanbhogue
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
>>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP
2025-03-06 6:12 ` Deepak Gupta
@ 2025-03-06 6:20 ` Alistair Francis
2025-03-06 6:22 ` Deepak Gupta
0 siblings, 1 reply; 10+ messages in thread
From: Alistair Francis @ 2025-03-06 6:20 UTC (permalink / raw)
To: Deepak Gupta
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
liwei1518, dbarboza, zhiwei_liu, Adam Zabrocki
On Thu, Mar 6, 2025 at 4:12 PM Deepak Gupta <debug@rivosinc.com> wrote:
>
> On Thu, Mar 06, 2025 at 03:20:55PM +1000, Alistair Francis wrote:
> >On Tue, Feb 18, 2025 at 12:56 PM Deepak Gupta <debug@rivosinc.com> wrote:
> >>
> >> Commit:8205bc1 ("target/riscv: introduce ssp and enabling controls for
> >> zicfiss") introduced CSR_SSP but it mis-interpreted the spec on access
> >> to CSR_SSP in M-mode. Gated to CSR_SSP is not gated via `xSSE`. But
> >> rather rules clearly specified in section "2.2.4. Shadow Stack Pointer"
> >
> >Do you mean "22.2.1. Shadow Stack Pointer (ssp) CSR access contr" in
> >the priv spec?
>
> No I meant 2.2.4 of zicfiss specification. Section 22.2.1 of priv spec
> says same.
I meant that it's now just in the priv spec, the zicfiss spec is no
longer maintained so we should just reference the priv spec
Alistair
>
> >
> >> of `zicfiss` specification. Thanks to Adam Zabrocki for bringing this
> >> to attention.
> >
> >The thanks should probably be below the line
>
> Sure
>
> >
> >>
> >> Fixes: 8205bc127a83 ("target/riscv: introduce ssp and enabling controls
> >> for zicfiss"
> >>
> >> Reported-by: Adam Zabrocki <azabrocki@nvidia.com>
> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> >
> >The actual change looks good:
> >
> >Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >
> >Alistair
> >
> >> ---
> >> target/riscv/csr.c | 5 +++++
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index afb7544f07..75c661d2a1 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -191,6 +191,11 @@ static RISCVException cfi_ss(CPURISCVState *env, int csrno)
> >> return RISCV_EXCP_ILLEGAL_INST;
> >> }
> >>
> >> + /* If ext implemented, M-mode always have access to SSP CSR */
> >> + if (env->priv == PRV_M) {
> >> + return RISCV_EXCP_NONE;
> >> + }
> >> +
> >> /* if bcfi not active for current env, access to csr is illegal */
> >> if (!cpu_get_bcfien(env)) {
> >> #if !defined(CONFIG_USER_ONLY)
> >> --
> >> 2.34.1
> >>
> >>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP
2025-03-06 6:20 ` Alistair Francis
@ 2025-03-06 6:22 ` Deepak Gupta
0 siblings, 0 replies; 10+ messages in thread
From: Deepak Gupta @ 2025-03-06 6:22 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
liwei1518, dbarboza, zhiwei_liu, Adam Zabrocki
On Thu, Mar 06, 2025 at 04:20:56PM +1000, Alistair Francis wrote:
>On Thu, Mar 6, 2025 at 4:12 PM Deepak Gupta <debug@rivosinc.com> wrote:
>>
>> On Thu, Mar 06, 2025 at 03:20:55PM +1000, Alistair Francis wrote:
>> >On Tue, Feb 18, 2025 at 12:56 PM Deepak Gupta <debug@rivosinc.com> wrote:
>> >>
>> >> Commit:8205bc1 ("target/riscv: introduce ssp and enabling controls for
>> >> zicfiss") introduced CSR_SSP but it mis-interpreted the spec on access
>> >> to CSR_SSP in M-mode. Gated to CSR_SSP is not gated via `xSSE`. But
>> >> rather rules clearly specified in section "2.2.4. Shadow Stack Pointer"
>> >
>> >Do you mean "22.2.1. Shadow Stack Pointer (ssp) CSR access contr" in
>> >the priv spec?
>>
>> No I meant 2.2.4 of zicfiss specification. Section 22.2.1 of priv spec
>> says same.
>
>I meant that it's now just in the priv spec, the zicfiss spec is no
>longer maintained so we should just reference the priv spec
Got it.
>
>Alistair
>
>>
>> >
>> >> of `zicfiss` specification. Thanks to Adam Zabrocki for bringing this
>> >> to attention.
>> >
>> >The thanks should probably be below the line
>>
>> Sure
>>
>> >
>> >>
>> >> Fixes: 8205bc127a83 ("target/riscv: introduce ssp and enabling controls
>> >> for zicfiss"
>> >>
>> >> Reported-by: Adam Zabrocki <azabrocki@nvidia.com>
>> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>> >
>> >The actual change looks good:
>> >
>> >Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> >
>> >Alistair
>> >
>> >> ---
>> >> target/riscv/csr.c | 5 +++++
>> >> 1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> >> index afb7544f07..75c661d2a1 100644
>> >> --- a/target/riscv/csr.c
>> >> +++ b/target/riscv/csr.c
>> >> @@ -191,6 +191,11 @@ static RISCVException cfi_ss(CPURISCVState *env, int csrno)
>> >> return RISCV_EXCP_ILLEGAL_INST;
>> >> }
>> >>
>> >> + /* If ext implemented, M-mode always have access to SSP CSR */
>> >> + if (env->priv == PRV_M) {
>> >> + return RISCV_EXCP_NONE;
>> >> + }
>> >> +
>> >> /* if bcfi not active for current env, access to csr is illegal */
>> >> if (!cpu_get_bcfien(env)) {
>> >> #if !defined(CONFIG_USER_ONLY)
>> >> --
>> >> 2.34.1
>> >>
>> >>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode
2025-03-06 6:13 ` Deepak Gupta
@ 2025-03-06 6:22 ` Alistair Francis
2025-03-06 6:30 ` Deepak Gupta
0 siblings, 1 reply; 10+ messages in thread
From: Alistair Francis @ 2025-03-06 6:22 UTC (permalink / raw)
To: Deepak Gupta
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
liwei1518, dbarboza, zhiwei_liu, Ved Shanbhogue
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
> >>
> >>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode
2025-03-06 6:22 ` Alistair Francis
@ 2025-03-06 6:30 ` Deepak Gupta
0 siblings, 0 replies; 10+ messages in thread
From: Deepak Gupta @ 2025-03-06 6:30 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
liwei1518, dbarboza, zhiwei_liu, Ved Shanbhogue
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
>> >>
>> >>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-03-06 6:31 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-18 2:54 [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP Deepak Gupta
2025-02-18 2:54 ` [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode Deepak Gupta
2025-03-06 5:29 ` Alistair Francis
2025-03-06 6:13 ` Deepak Gupta
2025-03-06 6:22 ` Alistair Francis
2025-03-06 6:30 ` Deepak Gupta
2025-03-06 5:20 ` [PATCH 1/2] target/riscv: fix access permission checks for CSR_SSP Alistair Francis
2025-03-06 6:12 ` Deepak Gupta
2025-03-06 6:20 ` Alistair Francis
2025-03-06 6:22 ` Deepak Gupta
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).