From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754640AbcGTQrQ (ORCPT ); Wed, 20 Jul 2016 12:47:16 -0400 Received: from foss.arm.com ([217.140.101.70]:43540 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753941AbcGTQrH (ORCPT ); Wed, 20 Jul 2016 12:47:07 -0400 Subject: Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support To: Catalin Marinas References: <1467995754-32508-1-git-send-email-dave.long@linaro.org> <1467995754-32508-5-git-send-email-dave.long@linaro.org> <578FA238.3050206@arm.com> <20160720162827.GF25890@e104818-lin.cambridge.arm.com> Cc: David Long , Huang Shijie , James Morse , Pratyush Anand , Sandeepa Prabhu , Will Deacon , William Cohen , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Steve Capper , Masami Hiramatsu , Li Bin , Jisheng Zhang , Mark Rutland , Daniel Thompson , Vladimir Murzin , Petr Mladek , Ard Biesheuvel , Jens Wiklander , Robin Murphy , Mark Brown , Suzuki K Poulose , Dave P Martin , Andrey Ryabinin , yalin wang , Yang Shi , Zi Shen Lim , John Blackwood , Andrew Morton , =?UTF-8?Q?Alex_Benn=c3=a9e?= , Adam Buchbinder , Christoffer Dall From: Marc Zyngier Organization: ARM Ltd Message-ID: <578FAB02.90707@arm.com> Date: Wed, 20 Jul 2016 17:46:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.7.0 MIME-Version: 1.0 In-Reply-To: <20160720162827.GF25890@e104818-lin.cambridge.arm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20/07/16 17:28, Catalin Marinas wrote: > On Wed, Jul 20, 2016 at 05:09:28PM +0100, Marc Zyngier wrote: >> +static inline unsigned long min_stack_size(unsigned long addr) >> +{ >> + unsigned long size; >> + struct kprobe_ctlblk *ctl; >> + >> + 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, sizeof(ctl->jprobes_stack)); >> +} > > We could drop the local ctl pointer: > > return min(size, sizeof(((struct kprobe_ctlblk *)0)->jprobes_stack)); > > If you add a log, I'll push the patch on top of the kprobes branch. Here you go: ----8<---- >>From 0d120f95b3348e1946d8a789c7147f316c27ea6b Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Wed, 20 Jul 2016 17:36:42 +0100 Subject: [PATCH] arm64: kprobes: Fix overflow when saving stack The MIN_STACK_SIZE macro tries evaluate how much stack space needs to be saved in the jprobes_stack array, sized at 128 bytes. When using the IRQ stack, said macro can happily return up to IRQ_STACK_SIZE, which is 16kB. Mayhem follows. This patch fixes things by getting rid of the crazy macro and limiting the copy to be at most the size of the jprobes_stack array, no matter which stack we're on. Signed-off-by: Marc Zyngier --- arch/arm64/kernel/probes/kprobes.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index 823cf92..87a24f6 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -34,12 +34,6 @@ #include "decode-insn.h" -#define MIN_STACK_SIZE(addr) (on_irq_stack(addr, raw_smp_processor_id()) ? \ - min((unsigned long)IRQ_STACK_SIZE, \ - IRQ_STACK_PTR(raw_smp_processor_id()) - (addr)) : \ - min((unsigned long)MAX_STACK_SIZE, \ - (unsigned long)current_thread_info() + THREAD_START_SP - (addr))) - void jprobe_return_break(void); DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; @@ -48,6 +42,18 @@ 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 */ @@ -495,7 +501,7 @@ int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs) * the argument area. */ memcpy(kcb->jprobes_stack, (void *)stack_ptr, - MIN_STACK_SIZE(stack_ptr)); + min_stack_size(stack_ptr)); instruction_pointer_set(regs, (unsigned long) jp->entry); preempt_disable(); @@ -547,7 +553,7 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs) unpause_graph_tracing(); *regs = kcb->jprobe_saved_regs; memcpy((void *)stack_addr, kcb->jprobes_stack, - MIN_STACK_SIZE(stack_addr)); + min_stack_size(stack_addr)); preempt_enable_no_resched(); return 1; } -- 2.1.4 Thanks, M. -- Jazz is not dead. It just smells funny...