| Message ID | 20250218025446.2452254-1-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: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 > >
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 >> >>
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 > >> > >>
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 >> >> >> >>
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)
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(+)