Re: [Qemu-devel] [Qemu-riscv] [PULL 10/34] RISC-V: Fix a PMP check with the correct access size

[Jonathan Behrens]

I think this patch is slightly incorrect. If the PMP region is valid for
the size of the access, but not the rest of the page then a few lines down
in this function the entire page should not be placed into the TLB. Instead
only the portion of the page that passed the access check should be
included. To give an example of where this goes wrong, in the code below
access to address 0x90000008 should always fail due to PMP rules, but if
the TLB has already been primed by loading the (allowed) address 0x90000000
then no fault will be triggered. Notably, this code also executes
improperly without the patch because the first `ld` instruction traps when
it shouldn't.

  li t0, 0x0000000024000000 // region[0]: 0x90000000..0x90000007
  csrw pmpaddr0, t0

  li t0, 0x00000000240001FF // region[1]: 0x90000000..0x90000fff
  csrw pmpaddr1, t0

  li t0, 0x1F0000000000989F // cfg[0] = LXRW, cfg[1] = L
  csrw pmpcfg0, t0

  sfence.vma

  li t0, 0x90000000
  ld s0, 0(t0)
  ld s1, 8(t0) // NO TRAP: address is incorrectly in TLB!

  sfence.vma
  ld s1, 8(t0) // TRAP: TLB has been flushed!

I think that the proper fix would be to first do a PMP check for the full
PAGE_SIZE and execute normally if it passes. Then in the event the full
page fails, there could be a more granular PMP check with only the accessed
region inserted as an entry in the TLB.

Unrelated question: should I be sending "Reviewed By" lines if I read
through patches that seem reasonable? Or there some formal process I'd have
to go through first to be approved?

Jonathan

On Thu, Jun 27, 2019 at 11:43 AM Palmer Dabbelt <address@hidden> wrote:

> From: Hesham Almatary <address@hidden>
>
> The PMP check should be of the memory access size rather
> than TARGET_PAGE_SIZE.
>
> Signed-off-by: Hesham Almatary <address@hidden>
> Reviewed-by: Alistair Francis <address@hidden>
> Signed-off-by: Palmer Dabbelt <address@hidden>
> ---
>  target/riscv/cpu_helper.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 66be83210f11..e1b079e69c60 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -452,8 +452,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
>
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>          (ret == TRANSLATE_SUCCESS) &&
> -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type,
> -        mode)) {
> +        !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
>          ret = TRANSLATE_PMP_FAIL;
>      }
>      if (ret == TRANSLATE_PMP_FAIL) {
> --
> 2.21.0
>
>
>