Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub

[Andreas Krebbel]

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