|
From: | Philippe Mathieu-Daudé |
Subject: | 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 |
Date: | Fri, 28 Jul 2017 14:01:10 -0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |