From patchwork Thu Aug 4 04:47:27 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Long X-Patchwork-Id: 73236 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp1196651qga; Wed, 3 Aug 2016 21:49:12 -0700 (PDT) X-Received: by 10.98.66.209 with SMTP id h78mr123152486pfd.11.1470286152898; Wed, 03 Aug 2016 21:49:12 -0700 (PDT) Return-Path: Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id t12si12519687pfj.221.2016.08.03.21.49.12 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 03 Aug 2016 21:49:12 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) client-ip=2001:1868:205::9; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) smtp.mailfrom=linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bVAZh-0004KE-Lz; Thu, 04 Aug 2016 04:48:05 +0000 Received: from mail-qk0-x22d.google.com ([2607:f8b0:400d:c09::22d]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1bVAZa-0004Fl-CT for linux-arm-kernel@lists.infradead.org; Thu, 04 Aug 2016 04:48:00 +0000 Received: by mail-qk0-x22d.google.com with SMTP id p186so96421857qkd.1 for ; Wed, 03 Aug 2016 21:47:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=PdZCPY0H7008fXJgUmiZjKdYPmdioa3nLTlK7og/VBM=; b=a0DQf6Kdhk/CLoWCTYAmN7HiDIzxjCf6wigilY/A9L88dPTcoXJrVihXOb2f7/UeMB P6k0Kn0ITpJ2yaKeCyZisb7YrLe8jT9pD46R0c06Gir8S0nnFGfKD3Vy/i9DcCFcE80e LV3OYEYM1ZR8TnrxuyYfoZLVtEA6F9Nuyuyeo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=PdZCPY0H7008fXJgUmiZjKdYPmdioa3nLTlK7og/VBM=; b=PsRx4fPR/EkT0Wv/2QTWLUhjpZzp9yQoysfqJ8o6U/394mJO79aEBPKfJInFXeMpii V2U/y6jnY/47lvIw+xImhWgqmigSoJPwvyUVp6jp6m60qszXvKm2fw2EYBl34cdx6UGp C6HBWiD3IxjcxpLUFzp0m1zDuQ3xucACjBECbktSQcJIkqIsg4xWFLM36E+xSIUGcOsp RRzxOlcYUJUJzXb7XZl1bJcd5Zbqpgp9IMQocMo9hxuirGICNjtjg2mWJ91Jfvrx7YL7 YsmZt9Sm4gDI8NTwm9j2BeO4QbaixGx8I1Ojy+W0HM1/jo785wsT2xynczPSDFMmMjBi uqVA== X-Gm-Message-State: AEkooutRftFgg9wi6qLBROLlpacworW+CC7h5HoPblbOPWhxL2/IYVL5BQzck/TXR2KXJE3L X-Received: by 10.55.20.90 with SMTP id e87mr3840523qkh.260.1470286056949; Wed, 03 Aug 2016 21:47:36 -0700 (PDT) Received: from [192.168.1.116] (pool-72-71-243-24.cncdnh.fast00.myfairpoint.net. [72.71.243.24]) by smtp.googlemail.com with ESMTPSA id s6sm3865186qkc.42.2016.08.03.21.47.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 03 Aug 2016 21:47:35 -0700 (PDT) Subject: Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support To: Daniel Thompson , Catalin Marinas References: <578FA238.3050206@arm.com> <5790F960.5050007@linaro.org> <57910528.7070902@arm.com> <57911590.50305@linaro.org> <20160722101617.GA17821@e104818-lin.cambridge.arm.com> <57924104.1080202@linaro.org> <20160725171350.GE2423@e104818-lin.cambridge.arm.com> <57969234.1070201@linaro.org> <22b277ba-6812-a0dd-9e8e-c29bdb3aa672@linaro.org> <57993211.1040600@linaro.org> <20160728144053.GA26510@e104818-lin.cambridge.arm.com> <360d582b-5401-7126-ef40-bd78369c0a34@linaro.org> From: David Long Message-ID: <57A2C8DF.7050401@linaro.org> Date: Thu, 4 Aug 2016 00:47:27 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <360d582b-5401-7126-ef40-bd78369c0a34@linaro.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160803_214758_601255_3A62B1DB X-CRM114-Status: GOOD ( 34.76 ) X-Spam-Score: -2.7 (--) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-2.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2607:f8b0:400d:c09:0:0:0:22d listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Petr Mladek , Zi Shen Lim , Will Deacon , Andrey Ryabinin , yalin wang , Li Bin , Jisheng Zhang , John Blackwood , Pratyush Anand , Huang Shijie , Dave P Martin , Yang Shi , Vladimir Murzin , Steve Capper , Suzuki K Poulose , Marc Zyngier , Mark Brown , Sandeepa Prabhu , William Cohen , =?UTF-8?Q?Alex_Benn=c3=a9e?= , Adam Buchbinder , linux-arm-kernel@lists.infradead.org, Ard Biesheuvel , linux-kernel@vger.kernel.org, James Morse , Masami Hiramatsu , Andrew Morton , Robin Murphy , Jens Wiklander , Christoffer Dall Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org On 07/29/2016 05:01 AM, Daniel Thompson wrote: > On 28/07/16 15:40, Catalin Marinas wrote: >> On Wed, Jul 27, 2016 at 06:13:37PM -0400, David Long wrote: >>> On 07/27/2016 07:50 AM, Daniel Thompson wrote: >>>> On 25/07/16 23:27, David Long wrote: >>>>> On 07/25/2016 01:13 PM, Catalin Marinas wrote: >>>>>> The problem is that the original design was done on x86 for its >>>>>> PCS and >>>>>> it doesn't always fit other architectures. So we could either >>>>>> ignore the >>>>>> problem, hoping that no probed function requires argument passing on >>>>>> stack or we copy all the valid data on the kernel stack: >>>>>> >>>>>> diff --git a/arch/arm64/include/asm/kprobes.h >>>>>> b/arch/arm64/include/asm/kprobes.h >>>>>> index 61b49150dfa3..157fd0d0aa08 100644 >>>>>> --- a/arch/arm64/include/asm/kprobes.h >>>>>> +++ b/arch/arm64/include/asm/kprobes.h >>>>>> @@ -22,7 +22,7 @@ >>>>>> >>>>>> #define __ARCH_WANT_KPROBES_INSN_SLOT >>>>>> #define MAX_INSN_SIZE 1 >>>>>> -#define MAX_STACK_SIZE 128 >>>>>> +#define MAX_STACK_SIZE THREAD_SIZE >>>>>> >>>>>> #define flush_insn_slot(p) do { } while (0) >>>>>> #define kretprobe_blacklist_size 0 >>>>> >>>>> I doubt the ARM PCS is unusual. At any rate I'm certain there are >>>>> other >>>>> architectures that pass aggregate parameters on the stack. I suspect >>>>> other RISC(-ish) architectures have similar PCS issues and I think >>>>> this >>>>> is at least a big part of where this simple copy with a 64/128 limit >>>>> comes from, or at least why it continues to exist. That said, I'm not >>>>> enthusiastic about researching that assertion in detail as it could be >>>>> time consuming. >>>> >>>> Given Mark shared a test program I *was* curious enough to take a look >>>> at this. >>>> >>>> The only architecture I can find that behaves like arm64 with the >>>> implicit pass-by-reference described by Catalin/Mark is sparc64. >>>> >>>> In contrast alpha, arm (32-bit), hppa64, mips64 and powerpc64 all use a >>>> hybrid approach where the first fragments of the structure are >>>> passed in >>>> registers and the remainder on the stack. >>> >>> That's interesting. It also looks like sparc64 does not copy any >>> stack for >>> jprobes. I guess that approach at least makes it clear what will and >>> won't >>> work. >> >> I suggest we do the same for arm64 - avoid the copying entirely as it's >> not safe anyway. We don't know how much to copy, nor can we be sure it >> is safe (see Dave's DMA to the stack example). This would need to be >> documented in the kprobes.txt file and MAX_STACK_SIZE removed from the >> arm64 kprobes support. >> >> There is also the case that Daniel was talking about - passing more than >> 8 arguments. I don't think it's worth handling this > > Its actually quite hard to document the (architecture specific) "no big > structures" *and* the "8 argument" limits. It ends up as something like: > > Structures/unions >16 bytes must not be passed by value and the > size of all arguments, after padding each to an 8 byte boundary, must > be less than 64 bytes. > > We cannot avoid tackling big structures through documentation but when > we impose additional limits like "only 8 arguments" we are swapping an > architecture neutral "gotcha" that affects almost all jprobes uses (and > can be inferred from the documentation) with an architecture specific one! > See new patch below. The documentation change in it could use some scrutiny. I've tested with one-off jprobes functions in a test module and I've verified NET_TCPPROBE doesn't cause misbehavior. > > > but we should at >> least add a warning and skip the probe: >> >> diff --git a/arch/arm64/kernel/probes/kprobes.c >> b/arch/arm64/kernel/probes/kprobes.c >> index bf9768588288..84e02606ec3d 100644 >> --- a/arch/arm64/kernel/probes/kprobes.c >> +++ b/arch/arm64/kernel/probes/kprobes.c >> @@ -491,6 +491,10 @@ int __kprobes setjmp_pre_handler(struct kprobe >> *p, struct pt_regs *regs) >> struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); >> long stack_ptr = kernel_stack_pointer(regs); >> >> + /* do not allow arguments passed on the stack */ >> + if (WARN_ON_ONCE(regs->sp != regs->regs[29])) >> + return 0; >> + > > I don't really understand this test. > > If we could reliably assume that the frame record was at the lowest > address within a stack frame then we could exploit that to store the > stacked arguments without risking overwriting volatile variables on the > stack. > > > Daniel. > I'm assuming the consensus is to not use the above snippet of code. Thanks, -dl ----------cut here-------- >From b451caa1adaf1d03e08a44b5dad3fca31cebd97a Mon Sep 17 00:00:00 2001 From: "David A. Long" Date: Thu, 4 Aug 2016 00:35:33 -0400 Subject: [PATCH] arm64: Remove stack duplicating code from jprobes Because the arm64 calling standard allows stacked function arguments to be anywhere in the stack frame, do not attempt to duplicate the stack frame for jprobes handler functions. Signed-off-by: David A. Long --- Documentation/kprobes.txt | 7 +++++++ arch/arm64/include/asm/kprobes.h | 2 -- arch/arm64/kernel/probes/kprobes.c | 31 +++++-------------------------- 3 files changed, 12 insertions(+), 28 deletions(-) -- 2.5.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt index 1f9b3e2..bd01839 100644 --- a/Documentation/kprobes.txt +++ b/Documentation/kprobes.txt @@ -103,6 +103,13 @@ Note that the probed function's args may be passed on the stack or in registers. The jprobe will work in either case, so long as the handler's prototype matches that of the probed function. +Note that in some architectures (e.g.: arm64) the stack copy is not +done, as the actual location of stacked parameters may be outside of +a reasonable MAX_STACK_SIZE value and because that location cannot be +determined by the jprobes code. In this case the jprobes user must be +careful to make certain the calling signature of the function does +not cause parameters to be passed on the stack. + 1.3 Return Probes 1.3.1 How Does a Return Probe Work? diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h index 61b4915..1737aec 100644 --- a/arch/arm64/include/asm/kprobes.h +++ b/arch/arm64/include/asm/kprobes.h @@ -22,7 +22,6 @@ #define __ARCH_WANT_KPROBES_INSN_SLOT #define MAX_INSN_SIZE 1 -#define MAX_STACK_SIZE 128 #define flush_insn_slot(p) do { } while (0) #define kretprobe_blacklist_size 0 @@ -47,7 +46,6 @@ struct kprobe_ctlblk { struct prev_kprobe prev_kprobe; struct kprobe_step_ctx ss_ctx; struct pt_regs jprobe_saved_regs; - char jprobes_stack[MAX_STACK_SIZE]; }; void arch_remove_kprobe(struct kprobe *); diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index bf97685..c6b0f40 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -41,18 +41,6 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); static void __kprobes post_kprobe_handler(struct kprobe_ctlblk *, struct pt_regs *); -static inline unsigned long min_stack_size(unsigned long addr) -{ - unsigned long size; - - if (on_irq_stack(addr, raw_smp_processor_id())) - size = IRQ_STACK_PTR(raw_smp_processor_id()) - addr; - else - size = (unsigned long)current_thread_info() + THREAD_START_SP - addr; - - return min(size, FIELD_SIZEOF(struct kprobe_ctlblk, jprobes_stack)); -} - static void __kprobes arch_prepare_ss_slot(struct kprobe *p) { /* prepare insn slot */ @@ -489,20 +477,15 @@ int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs) { struct jprobe *jp = container_of(p, struct jprobe, kp); struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); - long stack_ptr = kernel_stack_pointer(regs); kcb->jprobe_saved_regs = *regs; /* - * As Linus pointed out, gcc assumes that the callee - * owns the argument space and could overwrite it, e.g. - * tailcall optimization. So, to be absolutely safe - * we also save and restore enough stack bytes to cover - * the argument area. + * Since we can't be sure where in the stack frame "stacked" + * pass-by-value arguments are stored we just don't try to + * duplicate any of the stack. Do not use jprobes on functions that + * use more than 64 bytes (after padding each to an 8 byte boundary) + * of arguments, or pass individual arguments larger than 16 bytes. */ - kasan_disable_current(); - memcpy(kcb->jprobes_stack, (void *)stack_ptr, - min_stack_size(stack_ptr)); - kasan_enable_current(); instruction_pointer_set(regs, (unsigned long) jp->entry); preempt_disable(); @@ -554,10 +537,6 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs) } unpause_graph_tracing(); *regs = kcb->jprobe_saved_regs; - kasan_disable_current(); - memcpy((void *)stack_addr, kcb->jprobes_stack, - min_stack_size(stack_addr)); - kasan_enable_current(); preempt_enable_no_resched(); return 1; }