| Message ID | 20210324085129.29684-1-krebbel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] linux-user/s390x: Apply h2g to address of sigreturn stub | expand |
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
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
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~
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
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)); }
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(-)