[PATCH for 5.0 v1 0/2] RISC-V: Fix Hypervisor guest user space

[PATCH for 5.0 v1 0/2] RISC-V: Fix Hypervisor guest user space

[Alistair Francis]

This series fixes two bugs in the RISC-V two stage lookup
implementation. This fixes the Hypervisor userspace failing to start.

Alistair Francis (2):
  riscv: Don't use stage-2 PTE lookup protection flags
  riscv: AND stage-1 and stage-2 protection flags

 target/riscv/cpu_helper.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.26.0

[PATCH for 5.0 v1 1/2] riscv: Don't use stage-2 PTE lookup protection flags

[Alistair Francis]

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;
-- 
2.26.0

Re: [PATCH for 5.0 v1 1/2] riscv: Don't use stage-2 PTE lookup protection flags

[Richard Henderson]

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~

Re: [PATCH for 5.0 v1 1/2] riscv: Don't use stage-2 PTE lookup protection flags

[Alistair Francis]

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~

Re: [PATCH for 5.0 v1 1/2] riscv: Don't use stage-2 PTE lookup protection flags

[Richard Henderson]

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~

[PATCH for 5.0 v1 2/2] riscv: AND stage-1 and stage-2 protection flags

[Alistair Francis]

Take the result of stage-1 and stage-2 page table walks and AND the two
protection flags together. This way we require both to set permissions
instead of just stage-2.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu_helper.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f36d184b7b..50e13a064f 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -707,7 +707,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
 #ifndef CONFIG_USER_ONLY
     vaddr im_address;
     hwaddr pa = 0;
-    int prot;
+    int prot, prot2;
     bool pmp_violation = false;
     bool m_mode_two_stage = false;
     bool hs_mode_two_stage = false;
@@ -757,13 +757,15 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
             /* Second stage lookup */
             im_address = pa;
 
-            ret = get_physical_address(env, &pa, &prot, im_address,
+            ret = get_physical_address(env, &pa, &prot2, im_address,
                                        access_type, mmu_idx, false, true);
 
             qemu_log_mask(CPU_LOG_MMU,
                     "%s 2nd-stage address=%" VADDR_PRIx " ret %d physical "
                     TARGET_FMT_plx " prot %d\n",
-                    __func__, im_address, ret, pa, prot);
+                    __func__, im_address, ret, pa, prot2);
+
+            prot &= prot2;
 
             if (riscv_feature(env, RISCV_FEATURE_PMP) &&
                 (ret == TRANSLATE_SUCCESS) &&
-- 
2.26.0

Re: [PATCH for 5.0 v1 2/2] riscv: AND stage-1 and stage-2 protection flags

[Richard Henderson]

On 3/26/20 3:44 PM, Alistair Francis wrote:
> Take the result of stage-1 and stage-2 page table walks and AND the two
> protection flags together. This way we require both to set permissions
> instead of just stage-2.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu_helper.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f36d184b7b..50e13a064f 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -707,7 +707,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>  #ifndef CONFIG_USER_ONLY
>      vaddr im_address;
>      hwaddr pa = 0;
> -    int prot;
> +    int prot, prot2;
>      bool pmp_violation = false;
>      bool m_mode_two_stage = false;
>      bool hs_mode_two_stage = false;
> @@ -757,13 +757,15 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>              /* Second stage lookup */
>              im_address = pa;
>  
> -            ret = get_physical_address(env, &pa, &prot, im_address,
> +            ret = get_physical_address(env, &pa, &prot2, im_address,
>                                         access_type, mmu_idx, false, true);
>  
>              qemu_log_mask(CPU_LOG_MMU,
>                      "%s 2nd-stage address=%" VADDR_PRIx " ret %d physical "
>                      TARGET_FMT_plx " prot %d\n",
> -                    __func__, im_address, ret, pa, prot);
> +                    __func__, im_address, ret, pa, prot2);
> +
> +            prot &= prot2;
>  
>              if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>                  (ret == TRANSLATE_SUCCESS) &&
> 

Whee!  Yes, I think this is what you've been looking for.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH for 5.0 v1 2/2] riscv: AND stage-1 and stage-2 protection flags

[Alistair Francis]

On Thu, Mar 26, 2020 at 4:32 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/26/20 3:44 PM, Alistair Francis wrote:
> > Take the result of stage-1 and stage-2 page table walks and AND the two
> > protection flags together. This way we require both to set permissions
> > instead of just stage-2.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  target/riscv/cpu_helper.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index f36d184b7b..50e13a064f 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -707,7 +707,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >  #ifndef CONFIG_USER_ONLY
> >      vaddr im_address;
> >      hwaddr pa = 0;
> > -    int prot;
> > +    int prot, prot2;
> >      bool pmp_violation = false;
> >      bool m_mode_two_stage = false;
> >      bool hs_mode_two_stage = false;
> > @@ -757,13 +757,15 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >              /* Second stage lookup */
> >              im_address = pa;
> >
> > -            ret = get_physical_address(env, &pa, &prot, im_address,
> > +            ret = get_physical_address(env, &pa, &prot2, im_address,
> >                                         access_type, mmu_idx, false, true);
> >
> >              qemu_log_mask(CPU_LOG_MMU,
> >                      "%s 2nd-stage address=%" VADDR_PRIx " ret %d physical "
> >                      TARGET_FMT_plx " prot %d\n",
> > -                    __func__, im_address, ret, pa, prot);
> > +                    __func__, im_address, ret, pa, prot2);
> > +
> > +            prot &= prot2;
> >
> >              if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> >                  (ret == TRANSLATE_SUCCESS) &&
> >
>
> Whee!  Yes, I think this is what you've been looking for.

Yep!

I actually tried this ages ago, but it didn't work without the first
path so it never fixed the problem.

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks

Alistair

>
>
> r~

Re: [PATCH for 5.0 v1 0/2] RISC-V: Fix Hypervisor guest user space

[Palmer Dabbelt]

On Thu, 26 Mar 2020 15:44:04 PDT (-0700), Alistair Francis wrote:
> This series fixes two bugs in the RISC-V two stage lookup
> implementation. This fixes the Hypervisor userspace failing to start.
>
> Alistair Francis (2):
>   riscv: Don't use stage-2 PTE lookup protection flags
>   riscv: AND stage-1 and stage-2 protection flags
>
>  target/riscv/cpu_helper.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Thanks, these are in the queue.

Re: [PATCH for 5.0 v1 0/2] RISC-V: Fix Hypervisor guest user space

[Anup Patel]

Hi Palmer,

On Fri, Mar 27, 2020 at 5:30 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Thu, 26 Mar 2020 15:44:04 PDT (-0700), Alistair Francis wrote:
> > This series fixes two bugs in the RISC-V two stage lookup
> > implementation. This fixes the Hypervisor userspace failing to start.
> >
> > Alistair Francis (2):
> >   riscv: Don't use stage-2 PTE lookup protection flags
> >   riscv: AND stage-1 and stage-2 protection flags
> >
> >  target/riscv/cpu_helper.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
>
> Thanks, these are in the queue.
>

I have tested this patch series on latest QEMU master without
"target/riscv: Don't set write permissions on dirty PTEs" workaround
patch. It works fine now.

Tested-by: Anup Patel <anup@brainfault.org>

Please drop the work-around patch  "target/riscv: Don't set write
permissions on dirty PTEs" from your for-next.

Regards,
Anup

Re: [PATCH for 5.0 v1 0/2] RISC-V: Fix Hypervisor guest user space

[Alistair Francis]

On Thu, Mar 26, 2020 at 3:51 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> This series fixes two bugs in the RISC-V two stage lookup
> implementation. This fixes the Hypervisor userspace failing to start.
>
> Alistair Francis (2):
>   riscv: Don't use stage-2 PTE lookup protection flags
>   riscv: AND stage-1 and stage-2 protection flags

Applied to the RISC-V tree for 5.1

Alistair

>
>  target/riscv/cpu_helper.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> --
> 2.26.0
>