* [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.