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=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 052C6C433ED for ; Thu, 22 Apr 2021 18:25:43 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 4B43561422 for ; Thu, 22 Apr 2021 18:25:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4B43561422 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xmission.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Subject:MIME-Version:Message-ID:Date:References: In-Reply-To:Cc:To:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=YqyQ3/lu6N1AgdZMT9R5mlj6GxhPHPa7yiV9avaplBA=; b=ozI90HJYj288PtzcQMurWhKBQ WANuRUrNjey5QcWBeAInkRsuIwmT17UZQOEVROXA7g7IOArlTiwqckbhElculWMv5aWI1Ln21EVnP d2hYa0H+wPXlcYH5GDUj+vRtmAS+OsGUChsjPvbdytn+1N3bBmwTFTFA4cUDMt22sR6rYJR/HU9oW MDb2wF78XWNAasBesuSrSO6UZWDdaJFEcCpJhhRalWBaXq3MlHUiDEhP8LxQ7ng/0WK8kUwmHQgef elGe+RFHkvL7fiyhKaB/YGhsKMvBFNN4xd2oliYvVVtQmTuft8ArmhvkvdTttYUdboOlwN6XZaHMt o+QdNJIyg==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lZdzE-00HEc5-Sc; Thu, 22 Apr 2021 18:23:37 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lZdzA-00HEbs-Hy for linux-arm-kernel@desiato.infradead.org; Thu, 22 Apr 2021 18:23:34 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Subject:Content-Type:MIME-Version: Message-ID:Date:References:In-Reply-To:Cc:To:From:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=U+qSarm+PX/uTj5SvSnv/ao8yfy/Oym1dZ7wHwxnrNo=; b=iNfPF3r8HZvDHBPNXcRehB1SvZ oNTYbh5Da0D1rsL7C+OpTanWaYzU9b2RooHXc7jyMb5OFScZ+je0AzLDM/gwp/CuWyl+fewiwYjVc Ouh8ogn4k/UycOZb50Z4k2JXhJ7UTpeGy0mvmBjfSlzx2Km9IHZETzmry9wgRHSqiluv7eYL666dn GwkfIq5zXjI8ixXNt3OxRcpBS+cWRE5v1qfPzvAsXG1DWRifZGeJ2TNGTvUx6TUzB3Wcz1hbVlqEf pUB2/wfhnwL3OGca6tX+Gqx7SNQ0idlSWpnk1ju3e+LvYYNWehQroV2It4CcvGmbYl3/muHs872tw k9O62INw==; Received: from out01.mta.xmission.com ([166.70.13.231]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lZdz7-00Dus1-ED for linux-arm-kernel@lists.infradead.org; Thu, 22 Apr 2021 18:23:31 +0000 Received: from in02.mta.xmission.com ([166.70.13.52]) by out01.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1lZdz3-00FE0J-4i; Thu, 22 Apr 2021 12:23:25 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=fess.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1lZdz1-0002QV-Qf; Thu, 22 Apr 2021 12:23:24 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Will Deacon Cc: Liam Howlett , Catalin Marinas , Julien Grall , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , "linux-arm-kernel\@lists.infradead.org" , "linux-kernel\@vger.kernel.org" , "netdev\@vger.kernel.org" , "bpf\@vger.kernel.org" In-Reply-To: <20210422124849.GA1521@willie-the-truck> (Will Deacon's message of "Thu, 22 Apr 2021 13:48:50 +0100") References: <20210420165001.3790670-1-Liam.Howlett@Oracle.com> <20210420165001.3790670-2-Liam.Howlett@Oracle.com> <20210422124849.GA1521@willie-the-truck> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) Date: Thu, 22 Apr 2021 13:22:04 -0500 Message-ID: MIME-Version: 1.0 X-XM-SPF: eid=1lZdz1-0002QV-Qf; ; ; mid=; ; ; hst=in02.mta.xmission.com; ; ; ip=68.227.160.95; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX18OxciQMXgnIAFK7zvjgsqQsTkQAuA57qY= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 2/3] arm64: signal: sigreturn() and rt_sigreturn() sometime returns the wrong signals X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210422_112329_527467_BF439C06 X-CRM114-Status: GOOD ( 29.85 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Will Deacon writes: > [+Eric as he actually understands how this is supposed to work] I try. > On Tue, Apr 20, 2021 at 04:50:13PM +0000, Liam Howlett wrote: >> arm64_notify_segfault() was used to force a SIGSEGV in all error cases >> in sigreturn() and rt_sigreturn() to avoid writing a new sig handler. >> There is now a better sig handler to use which does not search the VMA >> address space and return a slightly incorrect error code. Restore the >> older and correct si_code of SI_KERNEL by using arm64_notify_die(). In >> the case of !access_ok(), simply return SIGSEGV with si_code >> SEGV_ACCERR. What is userspace cares? Why does it care? This is changing userspace visible semantics so understanding userspace and understanding how it might break, and what the risk of regression seems the most important detail here. >> This change requires exporting arm64_notfiy_die() to the arm64 traps.h >> >> Fixes: f71016a8a8c5 (arm64: signal: Call arm64_notify_segfault when >> failing to deliver signal) >> Signed-off-by: Liam R. Howlett >> --- >> arch/arm64/include/asm/traps.h | 2 ++ >> arch/arm64/kernel/signal.c | 8 ++++++-- >> arch/arm64/kernel/signal32.c | 18 ++++++++++++++---- >> 3 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h >> index 54f32a0675df..9b76144fcba6 100644 >> --- a/arch/arm64/include/asm/traps.h >> +++ b/arch/arm64/include/asm/traps.h >> @@ -29,6 +29,8 @@ void arm64_notify_segfault(unsigned long addr); >> void arm64_force_sig_fault(int signo, int code, unsigned long far, const char *str); >> void arm64_force_sig_mceerr(int code, unsigned long far, short lsb, const char *str); >> void arm64_force_sig_ptrace_errno_trap(int errno, unsigned long far, const char *str); >> +void arm64_notify_die(const char *str, struct pt_regs *regs, int signo, >> + int sicode, unsigned long far, int err); >> >> /* >> * Move regs->pc to next instruction and do necessary setup before it >> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c >> index 6237486ff6bb..9fde6dc760c3 100644 >> --- a/arch/arm64/kernel/signal.c >> +++ b/arch/arm64/kernel/signal.c >> @@ -544,7 +544,7 @@ SYSCALL_DEFINE0(rt_sigreturn) >> frame = (struct rt_sigframe __user *)regs->sp; >> >> if (!access_ok(frame, sizeof (*frame))) >> - goto badframe; >> + goto e_access; >> >> if (restore_sigframe(regs, frame)) >> goto badframe; >> @@ -555,7 +555,11 @@ SYSCALL_DEFINE0(rt_sigreturn) >> return regs->regs[0]; >> >> badframe: >> - arm64_notify_segfault(regs->sp); >> + arm64_notify_die("Bad frame", regs, SIGSEGV, SI_KERNEL, regs->sp, 0); >> + return 0; >> + >> +e_access: >> + force_signal_inject(SIGSEGV, SEGV_ACCERR, regs->sp, 0); >> return 0; > > This seems really error-prone to me, but maybe I'm just missing some > context. What's the rule for reporting an si_code of SI_KERNEL vs > SEGV_ACCERR, and is the former actually valid for SIGSEGV? The si_codes SI_USER == 0 and SI_KERNEL == 0x80 are valid for all signals. SI_KERNEL means I don't have any information for you other than signal number. In general something better than SI_KERNEL is desirable. > With this change, pointing the (signal) stack to a kernel address will > result in SEGV_ACCERR but pointing it to something like a PROT_NONE user > address will give SI_KERNEL (well, assuming that we manage to deliver > the SEGV somehow). I'm having a hard time seeing why that's a useful > distinction to make.. > > If it's important to get this a particular way around, please can you > add some selftests? Going down the current path I see 3 possible cases: copy_from_user returns -EFAULT which becomes SEGV_MAPERR or SEGV_ACCERR. A signal frame parse error. For which SI_KERNEL seems as good an error code as any. On x86 there is no attempt to figure out the cause of the -EFAULT, and always uses SI_KERNEL. This is because x86 just does: "force_sig(SIGSEGV);" As arm64 did until f71016a8a8c5 ("arm64: signal: Call arm64_notify_segfault when failing to deliver signal") I think the big question is what does it make sense to do here. The big picture. Upon return from a signal the kernel arranges for rt_sigreturn to be called to return to a pre-signal state. As such rt_sigreturn can not return an error code. In general the kernel will write the signal frame and that will guarantee that the signal from can be processes by rt_sigreturn. For error handling we are dealing with the case that userspace has modified the signal frame. So that it either does not parse or that it is unmapped. So who cares? The only two cases I can think of are debuggers, and programs playing fancy memory management games like garbage collections. I have heard of applications (like garbage collectors) unmapping/mprotecting memory to create a barrier. Liam Howlett is that the issue here? Is not seeing SI_KERNEL confusing the JVM? For debuggers I expect the stack backtrace from SIGSEGV is enough to see that something is wrong. For applications performing fancy processing of the signal frame I think that tends to be very architecture specific. In part because even knowing the faulting address the size of the access is not known so the instruction must be interpreted. Getting a system call instead of a load or store instruction might be enough to completely confuse applications processing SEGV_MAPERR or SEGV_ACCERR. Such applications may also struggle with the fact that the address in siginfo is less precise than it would be for an ordinary page fault. So my sense is if you known you are helping userspace returning either SEGV_MAPERR or SEGV_ACCERR go for it. Otherwise there are enough variables that returning less information when rt_sigreturn fails would be more reliable. Or in short what is userspace doing? What does userspace care about? Eric _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel