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

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

[Deepak Gupta]

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 "22.2.1. Shadow Stack Pointer
(ssp) CSR access contr" in the priv spec.

Fixes: 8205bc127a83 ("target/riscv: introduce ssp and enabling controls
for zicfiss". Thanks to Adam Zabrocki for bringing this to attention.

Reported-by: Adam Zabrocki <azabrocki@nvidia.com>
Signed-off-by: Deepak Gupta <debug@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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

[PATCH v2 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode

[Deepak Gupta]

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) and effective priv is
less than M then `ssamoswap` must result in an illegal instruction
exception. However if effective priv is M, then `ssamoswap` results in
store/AMO access fault. See Section "22.2.3. Shadow Stack Memory
Protection" of priv spec.

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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
index e3ebc4977c..b0096adcd0 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,6 +84,11 @@ 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);
+    REQUIRE_ZICFISS(ctx);
+    if (ctx->priv == PRV_M) {
+        generate_exception(ctx, RISCV_EXCP_STORE_AMO_ACCESS_FAULT);
+    }
+
     if (!ctx->bcfi_enabled) {
         return false;
     }
@@ -97,6 +109,11 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a)
 {
     REQUIRE_64BIT(ctx);
     REQUIRE_A_OR_ZAAMO(ctx);
+    REQUIRE_ZICFISS(ctx);
+    if (ctx->priv == PRV_M) {
+        generate_exception(ctx, RISCV_EXCP_STORE_AMO_ACCESS_FAULT);
+    }
+
     if (!ctx->bcfi_enabled) {
         return false;
     }
-- 
2.34.1

Re: [PATCH v2 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode

[Alistair Francis]

On Thu, Mar 6, 2025 at 4:47 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) and effective priv is
> less than M then `ssamoswap` must result in an illegal instruction
> exception. However if effective priv is M, then `ssamoswap` results in
> store/AMO access fault. See Section "22.2.3. Shadow Stack Memory
> Protection" of priv spec.
>
> Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions")
>
> Reported-by: Ved Shanbhogue <ved@rivosinc.com>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/insn_trans/trans_rvzicfiss.c.inc | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
> index e3ebc4977c..b0096adcd0 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,6 +84,11 @@ 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);
> +    REQUIRE_ZICFISS(ctx);
> +    if (ctx->priv == PRV_M) {
> +        generate_exception(ctx, RISCV_EXCP_STORE_AMO_ACCESS_FAULT);
> +    }
> +
>      if (!ctx->bcfi_enabled) {
>          return false;
>      }
> @@ -97,6 +109,11 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a)
>  {
>      REQUIRE_64BIT(ctx);
>      REQUIRE_A_OR_ZAAMO(ctx);
> +    REQUIRE_ZICFISS(ctx);
> +    if (ctx->priv == PRV_M) {
> +        generate_exception(ctx, RISCV_EXCP_STORE_AMO_ACCESS_FAULT);
> +    }
> +
>      if (!ctx->bcfi_enabled) {
>          return false;
>      }
> --
> 2.34.1
>
>

Re: [PATCH v2 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode

[Alistair Francis]

On Thu, Mar 6, 2025 at 4:47 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) and effective priv is
> less than M then `ssamoswap` must result in an illegal instruction
> exception. However if effective priv is M, then `ssamoswap` results in
> store/AMO access fault. See Section "22.2.3. Shadow Stack Memory
> Protection" of priv spec.
>
> Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions")
>
> Reported-by: Ved Shanbhogue <ved@rivosinc.com>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>

Thanks!

Applied to riscv-to-apply.next

Please use a cover letter for a multi-patch series in the future

Alistair

> ---
>  target/riscv/insn_trans/trans_rvzicfiss.c.inc | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
> index e3ebc4977c..b0096adcd0 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,6 +84,11 @@ 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);
> +    REQUIRE_ZICFISS(ctx);
> +    if (ctx->priv == PRV_M) {
> +        generate_exception(ctx, RISCV_EXCP_STORE_AMO_ACCESS_FAULT);
> +    }
> +
>      if (!ctx->bcfi_enabled) {
>          return false;
>      }
> @@ -97,6 +109,11 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a)
>  {
>      REQUIRE_64BIT(ctx);
>      REQUIRE_A_OR_ZAAMO(ctx);
> +    REQUIRE_ZICFISS(ctx);
> +    if (ctx->priv == PRV_M) {
> +        generate_exception(ctx, RISCV_EXCP_STORE_AMO_ACCESS_FAULT);
> +    }
> +
>      if (!ctx->bcfi_enabled) {
>          return false;
>      }
> --
> 2.34.1
>
>

Re: [PATCH v2 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode

[Deepak Gupta]

On Thu, Mar 6, 2025 at 6:05 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Mar 6, 2025 at 4:47 PM Deepak Gupta <debug@rivosinc.com> wrote:
> >
> > Commit f06bfe3dc38c ("target/riscv: implement zicfiss instructions") adds
> > `ssamoswap` instruction. `ssamoswap` takes the code-point from existing
<... snipped...>
> > Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions")
> >
> > Reported-by: Ved Shanbhogue <ved@rivosinc.com>
> > Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>
> Thanks!
>
> Applied to riscv-to-apply.next

Thanks a lot.

>
> Please use a cover letter for a multi-patch series in the future
>

Noted.

Re: [PATCH v2 1/2] target/riscv: fix access permission checks for CSR_SSP

[Michael Tokarev]

On 06.03.2025 09:46, Deepak Gupta 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 "22.2.1. Shadow Stack Pointer
> (ssp) CSR access contr" in the priv spec.
> 
> Fixes: 8205bc127a83 ("target/riscv: introduce ssp and enabling controls
> for zicfiss". Thanks to Adam Zabrocki for bringing this to attention.

Is this patchset (including "[2/2] target/riscv: fixes a bug against
`ssamoswap` behavior in M-mode") applicable for stable qemu series?

Thanks,

/mjt

Re: [PATCH v2 1/2] target/riscv: fix access permission checks for CSR_SSP

[Alistair Francis]

On Sat, Mar 22, 2025 at 4:03 AM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> On 06.03.2025 09:46, Deepak Gupta 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 "22.2.1. Shadow Stack Pointer
> > (ssp) CSR access contr" in the priv spec.
> >
> > Fixes: 8205bc127a83 ("target/riscv: introduce ssp and enabling controls
> > for zicfiss". Thanks to Adam Zabrocki for bringing this to attention.
>
> Is this patchset (including "[2/2] target/riscv: fixes a bug against
> `ssamoswap` behavior in M-mode") applicable for stable qemu series?

Yes, if it applies it is (it probably won't apply to older stables as
it's a new-ish feature).

Alistair

>
> Thanks,
>
> /mjt
>