From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3E2DFC433C1 for ; Wed, 24 Mar 2021 15:38:46 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 90C2960240 for ; Wed, 24 Mar 2021 15:38:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 90C2960240 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=vivier.eu Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:42316 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lP5am-0007VY-Lr for qemu-devel@archiver.kernel.org; Wed, 24 Mar 2021 11:38:44 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:43892) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lP5Wh-00042B-Dy; Wed, 24 Mar 2021 11:34:31 -0400 Received: from mout.kundenserver.de ([212.227.126.131]:57617) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lP5WY-0002lP-LM; Wed, 24 Mar 2021 11:34:31 -0400 Received: from [192.168.100.1] ([82.142.25.162]) by mrelayeu.kundenserver.de (mreue012 [213.165.67.103]) with ESMTPSA (Nemesis) id 1MEUaQ-1lQkCl2Oj4-00G2qH; Wed, 24 Mar 2021 16:34:13 +0100 Subject: Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub To: Andreas Krebbel References: <20210324085129.29684-1-krebbel@linux.ibm.com> <5070a253-cd95-59b0-dbdb-2eb549e9f61c@redhat.com> <3d64f14f-58a4-7cc3-a069-f7ed1172d038@linux.ibm.com> <4fadf180-24d5-01f2-8345-14d52f098db4@linux.ibm.com> From: Laurent Vivier Message-ID: Date: Wed, 24 Mar 2021 16:34:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <4fadf180-24d5-01f2-8345-14d52f098db4@linux.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 8bit X-Provags-ID: V03:K1:hCzdDIdfl6zBaIzw6eEoIrd0rY7WWnQzbW6akASFQs1SFZN7xfE a5pYbBHbwvfNTz0zejUSKAatz4NSrf/lBsBSxZ7KTXSd1pkoAxOd2iLd6bXo49hNFroOAgr h5PBGUGFzzfrkCArvlo13TefEgaycjPuAjlJGgr1uwFEDWTA++ec66IhYn3KQMLQGThdE5L CROlKfeqGwaC+9IFhciRg== X-UI-Out-Filterresults: notjunk:1;V03:K0:DcxJM2DnbN4=:pskkBhywrt9aIuxC5y+Sfo IkGWNlMNeF5BIk8fYwwxvwjLiW0jA0mrIqPMFjabbV+ie52vMWKwbeWdnSiTRgLubMCITzMdt zGaeQTbBdgv2Zm0csNa67D/d40/K73IopF5xNYNCZKu1MkPhuMxaDpMlI1zOyDawido2isyw2 StmVEozmfdlQBPFNG+w9YTEnHstG7muz6ysDw+q4JD8ilTxNuv3v5nZvcofbjfKbFo9J3HCKh pSIwYBSRkS1q63ZEL3eikAlK4p1ZuD0B+zmKIFfS1d9KZ8xg2TbTsAYGqbccq2MmM/vlS+OrQ ouiMNLGCmz6NObhU3feoT5y6igVV8beS8IW9qyMwuoDFW/mrhg8cLqFuo2Txof79U5B4aT1Km MaZ2pYvtVPCVQGoO4fm+78Rx/X397D9Ko6GdaoaUmbWt0DQKqqWRXlyjKaEq5La04IH1d6Z6q m4tN+E59zA== Received-SPF: none client-ip=212.227.126.131; envelope-from=laurent@vivier.eu; helo=mout.kundenserver.de X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: qemu-s390x@nongnu.org, qemu-devel@nongnu.org, David Hildenbrand Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" 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 >>>>>> --- >>>>>>   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