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

[Laurent Vivier <laurent@vivier.eu>]

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