[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/riscv/pmp: guard against PMP ranges with a negative s
From: |
Nicolas Pitre |
Subject: |
Re: [PATCH] target/riscv/pmp: guard against PMP ranges with a negative size |
Date: |
Thu, 16 Jun 2022 08:20:31 -0400 (EDT) |
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