[PATCH 1/2] target/riscv: fix access permission checks for CSR

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [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 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 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 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

* 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 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 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

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).