All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
@ 2021-03-24  8:51 Andreas Krebbel
  2021-03-24  9:17 ` David Hildenbrand
  0 siblings, 1 reply; 11+ messages in thread
From: Andreas Krebbel @ 2021-03-24  8:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-s390x, david

The sigreturn SVC is put onto the stack by the emulation code.  Hence
the address of it should not be subject to guest_base transformation
when fetching it.

The fix applies h2g to the address when writing it into the return
address register to nullify the transformation applied to it later.

Note: This only caused problems if Qemu has been built with
--disable-pie (as it is in distros nowadays). Otherwise guest_base
defaults to 0 hiding the actual problem.

Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
---
 linux-user/s390x/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index ecfa2a14a9..1412376958 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
         env->regs[14] = (unsigned long)
                 ka->sa_restorer | PSW_ADDR_AMODE;
     } else {
-        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
+        env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
                         | PSW_ADDR_AMODE;
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
                    (uint16_t *)(frame->retcode));
@@ -213,7 +213,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
     } else {
-        env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
+        env->regs[14] = (unsigned long) h2g(frame->retcode) | PSW_ADDR_AMODE;
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
                    (uint16_t *)(frame->retcode));
     }
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
  2021-03-24  8:51 [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub Andreas Krebbel
@ 2021-03-24  9:17 ` David Hildenbrand
  2021-03-24 10:28   ` Laurent Vivier
  0 siblings, 1 reply; 11+ messages in thread
From: David Hildenbrand @ 2021-03-24  9:17 UTC (permalink / raw)
  To: Andreas Krebbel, qemu-devel; +Cc: qemu-s390x

On 24.03.21 09:51, Andreas Krebbel wrote:
> The sigreturn SVC is put onto the stack by the emulation code.  Hence
> the address of it should not be subject to guest_base transformation
> when fetching it.
> 
> The fix applies h2g to the address when writing it into the return
> address register to nullify the transformation applied to it later.
> 
> Note: This only caused problems if Qemu has been built with
> --disable-pie (as it is in distros nowadays). Otherwise guest_base
> defaults to 0 hiding the actual problem.
> 
> Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
> ---
>   linux-user/s390x/signal.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index ecfa2a14a9..1412376958 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>           env->regs[14] = (unsigned long)
>                   ka->sa_restorer | PSW_ADDR_AMODE;
>       } else {
> -        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
> +        env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
>                           | PSW_ADDR_AMODE;
>           __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
>                      (uint16_t *)(frame->retcode));
> @@ -213,7 +213,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       if (ka->sa_flags & TARGET_SA_RESTORER) {
>           env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
>       } else {
> -        env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
> +        env->regs[14] = (unsigned long) h2g(frame->retcode) | PSW_ADDR_AMODE;
>           __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
>                      (uint16_t *)(frame->retcode));
>       }
> 

Sounds sane to me, although I am not an expert on that code :)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
  2021-03-24  9:17 ` David Hildenbrand
@ 2021-03-24 10:28   ` Laurent Vivier
  2021-03-24 11:26     ` Andreas Krebbel
  0 siblings, 1 reply; 11+ messages in thread
From: Laurent Vivier @ 2021-03-24 10:28 UTC (permalink / raw)
  To: Andreas Krebbel; +Cc: qemu-s390x, qemu-devel, David Hildenbrand

Le 24/03/2021 à 10:17, David Hildenbrand a écrit :
> On 24.03.21 09:51, Andreas Krebbel wrote:
>> The sigreturn SVC is put onto the stack by the emulation code.  Hence
>> the address of it should not be subject to guest_base transformation
>> when fetching it.
>>
>> The fix applies h2g to the address when writing it into the return
>> address register to nullify the transformation applied to it later.
>>
>> Note: This only caused problems if Qemu has been built with
>> --disable-pie (as it is in distros nowadays). Otherwise guest_base
>> defaults to 0 hiding the actual problem.
>>
>> Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> ---
>>   linux-user/s390x/signal.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>> index ecfa2a14a9..1412376958 100644
>> --- a/linux-user/s390x/signal.c
>> +++ b/linux-user/s390x/signal.c
>> @@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>>           env->regs[14] = (unsigned long)
>>                   ka->sa_restorer | PSW_ADDR_AMODE;
>>       } else {
>> -        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>> +        env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
>>                           | PSW_ADDR_AMODE;

Well, it really doesn't sound good as frame_addr is a guest address (and sa_restorer is too)

Where is the code that does the g2h() you want to nullify?

Thanks,
Laurent


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
  2021-03-24 10:28   ` Laurent Vivier
@ 2021-03-24 11:26     ` Andreas Krebbel
  2021-03-24 13:06       ` Laurent Vivier
  0 siblings, 1 reply; 11+ messages in thread
From: Andreas Krebbel @ 2021-03-24 11:26 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: qemu-s390x, qemu-devel, David Hildenbrand

On 3/24/21 11:28 AM, Laurent Vivier wrote:
> Le 24/03/2021 à 10:17, David Hildenbrand a écrit :
>> On 24.03.21 09:51, Andreas Krebbel wrote:
>>> The sigreturn SVC is put onto the stack by the emulation code.  Hence
>>> the address of it should not be subject to guest_base transformation
>>> when fetching it.
>>>
>>> The fix applies h2g to the address when writing it into the return
>>> address register to nullify the transformation applied to it later.
>>>
>>> Note: This only caused problems if Qemu has been built with
>>> --disable-pie (as it is in distros nowadays). Otherwise guest_base
>>> defaults to 0 hiding the actual problem.
>>>
>>> Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
>>> ---
>>>   linux-user/s390x/signal.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>>> index ecfa2a14a9..1412376958 100644
>>> --- a/linux-user/s390x/signal.c
>>> +++ b/linux-user/s390x/signal.c
>>> @@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>>>           env->regs[14] = (unsigned long)
>>>                   ka->sa_restorer | PSW_ADDR_AMODE;
>>>       } else {
>>> -        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>>> +        env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
>>>                           | PSW_ADDR_AMODE;
> 
> Well, it really doesn't sound good as frame_addr is a guest address (and sa_restorer is too)

I would expect the sa_restorer address to actually point into the guest code section.

> 
> Where is the code that does the g2h() you want to nullify?

That's on the code path which usually fetches instructions from memory. In cpu_lduw_code called via:

s390x_tr_translate_insn->translate_one->extract_insn->ld_code2->cpu_lduw_code


Btw. Power also uses h2g while setting up the trampoline address:

...
    save_user_regs(env, mctx);
    encode_trampoline(TARGET_NR_rt_sigreturn, trampptr);

    /* The kernel checks for the presence of a VDSO here.  We don't
       emulate a vdso, so use a sigreturn system call.  */
    env->lr = (target_ulong) h2g(trampptr);
...

> 
> Thanks,
> Laurent
> 

Andreas


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
  2021-03-24 11:26     ` Andreas Krebbel
@ 2021-03-24 13:06       ` Laurent Vivier
  2021-03-24 14:14         ` Andreas Krebbel
  0 siblings, 1 reply; 11+ messages in thread
From: Laurent Vivier @ 2021-03-24 13:06 UTC (permalink / raw)
  To: Andreas Krebbel; +Cc: qemu-s390x, qemu-devel, David Hildenbrand

Le 24/03/2021 à 12:26, Andreas Krebbel a écrit :
> On 3/24/21 11:28 AM, Laurent Vivier wrote:
>> Le 24/03/2021 à 10:17, David Hildenbrand a écrit :
>>> On 24.03.21 09:51, Andreas Krebbel wrote:
>>>> The sigreturn SVC is put onto the stack by the emulation code.  Hence
>>>> the address of it should not be subject to guest_base transformation
>>>> when fetching it.
>>>>
>>>> The fix applies h2g to the address when writing it into the return
>>>> address register to nullify the transformation applied to it later.
>>>>
>>>> Note: This only caused problems if Qemu has been built with
>>>> --disable-pie (as it is in distros nowadays). Otherwise guest_base
>>>> defaults to 0 hiding the actual problem.
>>>>
>>>> Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
>>>> ---
>>>>   linux-user/s390x/signal.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>>>> index ecfa2a14a9..1412376958 100644
>>>> --- a/linux-user/s390x/signal.c
>>>> +++ b/linux-user/s390x/signal.c
>>>> @@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>>>>           env->regs[14] = (unsigned long)
>>>>                   ka->sa_restorer | PSW_ADDR_AMODE;
>>>>       } else {
>>>> -        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>>>> +        env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
>>>>                           | PSW_ADDR_AMODE;
>>
>> Well, it really doesn't sound good as frame_addr is a guest address (and sa_restorer is too)
> 
> I would expect the sa_restorer address to actually point into the guest code section.

yes, it does. like frame_addr. The host address is frame, see:

    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
        goto give_sigsegv;
    }

So frame = g2h(frame_addr)

This line put the address of the next instruction to execute (guest address space):

env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
                        | PSW_ADDR_AMODE;

This line put at this address the NR_sigreturn syscall (but __put_user() uses host address):

        __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
                   (uint16_t *)(frame->retcode));

In theory:

    frame_addr + offsetof(sigframe, retcode) == h2g(frame->retcode)

So the next instruction executed after this function is the sigreturn() syscall.

I think the problem is elsewhere.

But I don't see what is the problem you are trying to solve.

> 
>>
>> Where is the code that does the g2h() you want to nullify?
> 
> That's on the code path which usually fetches instructions from memory. In cpu_lduw_code called via:
> 
> s390x_tr_translate_insn->translate_one->extract_insn->ld_code2->cpu_lduw_code

cpu_lduw_code() takes a guest a address and needs to translate it to host address. We need the g2h()
here because we have a guest address.

> 
> 
> Btw. Power also uses h2g while setting up the trampoline address:
> 
> ...
>     save_user_regs(env, mctx);
>     encode_trampoline(TARGET_NR_rt_sigreturn, trampptr);
> 
>     /* The kernel checks for the presence of a VDSO here.  We don't
>        emulate a vdso, so use a sigreturn system call.  */
>     env->lr = (target_ulong) h2g(trampptr);
> ...

But here, it's correct because trampptr is an host address:

    trampptr = &rt_sf->trampoline[0];

Thanks,
Laurent


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
  2021-03-24 13:06       ` Laurent Vivier
@ 2021-03-24 14:14         ` Andreas Krebbel
  2021-03-24 14:34           ` Richard Henderson
  2021-03-24 15:34           ` Laurent Vivier
  0 siblings, 2 replies; 11+ messages in thread
From: Andreas Krebbel @ 2021-03-24 14:14 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: qemu-s390x, qemu-devel, David Hildenbrand

On 3/24/21 2:06 PM, Laurent Vivier wrote:
> Le 24/03/2021 à 12:26, Andreas Krebbel a écrit :
>> On 3/24/21 11:28 AM, Laurent Vivier wrote:
>>> Le 24/03/2021 à 10:17, David Hildenbrand a écrit :
>>>> On 24.03.21 09:51, Andreas Krebbel wrote:
>>>>> The sigreturn SVC is put onto the stack by the emulation code.  Hence
>>>>> the address of it should not be subject to guest_base transformation
>>>>> when fetching it.
>>>>>
>>>>> The fix applies h2g to the address when writing it into the return
>>>>> address register to nullify the transformation applied to it later.
>>>>>
>>>>> Note: This only caused problems if Qemu has been built with
>>>>> --disable-pie (as it is in distros nowadays). Otherwise guest_base
>>>>> defaults to 0 hiding the actual problem.
>>>>>
>>>>> Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
>>>>> ---
>>>>>   linux-user/s390x/signal.c | 4 ++--
>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>>>>> index ecfa2a14a9..1412376958 100644
>>>>> --- a/linux-user/s390x/signal.c
>>>>> +++ b/linux-user/s390x/signal.c
>>>>> @@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>>>>>           env->regs[14] = (unsigned long)
>>>>>                   ka->sa_restorer | PSW_ADDR_AMODE;
>>>>>       } else {
>>>>> -        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>>>>> +        env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
>>>>>                           | PSW_ADDR_AMODE;
>>>
>>> Well, it really doesn't sound good as frame_addr is a guest address (and sa_restorer is too)
>>
>> I would expect the sa_restorer address to actually point into the guest code section.
> 
> yes, it does. like frame_addr. The host address is frame, see:
> 
>     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
>         goto give_sigsegv;
>     }
> 
> So frame = g2h(frame_addr)
> 
> This line put the address of the next instruction to execute (guest address space):
> 
> env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>                         | PSW_ADDR_AMODE;
> 
> This line put at this address the NR_sigreturn syscall (but __put_user() uses host address):
> 
>         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
>                    (uint16_t *)(frame->retcode));
> 
> In theory:
> 
>     frame_addr + offsetof(sigframe, retcode) == h2g(frame->retcode)
> 
> So the next instruction executed after this function is the sigreturn() syscall.
> 
> I think the problem is elsewhere.
> 
> But I don't see what is the problem you are trying to solve.
> 
>>
>>>
>>> Where is the code that does the g2h() you want to nullify?
>>
>> That's on the code path which usually fetches instructions from memory. In cpu_lduw_code called via:
>>
>> s390x_tr_translate_insn->translate_one->extract_insn->ld_code2->cpu_lduw_code
> 
> cpu_lduw_code() takes a guest a address and needs to translate it to host address. We need the g2h()
> here because we have a guest address.
> 
>>
>>
>> Btw. Power also uses h2g while setting up the trampoline address:
>>
>> ...
>>     save_user_regs(env, mctx);
>>     encode_trampoline(TARGET_NR_rt_sigreturn, trampptr);
>>
>>     /* The kernel checks for the presence of a VDSO here.  We don't
>>        emulate a vdso, so use a sigreturn system call.  */
>>     env->lr = (target_ulong) h2g(trampptr);
>> ...
> 
> But here, it's correct because trampptr is an host address:
> 
>     trampptr = &rt_sf->trampoline[0];
> 
> Thanks,
> Laurent
> 

Unfortunately I've confused the two locations which do the trampoline setup in the discussion
setup_frame vs setup_rt_frame.

The part I actually needed to fix was in setup_rt_frame and there the fix is correct I think since
here we do use 'frame' which is the host address.

While doing that change I also stumbled upon the other location in setup_frame. There it is using
frame_addr for doing the same thing. There as you say adding h2g is wrong.

Here just the change which I think is needed:

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index ecfa2a14a9..7fba1c7999 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -213,7 +213,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
     } else {
-        env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
+        env->regs[14] = (unsigned long) h2g(frame->retcode) | PSW_ADDR_AMODE;
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
                    (uint16_t *)(frame->retcode));
     }


Andreas


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
  2021-03-24 14:14         ` Andreas Krebbel
@ 2021-03-24 14:34           ` Richard Henderson
  2021-03-24 15:34           ` Laurent Vivier
  1 sibling, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2021-03-24 14:34 UTC (permalink / raw)
  To: Andreas Krebbel, Laurent Vivier; +Cc: qemu-s390x, qemu-devel, David Hildenbrand

On 3/24/21 8:14 AM, Andreas Krebbel wrote:
> The part I actually needed to fix was in setup_rt_frame and there the fix is correct I think since
> here we do use 'frame' which is the host address.
> 
> While doing that change I also stumbled upon the other location in setup_frame. There it is using
> frame_addr for doing the same thing. There as you say adding h2g is wrong.
> 
> Here just the change which I think is needed:
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index ecfa2a14a9..7fba1c7999 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -213,7 +213,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       if (ka->sa_flags & TARGET_SA_RESTORER) {
>           env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
>       } else {
> -        env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
> +        env->regs[14] = (unsigned long) h2g(frame->retcode) | PSW_ADDR_AMODE;

Correct, though I think the formulation using frame_addr is more obvious.

Unrelated, but all the uses of "unsigned long" are wrong -- they should be 
target_ulong.


r~


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
  2021-03-24 14:14         ` Andreas Krebbel
  2021-03-24 14:34           ` Richard Henderson
@ 2021-03-24 15:34           ` Laurent Vivier
  2021-03-24 15:55             ` [PATCH v2] linux-user/s390x: Use the guest pointer for the " Andreas Krebbel
  1 sibling, 1 reply; 11+ messages in thread
From: Laurent Vivier @ 2021-03-24 15:34 UTC (permalink / raw)
  To: Andreas Krebbel; +Cc: qemu-s390x, qemu-devel, David Hildenbrand

Le 24/03/2021 à 15:14, Andreas Krebbel a écrit :
> On 3/24/21 2:06 PM, Laurent Vivier wrote:
>> Le 24/03/2021 à 12:26, Andreas Krebbel a écrit :
>>> On 3/24/21 11:28 AM, Laurent Vivier wrote:
>>>> Le 24/03/2021 à 10:17, David Hildenbrand a écrit :
>>>>> On 24.03.21 09:51, Andreas Krebbel wrote:
>>>>>> The sigreturn SVC is put onto the stack by the emulation code.  Hence
>>>>>> the address of it should not be subject to guest_base transformation
>>>>>> when fetching it.
>>>>>>
>>>>>> The fix applies h2g to the address when writing it into the return
>>>>>> address register to nullify the transformation applied to it later.
>>>>>>
>>>>>> Note: This only caused problems if Qemu has been built with
>>>>>> --disable-pie (as it is in distros nowadays). Otherwise guest_base
>>>>>> defaults to 0 hiding the actual problem.
>>>>>>
>>>>>> Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
>>>>>> ---
>>>>>>   linux-user/s390x/signal.c | 4 ++--
>>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>>>>>> index ecfa2a14a9..1412376958 100644
>>>>>> --- a/linux-user/s390x/signal.c
>>>>>> +++ b/linux-user/s390x/signal.c
>>>>>> @@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>>>>>>           env->regs[14] = (unsigned long)
>>>>>>                   ka->sa_restorer | PSW_ADDR_AMODE;
>>>>>>       } else {
>>>>>> -        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>>>>>> +        env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
>>>>>>                           | PSW_ADDR_AMODE;
>>>>
>>>> Well, it really doesn't sound good as frame_addr is a guest address (and sa_restorer is too)
>>>
>>> I would expect the sa_restorer address to actually point into the guest code section.
>>
>> yes, it does. like frame_addr. The host address is frame, see:
>>
>>     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
>>         goto give_sigsegv;
>>     }
>>
>> So frame = g2h(frame_addr)
>>
>> This line put the address of the next instruction to execute (guest address space):
>>
>> env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>>                         | PSW_ADDR_AMODE;
>>
>> This line put at this address the NR_sigreturn syscall (but __put_user() uses host address):
>>
>>         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
>>                    (uint16_t *)(frame->retcode));
>>
>> In theory:
>>
>>     frame_addr + offsetof(sigframe, retcode) == h2g(frame->retcode)
>>
>> So the next instruction executed after this function is the sigreturn() syscall.
>>
>> I think the problem is elsewhere.
>>
>> But I don't see what is the problem you are trying to solve.
>>
>>>
>>>>
>>>> Where is the code that does the g2h() you want to nullify?
>>>
>>> That's on the code path which usually fetches instructions from memory. In cpu_lduw_code called via:
>>>
>>> s390x_tr_translate_insn->translate_one->extract_insn->ld_code2->cpu_lduw_code
>>
>> cpu_lduw_code() takes a guest a address and needs to translate it to host address. We need the g2h()
>> here because we have a guest address.
>>
>>>
>>>
>>> Btw. Power also uses h2g while setting up the trampoline address:
>>>
>>> ...
>>>     save_user_regs(env, mctx);
>>>     encode_trampoline(TARGET_NR_rt_sigreturn, trampptr);
>>>
>>>     /* The kernel checks for the presence of a VDSO here.  We don't
>>>        emulate a vdso, so use a sigreturn system call.  */
>>>     env->lr = (target_ulong) h2g(trampptr);
>>> ...
>>
>> But here, it's correct because trampptr is an host address:
>>
>>     trampptr = &rt_sf->trampoline[0];
>>
>> Thanks,
>> Laurent
>>
> 
> Unfortunately I've confused the two locations which do the trampoline setup in the discussion
> setup_frame vs setup_rt_frame.
> 
> The part I actually needed to fix was in setup_rt_frame and there the fix is correct I think since
> here we do use 'frame' which is the host address.
> 
> While doing that change I also stumbled upon the other location in setup_frame. There it is using
> frame_addr for doing the same thing. There as you say adding h2g is wrong.
> 
> Here just the change which I think is needed:
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index ecfa2a14a9..7fba1c7999 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -213,7 +213,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>      if (ka->sa_flags & TARGET_SA_RESTORER) {
>          env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
>      } else {
> -        env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
> +        env->regs[14] = (unsigned long) h2g(frame->retcode) | PSW_ADDR_AMODE;
>          __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
>                     (uint16_t *)(frame->retcode));
>      }
> 

This is correct, but as we have frame_addr, it's better to have the same code as in setup_frame()
(frame_addr + offsetof(sigframe, retcode))

Thanks,
Laurent



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] linux-user/s390x: Use the guest pointer for the sigreturn stub
  2021-03-24 15:34           ` Laurent Vivier
@ 2021-03-24 15:55             ` Andreas Krebbel
  2021-03-24 17:53               ` Laurent Vivier
  0 siblings, 1 reply; 11+ messages in thread
From: Andreas Krebbel @ 2021-03-24 15:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-s390x, richard.henderson, laurent

When setting up the pointer for the sigreturn stub in the return
address register (r14) we have to use the guest frame pointer instead
of the host frame pointer.

Note: This only caused problems if Qemu has been built with
--disable-pie (as it is in distros nowadays). Otherwise guest_base
defaults to 0 hiding the actual problem.

Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
---
 linux-user/s390x/signal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index ecfa2a14a9..e9bf865074 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -213,7 +213,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
     } else {
-        env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
+        env->regs[14] = (target_ulong) (frame_addr + offsetof(rt_sigframe, retcode))
+                        | PSW_ADDR_AMODE;
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
                    (uint16_t *)(frame->retcode));
     }
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] linux-user/s390x: Use the guest pointer for the sigreturn stub
  2021-03-24 15:55             ` [PATCH v2] linux-user/s390x: Use the guest pointer for the " Andreas Krebbel
@ 2021-03-24 17:53               ` Laurent Vivier
  2021-03-24 18:48                 ` Andreas Krebbel
  0 siblings, 1 reply; 11+ messages in thread
From: Laurent Vivier @ 2021-03-24 17:53 UTC (permalink / raw)
  To: Andreas Krebbel, qemu-devel; +Cc: qemu-s390x, richard.henderson

Le 24/03/2021 à 16:55, Andreas Krebbel a écrit :
> When setting up the pointer for the sigreturn stub in the return
> address register (r14) we have to use the guest frame pointer instead
> of the host frame pointer.
> 
> Note: This only caused problems if Qemu has been built with
> --disable-pie (as it is in distros nowadays). Otherwise guest_base
> defaults to 0 hiding the actual problem.
> 
> Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
> ---
>  linux-user/s390x/signal.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index ecfa2a14a9..e9bf865074 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -213,7 +213,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>      if (ka->sa_flags & TARGET_SA_RESTORER) {
>          env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
>      } else {
> -        env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
> +        env->regs[14] = (target_ulong) (frame_addr + offsetof(rt_sigframe, retcode))
> +                        | PSW_ADDR_AMODE;
>          __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
>                     (uint16_t *)(frame->retcode));
>      }
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

but if you want to send a v3:
- to be consistent with lines below, use "offsetof(typeof(*frame), ..."
- in the line above, you can remove the (unsigned long) of the sa_restorer as it is an abi_ulong,
- don't send the "v2" as a reply to the v1 as it can be hidden in the mail thread and missed by the
maintainer :)

Thanks,
Laurent


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] linux-user/s390x: Use the guest pointer for the sigreturn stub
  2021-03-24 17:53               ` Laurent Vivier
@ 2021-03-24 18:48                 ` Andreas Krebbel
  0 siblings, 0 replies; 11+ messages in thread
From: Andreas Krebbel @ 2021-03-24 18:48 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel; +Cc: qemu-s390x, richard.henderson

On 3/24/21 6:53 PM, Laurent Vivier wrote:
> Le 24/03/2021 à 16:55, Andreas Krebbel a écrit :
>> When setting up the pointer for the sigreturn stub in the return
>> address register (r14) we have to use the guest frame pointer instead
>> of the host frame pointer.
>>
>> Note: This only caused problems if Qemu has been built with
>> --disable-pie (as it is in distros nowadays). Otherwise guest_base
>> defaults to 0 hiding the actual problem.
>>
>> Signed-off-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> ---
>>  linux-user/s390x/signal.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>> index ecfa2a14a9..e9bf865074 100644
>> --- a/linux-user/s390x/signal.c
>> +++ b/linux-user/s390x/signal.c
>> @@ -213,7 +213,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>>      if (ka->sa_flags & TARGET_SA_RESTORER) {
>>          env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
>>      } else {
>> -        env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
>> +        env->regs[14] = (target_ulong) (frame_addr + offsetof(rt_sigframe, retcode))
>> +                        | PSW_ADDR_AMODE;
>>          __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
>>                     (uint16_t *)(frame->retcode));
>>      }
>>
> 
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> 
> but if you want to send a v3:

Sure, will do.

> - to be consistent with lines below, use "offsetof(typeof(*frame), ..."
> - in the line above, you can remove the (unsigned long) of the sa_restorer as it is an abi_ulong,

The (target_ulong) cast could probably go away as well since frame_addr is also abi_ulong.

> - don't send the "v2" as a reply to the v1 as it can be hidden in the mail thread and missed by the
> maintainer :)

Ok.

Andreas

> 
> Thanks,
> Laurent
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-03-24 18:56 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-24  8:51 [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub Andreas Krebbel
2021-03-24  9:17 ` David Hildenbrand
2021-03-24 10:28   ` Laurent Vivier
2021-03-24 11:26     ` Andreas Krebbel
2021-03-24 13:06       ` Laurent Vivier
2021-03-24 14:14         ` Andreas Krebbel
2021-03-24 14:34           ` Richard Henderson
2021-03-24 15:34           ` Laurent Vivier
2021-03-24 15:55             ` [PATCH v2] linux-user/s390x: Use the guest pointer for the " Andreas Krebbel
2021-03-24 17:53               ` Laurent Vivier
2021-03-24 18:48                 ` Andreas Krebbel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.