Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Jonathan Behrens]

The remaining checks are not sufficient. If you look at the bottom of
csr.c, you'll see that for most of the M-mode CSRs the predicate is set to
"any" which unconditionally allows access regardless of privilege mode. The
S-mode CSR predicates similarly only check that supervisor mode exists, but
not that the processor is current running in that mode. I do agree with you
that using the register address to determine the required privilege mode is
a little strange, but RISC-V is designed so that bits 8 and 9 of the CSR
address encode that information: 0=U-mode, 1=S-mode, 2=HS-mode, 3=M-mode.

I also tested by booting RVirt <https://github.com/mit-pdos/RVirt> with a
Linux guest and found that this patch caused it to instantly crash because
guest CSR accesses weren't intercepted.

Jonathan

On Fri, Jul 26, 2019 at 4:28 PM Alistair Francis <address@hidden>
wrote:

> On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <address@hidden>
> wrote:
> >
> > Unless I'm missing something, this is the only place that QEMU checks
> the privilege level for read and writes to CSRs. The exact computation used
> here won't work with the hypervisor extension, but we also can't just get
> rid of privilege checking entirely...
>
> The csr_ops struct contains a checker function, so there are still
> some checks occurring. I haven't done negative testing on this patch,
> but the current check doesn't seem to make any sense so it should be
> removed. We can separately discuss adding more checks but this current
> way base don CSR address just seems strange.
>
> Alistair
>
> >
> > Jonathan
> >
> > On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <
> address@hidden> wrote:
> >>
> >> The privledge check based on the CSR address mask 0x300 doesn't work
> >> when using Hypervisor extensions so remove the check
> >>
> >> Signed-off-by: Alistair Francis <address@hidden>
> >> ---
> >>  target/riscv/csr.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index e0d4586760..af3b762c8b 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> >>
> >>      /* check privileges and return -1 if check fails */
> >>  #if !defined(CONFIG_USER_ONLY)
> >> -    int csr_priv = get_field(csrno, 0x300);
> >>      int read_only = get_field(csrno, 0xC00) == 3;
> >> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
> >> +    if (write_mask && read_only) {
> >>          return -1;
> >>      }
> >>  #endif
> >> --
> >> 2.22.0
> >>
> >>
>