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

[Nicolas Pitre]

On Thu, 16 Jun 2022, Alistair Francis wrote:

> 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)

The guest cannot change all the relevant PMP registers for a single PMP 
entry atomically. This is why you normally update the PMP configuration 
in m-mode with MPRV unset for invalid but transient PMP states to have 
no adverse effects.

> >         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.

Eventually that is the case i.e. when the type is changed from TOR to 
NAPOT in (4), at which point everything is well and sane. But that can't 
happen atomically as I said. Yet QEMU does interpret the intermediate 
state although it shouldn't.

> > 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?

It is more efficient to prevent sa > ea in the first place as 
pmp_get_tlb_size() is called way more often than pmp_update_rule_addr().

Let's not forget that those sa/ea are simplified representations of the 
hardware and not what the guest can see. The original register content 
written by the guest is preserved.

> > 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.

Right. But this is not a PMP error. It is QEMU screwing up its internal 
state. It does even segfault when this condition occur if -d cpu is 
provided.

> get_physical_address_pmp() should give us full permissions though in
> this case, so I don't see where the failure is.

And it does. That's not where the problem is.

> Can you include some more details?

As I said, in the middle of updating the pmpcfg registers in (3), 
tlb_flush() is invoked which causes the core to revalidate its memory 
TLB cache down through pmp_get_tlb_size() where bad answers are 
returned. This causes the core to assume a different memory area which 
doesn't exist and the next memory access ends up calling 
unassigned_mem_accepts() where false is unconditionally returned. And I 
suspect this is also the result of a buffer overflow as the address 
logged in memory_region_access_valid() is sometimes complete garbage 
too, and the occasional QEMU segfault.

> > 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.

The spec says that a given memory access has to be >= the bottom 
boundary and < the top boundary for a PMP entry to match. So an invalid 
PMP entry should simply not match. That's what a hardware comparator 
would do: it would just not match. Furthermore, you cannot tell if the 
guest is in the middle of updating its PMP configuration which are split 
across several registers as illustrated above, therefore transient 
invalid states are unavoidable.


Nicolas