diff mbox series

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

Message ID 1501153150-19984-3-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series M profile MPU bugfixes | expand

Commit Message

Peter Maydell July 27, 2017, 10:59 a.m. UTC 
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 <peter.maydell@linaro.org>

---
 target/arm/helper.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

-- 
2.7.4

Comments

Philippe Mathieu-Daudé July 27, 2017, 11:59 p.m. UTC | #1 
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 <peter.maydell@linaro.org>

> ---

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

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

> +            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;

>               }

>           }

> 

     }
     *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;

What do you think?

Regards,

Phil.
Peter Maydell July 28, 2017, 8:51 a.m. UTC | #2 
On 28 July 2017 at 00:59, Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <peter.maydell@linaro.org>

>> ---

>>   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 :-(

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

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

>>               }

>>           }

>>

>     }

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

thanks
-- PMM
Philippe Mathieu-Daudé July 28, 2017, 5:01 p.m. UTC | #3 
On 07/28/2017 05:51 AM, Peter Maydell wrote:
> On 28 July 2017 at 00:59, Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <peter.maydell@linaro.org>

>>> ---

>>>    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é <f4bug@amsat.org>


> 

> thanks

> -- PMM

>
diff mbox series

Patch

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;
 }
 
+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);
+            uint32_t xn = extract32(env->pmsav7.dracr[n], 12, 1);
+
+            if (is_system_region(env, address)) {
+                /* System space is always execute never */
+                xn = 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;
             }
         }