[PATCH] target/arm: Allow user-mode code to write CPSR.E via MSR

[PATCH] target/arm: Allow user-mode code to write CPSR.E via MSR

[Peter Maydell]

Using the MSR instruction to write to CPSR.E is deprecated, but it is
required to work from any mode including unprivileged code.  We were
incorrectly forbidding usermode code from writing it because
CPSR_USER did not include the CPSR_E bit.

We use CPSR_USER in only three places:
 * as the mask of what to allow userspace MSR to write to CPSR
 * when deciding what bits a linux-user signal-return should be
   able to write from the sigcontext structure
 * in target_user_copy_regs() when we set up the initial
   registers for the linux-user process

In the first two cases not being able to update CPSR.E is a
bug, and in the third case it doesn't matter because CPSR.E
is always 0 there. So we can fix both bugs by adding CPSR_E
to CPSR_EXEC.

(The recommended way to change CPSR.E is to use the 'SETEND'
instruction, which we do correctly allow from usermode code.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Bug reported on IRC. Quick-and-dirty test case at:
 https://people.linaro.org/~peter.maydell/msr-setend.c

 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5d995368d4f..677584e5da0 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1230,7 +1230,7 @@ void pmu_init(ARMCPU *cpu);
 #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT | CPSR_Q \
     | CPSR_NZCV)
 /* Bits writable in user mode.  */
-#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE)
+#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE | CPSR_E)
 /* Execution state bits.  MRS read as zero, MSR writes ignored.  */
 #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J | CPSR_IL)
 
-- 
2.20.1

Re: [PATCH] target/arm: Allow user-mode code to write CPSR.E via MSR

[Richard Henderson]

On 5/15/20 11:50 AM, Peter Maydell wrote:
> Using the MSR instruction to write to CPSR.E is deprecated, but it is
> required to work from any mode including unprivileged code.  We were
> incorrectly forbidding usermode code from writing it because
> CPSR_USER did not include the CPSR_E bit.
> 
> We use CPSR_USER in only three places:
>  * as the mask of what to allow userspace MSR to write to CPSR
>  * when deciding what bits a linux-user signal-return should be
>    able to write from the sigcontext structure
>  * in target_user_copy_regs() when we set up the initial
>    registers for the linux-user process
> 
> In the first two cases not being able to update CPSR.E is a
> bug, and in the third case it doesn't matter because CPSR.E
> is always 0 there. So we can fix both bugs by adding CPSR_E
> to CPSR_EXEC.

Wrong variable in description here.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> 
> (The recommended way to change CPSR.E is to use the 'SETEND'
> instruction, which we do correctly allow from usermode code.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Bug reported on IRC. Quick-and-dirty test case at:
>  https://people.linaro.org/~peter.maydell/msr-setend.c
> 
>  target/arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 5d995368d4f..677584e5da0 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1230,7 +1230,7 @@ void pmu_init(ARMCPU *cpu);
>  #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT | CPSR_Q \
>      | CPSR_NZCV)
>  /* Bits writable in user mode.  */
> -#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE)
> +#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE | CPSR_E)
>  /* Execution state bits.  MRS read as zero, MSR writes ignored.  */
>  #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J | CPSR_IL)
>  
> 

Re: [PATCH] target/arm: Allow user-mode code to write CPSR.E via MSR

[Philippe Mathieu-Daudé]

On 5/15/20 11:26 PM, Richard Henderson wrote:
> On 5/15/20 11:50 AM, Peter Maydell wrote:
>> Using the MSR instruction to write to CPSR.E is deprecated, but it is
>> required to work from any mode including unprivileged code.  We were
>> incorrectly forbidding usermode code from writing it because
>> CPSR_USER did not include the CPSR_E bit.
>>
>> We use CPSR_USER in only three places:
>>   * as the mask of what to allow userspace MSR to write to CPSR
>>   * when deciding what bits a linux-user signal-return should be
>>     able to write from the sigcontext structure
>>   * in target_user_copy_regs() when we set up the initial
>>     registers for the linux-user process
>>
>> In the first two cases not being able to update CPSR.E is a
>> bug, and in the third case it doesn't matter because CPSR.E
>> is always 0 there. So we can fix both bugs by adding CPSR_E
>> to CPSR_EXEC.
> 
> Wrong variable in description here.

Indeed CPSR_EXEC -> CPSR_USER typo.

> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~
> 
>>
>> (The recommended way to change CPSR.E is to use the 'SETEND'
>> instruction, which we do correctly allow from usermode code.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Bug reported on IRC.

Similar to commit a1ecb4381829d7:

Reported-by: Randy Yates <yates@ieee.org>

> Quick-and-dirty test case at:
>>   https://people.linaro.org/~peter.maydell/msr-setend.c
>>
>>   target/arm/cpu.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 5d995368d4f..677584e5da0 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -1230,7 +1230,7 @@ void pmu_init(ARMCPU *cpu);
>>   #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT | CPSR_Q \
>>       | CPSR_NZCV)
>>   /* Bits writable in user mode.  */
>> -#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE)
>> +#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE | CPSR_E)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>   /* Execution state bits.  MRS read as zero, MSR writes ignored.  */
>>   #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J | CPSR_IL)
>>   
>>
> 
> 

Re: [PATCH] target/arm: Allow user-mode code to write CPSR.E via MSR

[Randy Yates]

Thank you Philippe and the QEMU team!

--Randy

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 5/15/20 11:26 PM, Richard Henderson wrote:
>> On 5/15/20 11:50 AM, Peter Maydell wrote:
>>> Using the MSR instruction to write to CPSR.E is deprecated, but it is
>>> required to work from any mode including unprivileged code.  We were
>>> incorrectly forbidding usermode code from writing it because
>>> CPSR_USER did not include the CPSR_E bit.
>>>
>>> We use CPSR_USER in only three places:
>>>   * as the mask of what to allow userspace MSR to write to CPSR
>>>   * when deciding what bits a linux-user signal-return should be
>>>     able to write from the sigcontext structure
>>>   * in target_user_copy_regs() when we set up the initial
>>>     registers for the linux-user process
>>>
>>> In the first two cases not being able to update CPSR.E is a
>>> bug, and in the third case it doesn't matter because CPSR.E
>>> is always 0 there. So we can fix both bugs by adding CPSR_E
>>> to CPSR_EXEC.
>>
>> Wrong variable in description here.
>
> Indeed CPSR_EXEC -> CPSR_USER typo.
>
>>
>> Otherwise,
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>>
>> r~
>>
>>>
>>> (The recommended way to change CPSR.E is to use the 'SETEND'
>>> instruction, which we do correctly allow from usermode code.)
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> Bug reported on IRC.
>
> Similar to commit a1ecb4381829d7:
>
> Reported-by: Randy Yates <yates@ieee.org>
>
>> Quick-and-dirty test case at:
>>>   https://people.linaro.org/~peter.maydell/msr-setend.c
>>>
>>>   target/arm/cpu.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 5d995368d4f..677584e5da0 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -1230,7 +1230,7 @@ void pmu_init(ARMCPU *cpu);
>>>   #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT | CPSR_Q \
>>>       | CPSR_NZCV)
>>>   /* Bits writable in user mode.  */
>>> -#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE)
>>> +#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE | CPSR_E)
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>>>   /* Execution state bits.  MRS read as zero, MSR writes ignored.  */
>>>   #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J | CPSR_IL)
>>>   
>>>
>>
>>
>

-- 
Randy Yates, DSP/Embedded Firmware Developer
Digital Signal Labs
http://www.digitalsignallabs.com

[PATCH] target/arm: Allow user-mode code to write CPSR.E via MSR

[Peter Maydell]

Using the MSR instruction to write to CPSR.E is deprecated, but it is
required to work from any mode including unprivileged code.  We were
incorrectly forbidding usermode code from writing it because
CPSR_USER did not include the CPSR_E bit.

We use CPSR_USER in only three places:
 * as the mask of what to allow userspace MSR to write to CPSR
 * when deciding what bits a linux-user signal-return should be
   able to write from the sigcontext structure
 * in target_user_copy_regs() when we set up the initial
   registers for the linux-user process

In the first two cases not being able to update CPSR.E is a
bug, and in the third case it doesn't matter because CPSR.E
is always 0 there. So we can fix both bugs by adding CPSR_E
to CPSR_EXEC.

(The recommended way to change CPSR.E is to use the 'SETEND'
instruction, which we do correctly allow from usermode code.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Bug reported on IRC. Quick-and-dirty test case at:
 https://people.linaro.org/~peter.maydell/msr-setend.c

 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5d995368d4f..677584e5da0 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1230,7 +1230,7 @@ void pmu_init(ARMCPU *cpu);
 #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT | CPSR_Q \
     | CPSR_NZCV)
 /* Bits writable in user mode.  */
-#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE)
+#define CPSR_USER (CPSR_NZCV | CPSR_Q | CPSR_GE | CPSR_E)
 /* Execution state bits.  MRS read as zero, MSR writes ignored.  */
 #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J | CPSR_IL)
 
-- 
2.20.1