| Message ID | 931db85d6890ed4bc2b527fd1011197cd28299aa.1585262586.git.alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Fix Hypervisor guest user space | expand |
On 3/26/20 3:44 PM, Alistair Francis wrote: > When doing the fist of a two stage lookup (Hypervisor extensions) don't > set the current protection flags from the second stage lookup of the > base address PTE. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > target/riscv/cpu_helper.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index d3ba9efb02..f36d184b7b 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -452,10 +452,11 @@ restart: > hwaddr pte_addr; > > if (two_stage && first_stage) { > + int vbase_prot; > hwaddr vbase; > > /* Do the second stage translation on the base PTE address. */ > - get_physical_address(env, &vbase, prot, base, access_type, > + get_physical_address(env, &vbase, &vbase_prot, base, access_type, > mmu_idx, false, true); > > pte_addr = vbase + idx * ptesize; > Certainly stage2 pte lookup has nothing to do with the original lookup, so using a new variable for prot is correct. So as far as this minimal patch, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> However, this bit of code doesn't look right: (1) Similarly, what has the original access_type got to do with the PTE lookup? Seems like this should be MMU_DATA_LOAD always. (2) Why is the get_physical_address return value ignored? On failure, surely this should be some sort of PTE lookup failure. (3) Do we need to validate vbase_prot for write before updating the PTE for Access or Dirty? That seems like a loop-hole to allow silent modification of hypervisor read-only memory. I do wonder if it might be easier to manage all of this by using additional TLBs to handle the stage2 and physical address spaces. That's probably too invasive for this stage of development though. r~
On Thu, Mar 26, 2020 at 4:50 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 3/26/20 3:44 PM, Alistair Francis wrote: > > When doing the fist of a two stage lookup (Hypervisor extensions) don't > > set the current protection flags from the second stage lookup of the > > base address PTE. > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > --- > > target/riscv/cpu_helper.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > index d3ba9efb02..f36d184b7b 100644 > > --- a/target/riscv/cpu_helper.c > > +++ b/target/riscv/cpu_helper.c > > @@ -452,10 +452,11 @@ restart: > > hwaddr pte_addr; > > > > if (two_stage && first_stage) { > > + int vbase_prot; > > hwaddr vbase; > > > > /* Do the second stage translation on the base PTE address. */ > > - get_physical_address(env, &vbase, prot, base, access_type, > > + get_physical_address(env, &vbase, &vbase_prot, base, access_type, > > mmu_idx, false, true); > > > > pte_addr = vbase + idx * ptesize; > > > > Certainly stage2 pte lookup has nothing to do with the original lookup, so > using a new variable for prot is correct. > > So as far as this minimal patch, > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > However, this bit of code doesn't look right: Thanks for the comments here. Coming back to this after a while. > > (1) Similarly, what has the original access_type got to do with the PTE lookup? > Seems like this should be MMU_DATA_LOAD always. Fixed in master now > > (2) Why is the get_physical_address return value ignored? On failure, surely > this should be some sort of PTE lookup failure. Also fixed in master now > > (3) Do we need to validate vbase_prot for write before updating the PTE for > Access or Dirty? That seems like a loop-hole to allow silent modification of > hypervisor read-only memory. That's a good point. Updating the accessed bit seems correct to me as we did access it and that doesn't then provide write permissions. Updating the dirty bit would provide write permissions, but we would only change the dirty bit on a store and vbase_prot is now always a load. If the PTE was already dirty then we might incorrectly provide write access though. > > I do wonder if it might be easier to manage all of this by using additional > TLBs to handle the stage2 and physical address spaces. That's probably too > invasive for this stage of development though. Do you mean change riscv_cpu_mmu_index() to take into account virtulisation and have more then the current 3 (M, S and U) MMU indexes? Alistair > > > r~
On 6/25/20 12:02 PM, Alistair Francis wrote: >> (3) Do we need to validate vbase_prot for write before updating the PTE for >> Access or Dirty? That seems like a loop-hole to allow silent modification of >> hypervisor read-only memory. > > That's a good point. > > Updating the accessed bit seems correct to me as we did access it and > that doesn't then provide write permissions. I guess my first question is: Does the stage2 hypervisor pte provide read-only memory? If not, all of this is moot. However, if it does, consider: (1) The guest os creates a stage1 page table with a leaf table within the read-only memory. This is obviously hokey. (2) The guest os accesses a virtual address that utilizes the aforementioned PTE, the hardware (qemu) updates the accessed bit. (3) The read-only page has now been modified. Oops. >> I do wonder if it might be easier to manage all of this by using additional >> TLBs to handle the stage2 and physical address spaces. That's probably too >> invasive for this stage of development though. > > Do you mean change riscv_cpu_mmu_index() to take into account > virtulisation and have more then the current 3 (M, S and U) MMU > indexes? I had been thinking that you might be able to use some form of mmu-indexed load/lookup instead of address_space_ldq. Which would require 1 mmuidx that is physically mapped (same as M?) and another that uses only the hypervisor's second stage lookup. r~
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index d3ba9efb02..f36d184b7b 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -452,10 +452,11 @@ restart: hwaddr pte_addr; if (two_stage && first_stage) { + int vbase_prot; hwaddr vbase; /* Do the second stage translation on the base PTE address. */ - get_physical_address(env, &vbase, prot, base, access_type, + get_physical_address(env, &vbase, &vbase_prot, base, access_type, mmu_idx, false, true); pte_addr = vbase + idx * ptesize;
When doing the fist of a two stage lookup (Hypervisor extensions) don't set the current protection flags from the second stage lookup of the base address PTE. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- target/riscv/cpu_helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)