[Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[riku.voipio]

From: Riku Voipio <riku.voipio@linaro.org>

On AArch64 the si_addr field of siginfo_t is truncated to 32 bits
because the fault address passes through an uint32_t variable. This
is fixed by changing the variable to uint64_t.

v2 by Riku - follow Peters suggestion and drop the addr variable
since its only used once in the Aarch64 loop.

Reported-by: Amanieu d'Antras <amanieu@gmail.com>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 5887022..5c14c1e 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1006,7 +1006,6 @@ void cpu_loop(CPUARMState *env)
     CPUState *cs = CPU(arm_env_get_cpu(env));
     int trapnr, sig;
     target_siginfo_t info;
-    uint32_t addr;
 
     for (;;) {
         cpu_exec_start(cs);
@@ -1042,12 +1041,11 @@ void cpu_loop(CPUARMState *env)
             /* fall through for segv */
         case EXCP_PREFETCH_ABORT:
         case EXCP_DATA_ABORT:
-            addr = env->exception.vaddress;
             info.si_signo = SIGSEGV;
             info.si_errno = 0;
             /* XXX: check env->error_code */
             info.si_code = TARGET_SEGV_MAPERR;
-            info._sifields._sigfault._addr = addr;
+            info._sifields._sigfault._addr = env->exception.vaddress;
             queue_signal(env, info.si_signo, &info);
             break;
         case EXCP_DEBUG:
-- 
2.1.1

Re: [Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[Peter Maydell]

On 23 October 2014 12:55,  <riku.voipio@linaro.org> wrote:
> From: Riku Voipio <riku.voipio@linaro.org>
>
> On AArch64 the si_addr field of siginfo_t is truncated to 32 bits
> because the fault address passes through an uint32_t variable. This
> is fixed by changing the variable to uint64_t.
>
> v2 by Riku - follow Peters suggestion and drop the addr variable
> since its only used once in the Aarch64 loop.
>
> Reported-by: Amanieu d'Antras <amanieu@gmail.com>
> Signed-off-by: Riku Voipio <riku.voipio@linaro.org>

Ah, I was just wondering this morning whether I ought
to do a version of this patch, so you've saved me the effort :-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(You might want to put the notes about v2 below the '---'.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[Richard Henderson]

On 10/23/2014 05:06 AM, Peter Maydell wrote:
> On 23 October 2014 12:55,  <riku.voipio@linaro.org> wrote:
>> From: Riku Voipio <riku.voipio@linaro.org>
>>
>> On AArch64 the si_addr field of siginfo_t is truncated to 32 bits
>> because the fault address passes through an uint32_t variable. This
>> is fixed by changing the variable to uint64_t.
>>
>> v2 by Riku - follow Peters suggestion and drop the addr variable
>> since its only used once in the Aarch64 loop.
>>
>> Reported-by: Amanieu d'Antras <amanieu@gmail.com>
>> Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
> 
> Ah, I was just wondering this morning whether I ought
> to do a version of this patch, so you've saved me the effort :-)
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> (You might want to put the notes about v2 below the '---'.)

If you do that, please just modify the entire patch description.
I.e. don't talk about the type of a variable that doesn't exist.


r~

[Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[Amanieu d'Antras]

On AArch64 the si_addr field of siginfo_t is truncated to 32 bits
because the fault address passes through an uint32_t variable. This
is fixed by changing the variable to uint64_t.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
---
 linux-user/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 483eb3f..d63e093 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1006,7 +1006,7 @@ void cpu_loop(CPUARMState *env)
     CPUState *cs = CPU(arm_env_get_cpu(env));
     int trapnr, sig;
     target_siginfo_t info;
-    uint32_t addr;
+    uint64_t addr;
 
     for (;;) {
         cpu_exec_start(cs);
-- 
2.1.2

Re: [Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[Claudio Fontana]

Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>

On 10.10.2014 06:22, Amanieu d'Antras wrote:
> On AArch64 the si_addr field of siginfo_t is truncated to 32 bits
> because the fault address passes through an uint32_t variable. This
> is fixed by changing the variable to uint64_t.
> 
> Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> ---
>  linux-user/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 483eb3f..d63e093 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -1006,7 +1006,7 @@ void cpu_loop(CPUARMState *env)
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      int trapnr, sig;
>      target_siginfo_t info;
> -    uint32_t addr;
> +    uint64_t addr;
>  
>      for (;;) {
>          cpu_exec_start(cs);
> 

Re: [Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[Peter Maydell]

On 10 October 2014 05:22, Amanieu d'Antras <amanieu@gmail.com> wrote:
> On AArch64 the si_addr field of siginfo_t is truncated to 32 bits
> because the fault address passes through an uint32_t variable. This
> is fixed by changing the variable to uint64_t.
>
> Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> ---
>  linux-user/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 483eb3f..d63e093 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -1006,7 +1006,7 @@ void cpu_loop(CPUARMState *env)
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      int trapnr, sig;
>      target_siginfo_t info;
> -    uint32_t addr;
> +    uint64_t addr;
>
>      for (;;) {
>          cpu_exec_start(cs);

Thanks for catching this. Better to fix it by dropping
the unnecessary local variable completely and just setting
  info._sifields._sigfault._addr = env->exception.vaddress;
at the only point where we currently use 'addr', though,
I think.

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[Claudio Fontana]

On 10.10.2014 13:32, Peter Maydell wrote:
> On 10 October 2014 05:22, Amanieu d'Antras <amanieu@gmail.com> wrote:
>> On AArch64 the si_addr field of siginfo_t is truncated to 32 bits
>> because the fault address passes through an uint32_t variable. This
>> is fixed by changing the variable to uint64_t.
>>
>> Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
>> ---
>>  linux-user/main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index 483eb3f..d63e093 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -1006,7 +1006,7 @@ void cpu_loop(CPUARMState *env)
>>      CPUState *cs = CPU(arm_env_get_cpu(env));
>>      int trapnr, sig;
>>      target_siginfo_t info;
>> -    uint32_t addr;
>> +    uint64_t addr;
>>
>>      for (;;) {
>>          cpu_exec_start(cs);
> 
> Thanks for catching this. Better to fix it by dropping
> the unnecessary local variable completely and just setting
>   info._sifields._sigfault._addr = env->exception.vaddress;
> at the only point where we currently use 'addr', though,
> I think.
> 
> -- PMM

We do use the name code pattern in most of the functions of that file,
meaning

uint32_t / uint64_t / target_ulong addr;

and then do_something_with_addr(addr);

Ciao,

Claudio

Re: [Qemu-devel] [PATCH] linux-user: Fix fault address truncation AArch64

[Peter Maydell]

On 10 October 2014 15:58, Claudio Fontana <claudio.fontana@huawei.com> wrote:
> On 10.10.2014 13:32, Peter Maydell wrote:
>> Thanks for catching this. Better to fix it by dropping
>> the unnecessary local variable completely and just setting
>>   info._sifields._sigfault._addr = env->exception.vaddress;
>> at the only point where we currently use 'addr', though,
>> I think.

> We do use the name code pattern in most of the functions of that file,
> meaning
>
> uint32_t / uint64_t / target_ulong addr;
>
> and then do_something_with_addr(addr);

In other functions the variable is used more than once,
mostly. (In cases where it is not I would also be happy
with cleanup patches which removed it.)

-- PMM