| Message ID | b415f5b51e09418760b95e5c73fad5e68b97f173.1564080680.git.alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Hypervisor prep work part 2 | expand |
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... Jonathan On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com> 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 <alistair.francis@wdc.com> > --- > 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 > > >
On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com> 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 <alistair.francis@wdc.com> 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 <alistair.francis@wdc.com> >> --- >> 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 >> >>
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 <alistair23@gmail.com> wrote: > On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com> > 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 < > alistair.francis@wdc.com> 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 <alistair.francis@wdc.com> > >> --- > >> 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 > >> > >> >
On Fri, Jul 26, 2019 at 2:00 PM Jonathan Behrens <fintelia@gmail.com> wrote: > > 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. Ah, I didn't realise that was guaranteed. I will drop this patch from this series and update it in my Hypervisor series. Alistair > > I also tested by booting 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 <alistair23@gmail.com> wrote: >> >> On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com> 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 <alistair.francis@wdc.com> 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 <alistair.francis@wdc.com> >> >> --- >> >> 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 >> >> >> >>
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
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 <alistair.francis@wdc.com> --- target/riscv/csr.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)