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.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 18664C433E0 for ; Thu, 4 Feb 2021 17:00:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DBB4564F65 for ; Thu, 4 Feb 2021 17:00:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238256AbhBDQ7n (ORCPT ); Thu, 4 Feb 2021 11:59:43 -0500 Received: from foss.arm.com ([217.140.110.172]:33412 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237943AbhBDQm4 (ORCPT ); Thu, 4 Feb 2021 11:42:56 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 12BE411D4; Thu, 4 Feb 2021 08:42:08 -0800 (PST) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CFDA33F718; Thu, 4 Feb 2021 08:42:06 -0800 (PST) Date: Thu, 4 Feb 2021 16:41:46 +0000 From: Dave Martin To: Will Deacon Cc: Andrei Vagin , Catalin Marinas , Oleg Nesterov , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, Anthony Steinhauser , Keno Fischer Subject: Re: [PATCH 1/3] arm64/ptrace: don't clobber task registers on syscall entry/exit traps Message-ID: <20210204164145.GB21837@arm.com> References: <20210201194012.524831-1-avagin@gmail.com> <20210201194012.524831-2-avagin@gmail.com> <20210204152334.GA21058@willie-the-truck> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210204152334.GA21058@willie-the-truck> User-Agent: Mutt/1.5.23 (2014-03-12) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 04, 2021 at 03:23:34PM +0000, Will Deacon wrote: > On Mon, Feb 01, 2021 at 11:40:10AM -0800, Andrei Vagin wrote: > > ip/r12 for AArch32 and x7 for AArch64 is used to indicate whether or not > > the stop has been signalled from syscall entry or syscall exit. This > > means that: > > > > - Any writes by the tracer to this register during the stop are > > ignored/discarded. > > > > - The actual value of the register is not available during the stop, > > so the tracer cannot save it and restore it later. > > > > Right now, these registers are clobbered in tracehook_report_syscall. > > This change moves the logic to gpr_get and compat_gpr_get where > > registers are copied into a user-space buffer. > > > > This will allow to change these registers and to introduce a new > > ptrace option to get the full set of registers. > > > > Signed-off-by: Andrei Vagin > > --- > > arch/arm64/include/asm/ptrace.h | 5 ++ > > arch/arm64/kernel/ptrace.c | 104 ++++++++++++++++++++------------ > > 2 files changed, 69 insertions(+), 40 deletions(-) > > > > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h > > index e58bca832dff..0a9552b4f61e 100644 > > --- a/arch/arm64/include/asm/ptrace.h > > +++ b/arch/arm64/include/asm/ptrace.h > > @@ -170,6 +170,11 @@ static inline unsigned long pstate_to_compat_psr(const unsigned long pstate) > > return psr; > > } > > > > +enum ptrace_syscall_dir { > > + PTRACE_SYSCALL_ENTER = 0, > > + PTRACE_SYSCALL_EXIT, > > +}; > > + > > /* > > * This struct defines the way the registers are stored on the stack during an > > * exception. Note that sizeof(struct pt_regs) has to be a multiple of 16 (for > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > > index 8ac487c84e37..39da03104528 100644 > > --- a/arch/arm64/kernel/ptrace.c > > +++ b/arch/arm64/kernel/ptrace.c > > @@ -40,6 +40,7 @@ > > #include > > #include > > #include > > +#include > > > > #define CREATE_TRACE_POINTS > > #include > > @@ -561,7 +562,31 @@ static int gpr_get(struct task_struct *target, > > struct membuf to) > > { > > struct user_pt_regs *uregs = &task_pt_regs(target)->user_regs; > > - return membuf_write(&to, uregs, sizeof(*uregs)); > > + unsigned long saved_reg; > > + int ret; > > + > > + /* > > + * We have some ABI weirdness here in the way that we handle syscall > > + * exit stops because we indicate whether or not the stop has been > > + * signalled from syscall entry or syscall exit by clobbering the general > > + * purpose register x7. > > + */ > > When you move a comment, please don't truncate it! > > > + saved_reg = uregs->regs[7]; > > + > > + switch (target->ptrace_message) { > > + case PTRACE_EVENTMSG_SYSCALL_ENTRY: > > + uregs->regs[7] = PTRACE_SYSCALL_ENTER; > > + break; > > + case PTRACE_EVENTMSG_SYSCALL_EXIT: > > + uregs->regs[7] = PTRACE_SYSCALL_EXIT; > > + break; > > + } > > I'm wary of checking target->ptrace_message here, as I seem to recall the > regset code also being used for coredumps. What guarantees we don't break > things there? For a coredump, is there any way to know whether a given thread was inside a traced syscall when the coredump was generated? If so, x7 in the dump may already unreliable and we only need to make best efforts to get it "right". Since triggering of the coredump and death of other threads all require dequeueing of some signal, I think all threads must always outside the syscall-enter...syscall-exit path before any of the coredump runs anyway, in which case the above should never matter... Though somone else ought to eyeball the coredump code before we agree on that. ptrace_message doesn't seem absolutely the wrong thing to check, but we'd need to be sure that it can't be stale (say, left over from some previous trap). Out of interest, where did this arm64 ptrace feature come from? Was it just pasted from 32-bit and thinly adapted? It looks like an arch-specific attempt to do what PTRACE_O_TRACESYSGOOD does, in which case it may have been obsolete even before it was upstreamed. I wonder whether anyone is actually relying on it at all... Doesn't mean we can definitely fix it safely, but it's annoying. [...] Cheers ---Dave 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.6 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,URIBL_BLOCKED, 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 CB8E4C433E0 for ; Thu, 4 Feb 2021 16:44:42 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 879B164DF5 for ; Thu, 4 Feb 2021 16:44:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 879B164DF5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.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=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=OFMDgK75oTgv2v28z2aB6EAJGbEjTbY27Vg2Zc7kOdM=; b=BcZsKEElqS6w56sZR8aqX/w1V 8dMUAmhaPct2sP2S1jl7Y+GkOWCNr5Xg8Lz6OSGz0guOJIlaSnNtSGmtl/2ElEY3xALBOW9do+KIR PHjLtJLyYsaDSZ3EkB8UoxpRtGGgTZxp77d20ktE2DS16pPx1U0VIRiUkoKbPXa6DcTapUuN4z/W/ nWd5kqOHeKIVTERNQVMftFyEKhNJFcLkomBxSDXo68xvFY3CRu1fWqm2aFb1PjxTp8fEL0YXl33c/ s+MqXvofFZ8DlDUh43plfOJUsEeAZNwieC4jI5Nn1hoRcS6ce61/OCi2biobD8ndUaHqBjql6R/vd UUK5ibBlQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l7hj1-0004eZ-0q; Thu, 04 Feb 2021 16:43:23 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l7hhr-00047l-Ow for linux-arm-kernel@lists.infradead.org; Thu, 04 Feb 2021 16:42:13 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 12BE411D4; Thu, 4 Feb 2021 08:42:08 -0800 (PST) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CFDA33F718; Thu, 4 Feb 2021 08:42:06 -0800 (PST) Date: Thu, 4 Feb 2021 16:41:46 +0000 From: Dave Martin To: Will Deacon Subject: Re: [PATCH 1/3] arm64/ptrace: don't clobber task registers on syscall entry/exit traps Message-ID: <20210204164145.GB21837@arm.com> References: <20210201194012.524831-1-avagin@gmail.com> <20210201194012.524831-2-avagin@gmail.com> <20210204152334.GA21058@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210204152334.GA21058@willie-the-truck> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210204_114212_040426_60C3B1E9 X-CRM114-Status: GOOD ( 35.05 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Anthony Steinhauser , linux-api@vger.kernel.org, Oleg Nesterov , linux-kernel@vger.kernel.org, Keno Fischer , Andrei Vagin , Catalin Marinas , linux-arm-kernel@lists.infradead.org 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 On Thu, Feb 04, 2021 at 03:23:34PM +0000, Will Deacon wrote: > On Mon, Feb 01, 2021 at 11:40:10AM -0800, Andrei Vagin wrote: > > ip/r12 for AArch32 and x7 for AArch64 is used to indicate whether or not > > the stop has been signalled from syscall entry or syscall exit. This > > means that: > > > > - Any writes by the tracer to this register during the stop are > > ignored/discarded. > > > > - The actual value of the register is not available during the stop, > > so the tracer cannot save it and restore it later. > > > > Right now, these registers are clobbered in tracehook_report_syscall. > > This change moves the logic to gpr_get and compat_gpr_get where > > registers are copied into a user-space buffer. > > > > This will allow to change these registers and to introduce a new > > ptrace option to get the full set of registers. > > > > Signed-off-by: Andrei Vagin > > --- > > arch/arm64/include/asm/ptrace.h | 5 ++ > > arch/arm64/kernel/ptrace.c | 104 ++++++++++++++++++++------------ > > 2 files changed, 69 insertions(+), 40 deletions(-) > > > > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h > > index e58bca832dff..0a9552b4f61e 100644 > > --- a/arch/arm64/include/asm/ptrace.h > > +++ b/arch/arm64/include/asm/ptrace.h > > @@ -170,6 +170,11 @@ static inline unsigned long pstate_to_compat_psr(const unsigned long pstate) > > return psr; > > } > > > > +enum ptrace_syscall_dir { > > + PTRACE_SYSCALL_ENTER = 0, > > + PTRACE_SYSCALL_EXIT, > > +}; > > + > > /* > > * This struct defines the way the registers are stored on the stack during an > > * exception. Note that sizeof(struct pt_regs) has to be a multiple of 16 (for > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > > index 8ac487c84e37..39da03104528 100644 > > --- a/arch/arm64/kernel/ptrace.c > > +++ b/arch/arm64/kernel/ptrace.c > > @@ -40,6 +40,7 @@ > > #include > > #include > > #include > > +#include > > > > #define CREATE_TRACE_POINTS > > #include > > @@ -561,7 +562,31 @@ static int gpr_get(struct task_struct *target, > > struct membuf to) > > { > > struct user_pt_regs *uregs = &task_pt_regs(target)->user_regs; > > - return membuf_write(&to, uregs, sizeof(*uregs)); > > + unsigned long saved_reg; > > + int ret; > > + > > + /* > > + * We have some ABI weirdness here in the way that we handle syscall > > + * exit stops because we indicate whether or not the stop has been > > + * signalled from syscall entry or syscall exit by clobbering the general > > + * purpose register x7. > > + */ > > When you move a comment, please don't truncate it! > > > + saved_reg = uregs->regs[7]; > > + > > + switch (target->ptrace_message) { > > + case PTRACE_EVENTMSG_SYSCALL_ENTRY: > > + uregs->regs[7] = PTRACE_SYSCALL_ENTER; > > + break; > > + case PTRACE_EVENTMSG_SYSCALL_EXIT: > > + uregs->regs[7] = PTRACE_SYSCALL_EXIT; > > + break; > > + } > > I'm wary of checking target->ptrace_message here, as I seem to recall the > regset code also being used for coredumps. What guarantees we don't break > things there? For a coredump, is there any way to know whether a given thread was inside a traced syscall when the coredump was generated? If so, x7 in the dump may already unreliable and we only need to make best efforts to get it "right". Since triggering of the coredump and death of other threads all require dequeueing of some signal, I think all threads must always outside the syscall-enter...syscall-exit path before any of the coredump runs anyway, in which case the above should never matter... Though somone else ought to eyeball the coredump code before we agree on that. ptrace_message doesn't seem absolutely the wrong thing to check, but we'd need to be sure that it can't be stale (say, left over from some previous trap). Out of interest, where did this arm64 ptrace feature come from? Was it just pasted from 32-bit and thinly adapted? It looks like an arch-specific attempt to do what PTRACE_O_TRACESYSGOOD does, in which case it may have been obsolete even before it was upstreamed. I wonder whether anyone is actually relying on it at all... Doesn't mean we can definitely fix it safely, but it's annoying. [...] Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel