Re: [PATCH] target/riscv/pmp: guard against PMP ranges with a negative size

[Alistair Francis]

On Thu, Jun 16, 2022 at 7:12 AM Nicolas Pitre <nico@fluxnic.net> wrote:
>
> For a TOR entry to match, the stard address must be lower than the end
> address. Normally this is always the case, but correct code might still
> run into the following scenario:
>
> Initial state:
>
>         pmpaddr3 = 0x2000       pmp3cfg = OFF
>         pmpaddr4 = 0x3000       pmp4cfg = TOR
>
> Execution:
>
>         1. write 0x40ff to pmpaddr3
>         2. write 0x32ff to pmpaddr4

Hey, thanks for that patch!

So, at this point we have a PMP region enforcing

0x40ff <= addr < 0x32ff

which is going to be wrong as that isn't valid. But this is also
partially a guest bug. If a guest sets invalid PMP regions we should
be throwing exceptions (if the PMP region is enabled and enforced in
the current mode)

>         3. set pmp3cfg to NAPOT with a read-modify-write on pmpcfg0
>         4. set pmp4cfg to NAPOT with a read-modify-write on pmpcfg1
>
> When (2) is emulated, a call to pmp_update_rule() creates a negative
> range for pmp4 as pmp4cfg is still set to TOR. And when (3) is emulated,

I don't see where the negative comes from. From what I can tell we
should just set `sa` and `ea` to the values specified by the guest.

> a call to tlb_flush() is performed, causing pmp_get_tlb_size() to return
> a very creatively large TLB size for pmp4. This, in turn, may result in

Hmm.. pmp_get_tlb_size() assumes pmp_ea > pmp_sa. Maybe we should add
a check in there?

> accesses to non-existent/unitialized memory regions and a fault, so that
> (4) ends up never being executed.
>
> This is in m-mode with MPRV unset, meaning that unlocked PMP entries
> should have no effect. Therefore such a behavior based on PMP content
> is very unexpected.

Ok, this part is a QEMU bug. If we aren't enforcing PMP regions we
should not be throwing PMP errors.

get_physical_address_pmp() should give us full permissions though in
this case, so I don't see where the failure is. Can you include some
more details?

>
> Make sure no negative PMP range can be created, whether explicitly by
> the emulated code or implicitly like the above.
>
> Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 151da3fa08..ea2b67d947 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -167,6 +167,9 @@ void pmp_update_rule_addr(CPURISCVState *env, uint32_t 
> pmp_index)
>      case PMP_AMATCH_TOR:
>          sa = prev_addr << 2; /* shift up from [xx:0] to [xx+2:2] */
>          ea = (this_addr << 2) - 1u;
> +        if (sa > ea) {
> +            sa = ea = 0u;
> +        }

This doesn't seem right though.

Image if a guest sets the values you have above, then jumps to user
mode. The spec doesn't seem to say what should happen with invalid PMP
ranges, but I feel like we should throw exceptions instead of just
ignoring the config.

Alistair

>          break;
>
>      case PMP_AMATCH_NA4:
>