[Qemu-devel] target/arm: Fix abort on exception return from AArch64 to AArch32

[Qemu-devel] target/arm: Fix abort on exception return from AArch64 to AArch32

[Jaroslaw Pelczar]

Problem manifests itself when we handle the following sequence:

1. 64-bit Secure EL3 returns to 32-bit Secure EL1
2. 32-bit Secure EL1 performs SMC call to 64-bit Secure EL3
3. 64-bit Secure EL3 performs return ERET to 32-bit Secure EL1]
4. 32-bit Secure EL1 receives prefetch abort

If CPU's env->pc is not set to the same value as env->regs[15],
during ERET the simulator will try to fetch instruction from EL3's
virtual address inside 32-bit Secure EL1 virtual address space.
This will cause Prefetch Abort in 32-bit Secure EL1.

Problem occurs because of the following code generation scheme:

1. disas_uncond_b_reg will decode ERET
2. gen_helper_exception_return(cpu_env) will generate thunk to
   helper_exception_return
3. s->is_jmp is set to DISAS_JUMP
4. gen_intermediate_code_a64 will see dc->is_jmp == DISAS_JUMP
5. tcg_gen_lookup_and_goto_ptr(cpu_pc) will be called which trigger
   access cpu->pc to fetch next opcode

At this point cpu->pc will have EL3's PC value.

Signed-off-by: Jaroslaw Pelczar <j.pelczar@samsung.com>
---
 target/arm/op_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 2a85666..8222c19 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -1027,6 +1027,9 @@ void HELPER(exception_return)(CPUARMState *env)
         } else {
             env->regs[15] = env->elr_el[cur_el] & ~0x3;
         }
+
+        env->pc = env->regs[15];
+
         qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
                       "AArch32 EL%d PC 0x%" PRIx32 "\n",
                       cur_el, new_el, env->regs[15]);
-- 
2.7.4
	

Re: [Qemu-devel] target/arm: Fix abort on exception return from AArch64 to AArch32

[Peter Maydell]

On 12 July 2017 at 12:39, Jaroslaw Pelczar <j.pelczar@samsung.com> wrote:
> Problem manifests itself when we handle the following sequence:
>
> 1. 64-bit Secure EL3 returns to 32-bit Secure EL1
> 2. 32-bit Secure EL1 performs SMC call to 64-bit Secure EL3
> 3. 64-bit Secure EL3 performs return ERET to 32-bit Secure EL1]
> 4. 32-bit Secure EL1 receives prefetch abort
>
> If CPU's env->pc is not set to the same value as env->regs[15],
> during ERET the simulator will try to fetch instruction from EL3's
> virtual address inside 32-bit Secure EL1 virtual address space.
> This will cause Prefetch Abort in 32-bit Secure EL1.
>
> Problem occurs because of the following code generation scheme:
>
> 1. disas_uncond_b_reg will decode ERET
> 2. gen_helper_exception_return(cpu_env) will generate thunk to
>    helper_exception_return
> 3. s->is_jmp is set to DISAS_JUMP
> 4. gen_intermediate_code_a64 will see dc->is_jmp == DISAS_JUMP
> 5. tcg_gen_lookup_and_goto_ptr(cpu_pc) will be called which trigger
>    access cpu->pc to fetch next opcode
>
> At this point cpu->pc will have EL3's PC value.
>
> Signed-off-by: Jaroslaw Pelczar <j.pelczar@samsung.com>
> ---
>  target/arm/op_helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2a85666..8222c19 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -1027,6 +1027,9 @@ void HELPER(exception_return)(CPUARMState *env)
>          } else {
>              env->regs[15] = env->elr_el[cur_el] & ~0x3;
>          }
> +
> +        env->pc = env->regs[15];
> +
>          qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
>                        "AArch32 EL%d PC 0x%" PRIx32 "\n",
>                        cur_el, new_el, env->regs[15]);

Thanks for the bug report. I don't think this is the best
fix, though. What should happen is that for ERET we do
not attempt to do a lookup_and_goto_ptr(), we should just
go back out to the main loop and get the next TB from there.

I think this may be fixed as one of the things Alex's
recent patchset tackles?

thanks
-- PMM

Re: [Qemu-devel] target/arm: Fix abort on exception return from AArch64 to AArch32

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 12 July 2017 at 12:39, Jaroslaw Pelczar <j.pelczar@samsung.com> wrote:
>> Problem manifests itself when we handle the following sequence:
>>
>> 1. 64-bit Secure EL3 returns to 32-bit Secure EL1
>> 2. 32-bit Secure EL1 performs SMC call to 64-bit Secure EL3
>> 3. 64-bit Secure EL3 performs return ERET to 32-bit Secure EL1]
>> 4. 32-bit Secure EL1 receives prefetch abort
>>
>> If CPU's env->pc is not set to the same value as env->regs[15],
>> during ERET the simulator will try to fetch instruction from EL3's
>> virtual address inside 32-bit Secure EL1 virtual address space.
>> This will cause Prefetch Abort in 32-bit Secure EL1.
>>
>> Problem occurs because of the following code generation scheme:
>>
>> 1. disas_uncond_b_reg will decode ERET
>> 2. gen_helper_exception_return(cpu_env) will generate thunk to
>>    helper_exception_return
>> 3. s->is_jmp is set to DISAS_JUMP
>> 4. gen_intermediate_code_a64 will see dc->is_jmp == DISAS_JUMP
>> 5. tcg_gen_lookup_and_goto_ptr(cpu_pc) will be called which trigger
>>    access cpu->pc to fetch next opcode
>>
>> At this point cpu->pc will have EL3's PC value.
>>
>> Signed-off-by: Jaroslaw Pelczar <j.pelczar@samsung.com>
>> ---
>>  target/arm/op_helper.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
>> index 2a85666..8222c19 100644
>> --- a/target/arm/op_helper.c
>> +++ b/target/arm/op_helper.c
>> @@ -1027,6 +1027,9 @@ void HELPER(exception_return)(CPUARMState *env)
>>          } else {
>>              env->regs[15] = env->elr_el[cur_el] & ~0x3;
>>          }
>> +
>> +        env->pc = env->regs[15];
>> +
>>          qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
>>                        "AArch32 EL%d PC 0x%" PRIx32 "\n",
>>                        cur_el, new_el, env->regs[15]);
>
> Thanks for the bug report. I don't think this is the best
> fix, though. What should happen is that for ERET we do
> not attempt to do a lookup_and_goto_ptr(), we should just
> go back out to the main loop and get the next TB from there.
>
> I think this may be fixed as one of the things Alex's
> recent patchset tackles?

Yes, see:

  https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg02963.html

I'll probably get v4 out tomorrow at which point you can merge it ;-)

>
> thanks
> -- PMM


--
Alex Bennée