Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 2/5] target/arm: Don't allow guest to make System space executable for M profile

[Philippe Mathieu-Daudé]

On 07/28/2017 05:51 AM, Peter Maydell wrote:
> On 28 July 2017 at 00:59, Philippe Mathieu-Daudé <address@hidden> wrote:
> > Hi Peter,
> >
> > On 07/27/2017 07:59 AM, Peter Maydell wrote:
> > > For an M profile v7PMSA, the system space (0xe0000000 - 0xffffffff) can
> > > never be executable, even if the guest tries to set the MPU registers
> > > up that way. Enforce this restriction.
> > >
> > > Signed-off-by: Peter Maydell <address@hidden>
> > > ---
> > >    target/arm/helper.c | 16 +++++++++++++++-
> > >    1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > > index ceef225..169c361 100644
> > > --- a/target/arm/helper.c
> > > +++ b/target/arm/helper.c
> > > @@ -8251,6 +8251,14 @@ static inline bool is_ppb_region(CPUARMState *env,
> > > uint32_t address)
> > >            extract32(address, 20, 12) == 0xe00;
> > >    }
> >
> >
> > I wonder if these should renamed pmsav7_is_ppb_region() and
> > pmsav7_is_system_region().
>
> Yeah, perhaps better; I'm never quite sure how much disambiguation
> to put in to file-local function names. Maybe m_is_ppb_region()?
> PPB and system region are M profile concepts, not PMSAv7 ones.
> That doesn't seem any clearer than where we started though :-(

m_is_ppb_region() isn't bad.

> > > +static inline bool is_system_region(CPUARMState *env, uint32_t address)
> > > +{
> > > +    /* True if address is in the M profile system region
> > > +     * 0xe0000000 - 0xffffffff
> > > +     */
> > > +    return arm_feature(env, ARM_FEATURE_M) && extract32(address, 29, 3)
> > > == 0x7;
> > > +}
> > > +
> > >    static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
> > >                                     int access_type, ARMMMUIdx mmu_idx,
> > >                                     hwaddr *phys_ptr, int *prot, uint32_t
> > > *fsr)
> > > @@ -8354,6 +8362,12 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
> > > uint32_t address,
> > >                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> > >            } else { /* a MPU hit! */
> > >                uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
> >
> >
> > Maybe names access_perms/execute_never are easier to read..
>
> Following existing practice in the LPAE code, we use the
> field names that the architecture spec uses.

I see, but below xn has an helpful comment /* execute never */ that 
eases code review, maybe add both comment on declaration.

> > > +            uint32_t xn = extract32(env->pmsav7.dracr[n], 12, 1);
> > > +
> >
> >
> > clear MemManage exceptions:
> >
> >                 *fsr &= ~0xff;
>
> > > +            if (is_system_region(env, address)) {
> > > +                /* System space is always execute never */
> > > +                xn = 1;
> >
> >
> >                 } else {
> >                     xn = extract32(env->pmsav7.dracr[n], 12, 1);
> >
> > > +            }
> > >                  if (is_user) { /* User mode AP bit decoding */
> > >                    switch (ap) {
> > > @@ -8394,7 +8408,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
> > > uint32_t address,
> > >                }
> > >                  /* execute never */
> > > -            if (env->pmsav7.dracr[n] & (1 << 12)) {
> > > +            if (xn) {
> > >                    *prot &= ~PAGE_EXEC;
> >
> >
> > and here we now can set eXecuteNever violation:
> >
> >                      *fsr |= R_V7M_CFSR_IACCVIOL_MASK;
>
> No, *fsr is not an M profile CFSR, it's an A/R profile short
> descriptor format fault status value (because on R profile
> that's what it will be used as, and M profile is using the
> same MPU handling code here). We do the conversion in
> arm_v7m_cpu_do_interrupt(), where we look at the exception_index
> and the exception.fsr to identify what CFSR bits to set.

Ok I missed that, thank for correcting me.

> > >                }
> > >            }
> >      }
> >      *fsr = 0x00d; /* Permission fault */
> >
> > I don't understand this mask, I don't have bit [2] defined in my datashit,
> > maybe it was expected to turn on exception Entry/Return which I have defined
> > as bits 4 and 3 respectively, so I'd rather see here:
> >
> >      *fsr |= R_V7M_CFSR_MUNSTKERR_MASK | R_V7M_CFSR_MSTKERR_MASK;
>
> See above, *fsr isn't a v7m CFSR.

Yes, 0x00d is Permission fault using short-descriptor translation.

So:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> thanks
> -- PMM