Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1447325 > unrolled thread

Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Marc Zyngier <marc.zyngier@arm.com> - 2016-07-20 18:10 +0200
    Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Catalin Marinas <catalin.marinas@arm.com> - 2016-07-20 18:30 +0200
      Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Marc Zyngier <marc.zyngier@arm.com> - 2016-07-20 18:40 +0200
      Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Marc Zyngier <marc.zyngier@arm.com> - 2016-07-20 18:50 +0200
        Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Catalin Marinas <catalin.marinas@arm.com> - 2016-07-20 19:10 +0200
    Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support David Long <dave.long@linaro.org> - 2016-07-21 18:40 +0200
      Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Catalin Marinas <catalin.marinas@arm.com> - 2016-07-21 19:20 +0200
      Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Marc Zyngier <marc.zyngier@arm.com> - 2016-07-21 19:30 +0200
        Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support David Long <dave.long@linaro.org> - 2016-07-21 20:40 +0200
          Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Catalin Marinas <catalin.marinas@arm.com> - 2016-07-22 12:20 +0200
            Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support David Long <dave.long@linaro.org> - 2016-07-22 18:00 +0200
              Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Catalin Marinas <catalin.marinas@arm.com> - 2016-07-25 19:20 +0200
                Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support David Long <dave.long@linaro.org> - 2016-07-26 00:30 +0200
                  Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Daniel Thompson <daniel.thompson@linaro.org> - 2016-07-27 14:00 +0200
                    Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support David Long <dave.long@linaro.org> - 2016-07-28 00:20 +0200
                      Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Catalin Marinas <catalin.marinas@arm.com> - 2016-07-28 16:50 +0200
                        Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Daniel Thompson <daniel.thompson@linaro.org> - 2016-07-29 11:10 +0200
                          Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support David Long <dave.long@linaro.org> - 2016-08-04 07:00 +0200
                            Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Daniel Thompson <daniel.thompson@linaro.org> - 2016-08-08 13:20 +0200
                              Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support David Long <dave.long@linaro.org> - 2016-08-08 16:30 +0200
                                Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Masami Hiramatsu <mhiramat@kernel.org> - 2016-08-09 01:00 +0200
                                Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Catalin Marinas <catalin.marinas@arm.com> - 2016-08-09 19:30 +0200
                                  Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support David Long <dave.long@linaro.org> - 2016-08-10 22:50 +0200
                            Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Masami Hiramatsu <mhiramat@kernel.org> - 2016-08-09 00:20 +0200
                Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Daniel Thompson <daniel.thompson@linaro.org> - 2016-07-26 12:00 +0200
                  Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Catalin Marinas <catalin.marinas@arm.com> - 2016-07-26 19:00 +0200
                    Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Dave Martin <Dave.Martin@arm.com> - 2016-07-27 12:10 +0200
                  Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Mark Rutland <mark.rutland@arm.com> - 2016-07-26 20:00 +0200
                    Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Daniel Thompson <daniel.thompson@linaro.org> - 2016-07-27 13:30 +0200
                      Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Dave Martin <Dave.Martin@arm.com> - 2016-07-27 13:40 +0200
                        Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Daniel Thompson <daniel.thompson@linaro.org> - 2016-07-27 13:50 +0200
                      Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Mark Rutland <mark.rutland@arm.com> - 2016-07-27 15:40 +0200

Page 1 of 2  [1] 2  Next page →

#1447325 — Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support

On 08/07/16 17:35, David Long wrote:
> From: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
> 
> Add support for basic kernel probes(kprobes) and jump probes
> (jprobes) for ARM64.
> 
> Kprobes utilizes software breakpoint and single step debug
> exceptions supported on ARM v8.
> 
> A software breakpoint is placed at the probe address to trap the
> kernel execution into the kprobe handler.
> 
> ARM v8 supports enabling single stepping before the break exception
> return (ERET), with next PC in exception return address (ELR_EL1). The
> kprobe handler prepares an executable memory slot for out-of-line
> execution with a copy of the original instruction being probed, and
> enables single stepping. The PC is set to the out-of-line slot address
> before the ERET. With this scheme, the instruction is executed with the
> exact same register context except for the PC (and DAIF) registers.
> 
> Debug mask (PSTATE.D) is enabled only when single stepping a recursive
> kprobe, e.g.: during kprobes reenter so that probed instruction can be
> single stepped within the kprobe handler -exception- context.
> The recursion depth of kprobe is always 2, i.e. upon probe re-entry,
> any further re-entry is prevented by not calling handlers and the case
> counted as a missed kprobe).
> 
> Single stepping from the x-o-l slot has a drawback for PC-relative accesses
> like branching and symbolic literals access as the offset from the new PC
> (slot address) may not be ensured to fit in the immediate value of
> the opcode. Such instructions need simulation, so reject
> probing them.
> 
> Instructions generating exceptions or cpu mode change are rejected
> for probing.
> 
> Exclusive load/store instructions are rejected too.  Additionally, the
> code is checked to see if it is inside an exclusive load/store sequence
> (code from Pratyush).
> 
> System instructions are mostly enabled for stepping, except MSR/MRS
> accesses to "DAIF" flags in PSTATE, which are not safe for
> probing.
> 
> This also changes arch/arm64/include/asm/ptrace.h to use
> include/asm-generic/ptrace.h.
> 
> Thanks to Steve Capper and Pratyush Anand for several suggested
> Changes.
> 
> Signed-off-by: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
> Signed-off-by: David A. Long <dave.long@linaro.org>
> Signed-off-by: Pratyush Anand <panand@redhat.com>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm64/Kconfig                      |   1 +
>  arch/arm64/include/asm/debug-monitors.h |   5 +
>  arch/arm64/include/asm/insn.h           |   2 +
>  arch/arm64/include/asm/kprobes.h        |  60 ++++
>  arch/arm64/include/asm/probes.h         |  34 +++
>  arch/arm64/include/asm/ptrace.h         |  14 +-
>  arch/arm64/kernel/Makefile              |   2 +-
>  arch/arm64/kernel/debug-monitors.c      |  16 +-
>  arch/arm64/kernel/probes/Makefile       |   1 +
>  arch/arm64/kernel/probes/decode-insn.c  | 143 +++++++++
>  arch/arm64/kernel/probes/decode-insn.h  |  34 +++
>  arch/arm64/kernel/probes/kprobes.c      | 525 ++++++++++++++++++++++++++++++++
>  arch/arm64/kernel/vmlinux.lds.S         |   1 +
>  arch/arm64/mm/fault.c                   |  26 ++
>  14 files changed, 859 insertions(+), 5 deletions(-)
>  create mode 100644 arch/arm64/include/asm/kprobes.h
>  create mode 100644 arch/arm64/include/asm/probes.h
>  create mode 100644 arch/arm64/kernel/probes/Makefile
>  create mode 100644 arch/arm64/kernel/probes/decode-insn.c
>  create mode 100644 arch/arm64/kernel/probes/decode-insn.h
>  create mode 100644 arch/arm64/kernel/probes/kprobes.c
> 

[...]

> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
> new file mode 100644
> index 0000000..79c9511
> --- /dev/null
> +++ b/arch/arm64/include/asm/kprobes.h
> @@ -0,0 +1,60 @@
> +/*
> + * arch/arm64/include/asm/kprobes.h
> + *
> + * Copyright (C) 2013 Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#ifndef _ARM_KPROBES_H
> +#define _ARM_KPROBES_H
> +
> +#include <linux/types.h>
> +#include <linux/ptrace.h>
> +#include <linux/percpu.h>
> +
> +#define __ARCH_WANT_KPROBES_INSN_SLOT
> +#define MAX_INSN_SIZE			1
> +#define MAX_STACK_SIZE			128

Where is that value coming from? Because even on my 6502, I have a 256
byte stack.

> +
> +#define flush_insn_slot(p)		do { } while (0)
> +#define kretprobe_blacklist_size	0
> +
> +#include <asm/probes.h>
> +
> +struct prev_kprobe {
> +	struct kprobe *kp;
> +	unsigned int status;
> +};
> +
> +/* Single step context for kprobe */
> +struct kprobe_step_ctx {
> +	unsigned long ss_pending;
> +	unsigned long match_addr;
> +};
> +
> +/* per-cpu kprobe control block */
> +struct kprobe_ctlblk {
> +	unsigned int kprobe_status;
> +	unsigned long saved_irqflag;
> +	struct prev_kprobe prev_kprobe;
> +	struct kprobe_step_ctx ss_ctx;
> +	struct pt_regs jprobe_saved_regs;
> +	char jprobes_stack[MAX_STACK_SIZE];

Yeah, right. Let's keep this array in mind for a second.

> +};
> +
> +void arch_remove_kprobe(struct kprobe *);
> +int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
> +int kprobe_exceptions_notify(struct notifier_block *self,
> +			     unsigned long val, void *data);
> +int kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr);
> +int kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr);
> +
> +#endif /* _ARM_KPROBES_H */
> diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h
> new file mode 100644
> index 0000000..1e8a21a

[...]

> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> new file mode 100644
> index 0000000..4496801
> --- /dev/null
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -0,0 +1,525 @@
> +/*
> + * arch/arm64/kernel/probes/kprobes.c
> + *
> + * Kprobes support for ARM64
> + *
> + * Copyright (C) 2013 Linaro Limited.
> + * Author: Sandeepa Prabhu <sandeepa.prabhu@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/kprobes.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/stop_machine.h>
> +#include <linux/stringify.h>
> +#include <asm/traps.h>
> +#include <asm/ptrace.h>
> +#include <asm/cacheflush.h>
> +#include <asm/debug-monitors.h>
> +#include <asm/system_misc.h>
> +#include <asm/insn.h>
> +#include <asm/uaccess.h>
> +#include <asm/irq.h>
> +
> +#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)))

This macro makes me want to throw things at people, because there is no
way it can be reasonable parsed. So I've converted it to:

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 823cf92..5ee9c54 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -34,11 +34,23 @@
 
 #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)))
+static unsigned long min_stack_size(unsigned long addr)
+{
+	unsigned long max_size;
+	unsigned long size;
+
+	if (on_irq_stack(addr, raw_smp_processor_id())) {
+		max_size = IRQ_STACK_SIZE;
+		size = IRQ_STACK_PTR(raw_smp_processor_id()) - addr;
+	} else {
+		max_size = MAX_STACK_SIZE;
+		size = (unsigned long)current_thread_info() + THREAD_START_SP - addr;
+	}
+
+	return min(size, max_size);
+}
+
+#define MIN_STACK_SIZE(addr)	min_stack_size(addr)
 
 void jprobe_return_break(void);
 
And then you can instrument it. If you add a simple printk to dump how
much you're going to copy, you get:

root@10:/# nc -l -p 8080
size = 1248
size = 1248
Bad mode in Synchronous Abort handler detected on CPU0, code 0x86000006 -- IABT (current EL)
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.7.0-rc7-next-20160719-00068-g80315b6-dirty #6265
Hardware name: linux,dummy-virt (DT)
task: ffff000009020280 task.stack: ffff000009010000
PC is at 0x4000
LR is at enqueue_task_fair+0x8d8/0x1568
pc : [<0000000000004000>] lr : [<ffff000008101c78>] pstate: 200001c5
sp : ffff8000fffad7d0

Yes, 1248 bytes. How is that supposed to work?

So I've rewritten it like this:

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 823cf92..194a679 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -34,11 +34,20 @@
 
 #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)))
+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));
+}
+
+#define MIN_STACK_SIZE(addr)	min_stack_size(addr)
 
 void jprobe_return_break(void);
 
 
I'm not sure if these 128 bytes are the right size for this thing,
but at least it won't blindly take the kernel down.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [next] | [standalone]

#1447346

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.

Thanks.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1447349

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.

Sure, I'll write that now.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1447351

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 <marc.zyngier@arm.com>
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 <marc.zyngier@arm.com>
---
 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...

[toc] | [prev] | [next] | [standalone]

#1447365

On Wed, Jul 20, 2016 at 05:46:58PM +0100, Marc Zyngier wrote:
> From 0d120f95b3348e1946d8a789c7147f316c27ea6b Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> 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 <marc.zyngier@arm.com>

Applied to the kprobes branch. Thanks.

(I can't yet tell whether kprobes will make into 4.8; I need to run some
more tests before deciding)

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1448000

On 07/20/2016 12:09 PM, Marc Zyngier wrote:
> On 08/07/16 17:35, David Long wrote:
>> From: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
>>
>> Add support for basic kernel probes(kprobes) and jump probes
>> (jprobes) for ARM64.
>>
>> Kprobes utilizes software breakpoint and single step debug
>> exceptions supported on ARM v8.
>>
>> A software breakpoint is placed at the probe address to trap the
>> kernel execution into the kprobe handler.
>>
>> ARM v8 supports enabling single stepping before the break exception
>> return (ERET), with next PC in exception return address (ELR_EL1). The
>> kprobe handler prepares an executable memory slot for out-of-line
>> execution with a copy of the original instruction being probed, and
>> enables single stepping. The PC is set to the out-of-line slot address
>> before the ERET. With this scheme, the instruction is executed with the
>> exact same register context except for the PC (and DAIF) registers.
>>
>> Debug mask (PSTATE.D) is enabled only when single stepping a recursive
>> kprobe, e.g.: during kprobes reenter so that probed instruction can be
>> single stepped within the kprobe handler -exception- context.
>> The recursion depth of kprobe is always 2, i.e. upon probe re-entry,
>> any further re-entry is prevented by not calling handlers and the case
>> counted as a missed kprobe).
>>
>> Single stepping from the x-o-l slot has a drawback for PC-relative accesses
>> like branching and symbolic literals access as the offset from the new PC
>> (slot address) may not be ensured to fit in the immediate value of
>> the opcode. Such instructions need simulation, so reject
>> probing them.
>>
>> Instructions generating exceptions or cpu mode change are rejected
>> for probing.
>>
>> Exclusive load/store instructions are rejected too.  Additionally, the
>> code is checked to see if it is inside an exclusive load/store sequence
>> (code from Pratyush).
>>
>> System instructions are mostly enabled for stepping, except MSR/MRS
>> accesses to "DAIF" flags in PSTATE, which are not safe for
>> probing.
>>
>> This also changes arch/arm64/include/asm/ptrace.h to use
>> include/asm-generic/ptrace.h.
>>
>> Thanks to Steve Capper and Pratyush Anand for several suggested
>> Changes.
>>
>> Signed-off-by: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
>> Signed-off-by: David A. Long <dave.long@linaro.org>
>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
>> ---
>>   arch/arm64/Kconfig                      |   1 +
>>   arch/arm64/include/asm/debug-monitors.h |   5 +
>>   arch/arm64/include/asm/insn.h           |   2 +
>>   arch/arm64/include/asm/kprobes.h        |  60 ++++
>>   arch/arm64/include/asm/probes.h         |  34 +++
>>   arch/arm64/include/asm/ptrace.h         |  14 +-
>>   arch/arm64/kernel/Makefile              |   2 +-
>>   arch/arm64/kernel/debug-monitors.c      |  16 +-
>>   arch/arm64/kernel/probes/Makefile       |   1 +
>>   arch/arm64/kernel/probes/decode-insn.c  | 143 +++++++++
>>   arch/arm64/kernel/probes/decode-insn.h  |  34 +++
>>   arch/arm64/kernel/probes/kprobes.c      | 525 ++++++++++++++++++++++++++++++++
>>   arch/arm64/kernel/vmlinux.lds.S         |   1 +
>>   arch/arm64/mm/fault.c                   |  26 ++
>>   14 files changed, 859 insertions(+), 5 deletions(-)
>>   create mode 100644 arch/arm64/include/asm/kprobes.h
>>   create mode 100644 arch/arm64/include/asm/probes.h
>>   create mode 100644 arch/arm64/kernel/probes/Makefile
>>   create mode 100644 arch/arm64/kernel/probes/decode-insn.c
>>   create mode 100644 arch/arm64/kernel/probes/decode-insn.h
>>   create mode 100644 arch/arm64/kernel/probes/kprobes.c
>>
>
> [...]
>
>> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
>> new file mode 100644
>> index 0000000..79c9511
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/kprobes.h
>> @@ -0,0 +1,60 @@
>> +/*
>> + * arch/arm64/include/asm/kprobes.h
>> + *
>> + * Copyright (C) 2013 Linaro Limited
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +
>> +#ifndef _ARM_KPROBES_H
>> +#define _ARM_KPROBES_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/ptrace.h>
>> +#include <linux/percpu.h>
>> +
>> +#define __ARCH_WANT_KPROBES_INSN_SLOT
>> +#define MAX_INSN_SIZE			1
>> +#define MAX_STACK_SIZE			128
>
> Where is that value coming from? Because even on my 6502, I have a 256
> byte stack.
>

Although I don't claim to know the original author's thoughts I would 
guess it is based on the seven other existing implementations for 
kprobes on various architectures, all of which appear to use either 64 
or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the 
whole stack.

>> +
>> +#define flush_insn_slot(p)		do { } while (0)
>> +#define kretprobe_blacklist_size	0
>> +
>> +#include <asm/probes.h>
>> +
>> +struct prev_kprobe {
>> +	struct kprobe *kp;
>> +	unsigned int status;
>> +};
>> +
>> +/* Single step context for kprobe */
>> +struct kprobe_step_ctx {
>> +	unsigned long ss_pending;
>> +	unsigned long match_addr;
>> +};
>> +
>> +/* per-cpu kprobe control block */
>> +struct kprobe_ctlblk {
>> +	unsigned int kprobe_status;
>> +	unsigned long saved_irqflag;
>> +	struct prev_kprobe prev_kprobe;
>> +	struct kprobe_step_ctx ss_ctx;
>> +	struct pt_regs jprobe_saved_regs;
>> +	char jprobes_stack[MAX_STACK_SIZE];
>
> Yeah, right. Let's keep this array in mind for a second.
>
>> +};
>> +
>> +void arch_remove_kprobe(struct kprobe *);
>> +int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
>> +int kprobe_exceptions_notify(struct notifier_block *self,
>> +			     unsigned long val, void *data);
>> +int kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr);
>> +int kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr);
>> +
>> +#endif /* _ARM_KPROBES_H */
>> diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h
>> new file mode 100644
>> index 0000000..1e8a21a
>
> [...]
>
>> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
>> new file mode 100644
>> index 0000000..4496801
>> --- /dev/null
>> +++ b/arch/arm64/kernel/probes/kprobes.c
>> @@ -0,0 +1,525 @@
>> +/*
>> + * arch/arm64/kernel/probes/kprobes.c
>> + *
>> + * Kprobes support for ARM64
>> + *
>> + * Copyright (C) 2013 Linaro Limited.
>> + * Author: Sandeepa Prabhu <sandeepa.prabhu@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + *
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/kprobes.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/stop_machine.h>
>> +#include <linux/stringify.h>
>> +#include <asm/traps.h>
>> +#include <asm/ptrace.h>
>> +#include <asm/cacheflush.h>
>> +#include <asm/debug-monitors.h>
>> +#include <asm/system_misc.h>
>> +#include <asm/insn.h>
>> +#include <asm/uaccess.h>
>> +#include <asm/irq.h>
>> +
>> +#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)))
>
> This macro makes me want to throw things at people, because there is no
> way it can be reasonable parsed. So I've converted it to:
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 823cf92..5ee9c54 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -34,11 +34,23 @@
>
>   #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)))
> +static unsigned long min_stack_size(unsigned long addr)
> +{
> +	unsigned long max_size;
> +	unsigned long size;
> +
> +	if (on_irq_stack(addr, raw_smp_processor_id())) {
> +		max_size = IRQ_STACK_SIZE;
> +		size = IRQ_STACK_PTR(raw_smp_processor_id()) - addr;
> +	} else {
> +		max_size = MAX_STACK_SIZE;
> +		size = (unsigned long)current_thread_info() + THREAD_START_SP - addr;
> +	}
> +
> +	return min(size, max_size);
> +}
> +
> +#define MIN_STACK_SIZE(addr)	min_stack_size(addr)
>
>   void jprobe_return_break(void);
>
> And then you can instrument it. If you add a simple printk to dump how
> much you're going to copy, you get:
>
> root@10:/# nc -l -p 8080
> size = 1248
> size = 1248
> Bad mode in Synchronous Abort handler detected on CPU0, code 0x86000006 -- IABT (current EL)
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.7.0-rc7-next-20160719-00068-g80315b6-dirty #6265
> Hardware name: linux,dummy-virt (DT)
> task: ffff000009020280 task.stack: ffff000009010000
> PC is at 0x4000
> LR is at enqueue_task_fair+0x8d8/0x1568
> pc : [<0000000000004000>] lr : [<ffff000008101c78>] pstate: 200001c5
> sp : ffff8000fffad7d0
>
> Yes, 1248 bytes. How is that supposed to work?
>
> So I've rewritten it like this:
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 823cf92..194a679 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -34,11 +34,20 @@
>
>   #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)))
> +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));
> +}
> +
> +#define MIN_STACK_SIZE(addr)	min_stack_size(addr)
>
>   void jprobe_return_break(void);
>
>
> I'm not sure if these 128 bytes are the right size for this thing,
> but at least it won't blindly take the kernel down.
>
> Thanks,
>
> 	M.
>

Thanks for finding and fixing this bug.

-dl

[toc] | [prev] | [next] | [standalone]

#1448021

On Thu, Jul 21, 2016 at 12:33:36PM -0400, David Long wrote:
> On 07/20/2016 12:09 PM, Marc Zyngier wrote:
> >On 08/07/16 17:35, David Long wrote:
> >>diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
> >>new file mode 100644
> >>index 0000000..79c9511
> >>--- /dev/null
> >>+++ b/arch/arm64/include/asm/kprobes.h
> >>@@ -0,0 +1,60 @@
> >>+/*
> >>+ * arch/arm64/include/asm/kprobes.h
> >>+ *
> >>+ * Copyright (C) 2013 Linaro Limited
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify
> >>+ * it under the terms of the GNU General Public License version 2 as
> >>+ * published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>+ * General Public License for more details.
> >>+ */
> >>+
> >>+#ifndef _ARM_KPROBES_H
> >>+#define _ARM_KPROBES_H
> >>+
> >>+#include <linux/types.h>
> >>+#include <linux/ptrace.h>
> >>+#include <linux/percpu.h>
> >>+
> >>+#define __ARCH_WANT_KPROBES_INSN_SLOT
> >>+#define MAX_INSN_SIZE			1
> >>+#define MAX_STACK_SIZE			128
> >
> >Where is that value coming from? Because even on my 6502, I have a 256
> >byte stack.
> 
> Although I don't claim to know the original author's thoughts I would guess
> it is based on the seven other existing implementations for kprobes on
> various architectures, all of which appear to use either 64 or 128 for
> MAX_STACK_SIZE.  The code is not trying to duplicate the whole stack.

This seems to be some random choice in the hope that arguments passed on
the stack cannot exceed MAX_STACK_SIZE.

I'll put a patch together which checks the previous stack frame and
limit the copying. If the fp information or if the size to be copied
exceeds MAX_STACK_SIZE, just skip the probe hook (return 0 in
setjmp_pre_handler).

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1448023

On 21/07/16 17:33, David Long wrote:
> On 07/20/2016 12:09 PM, Marc Zyngier wrote:
>> On 08/07/16 17:35, David Long wrote:
>>> From: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
>>>
>>> Add support for basic kernel probes(kprobes) and jump probes
>>> (jprobes) for ARM64.
>>>
>>> Kprobes utilizes software breakpoint and single step debug
>>> exceptions supported on ARM v8.
>>>
>>> A software breakpoint is placed at the probe address to trap the
>>> kernel execution into the kprobe handler.
>>>
>>> ARM v8 supports enabling single stepping before the break exception
>>> return (ERET), with next PC in exception return address (ELR_EL1). The
>>> kprobe handler prepares an executable memory slot for out-of-line
>>> execution with a copy of the original instruction being probed, and
>>> enables single stepping. The PC is set to the out-of-line slot address
>>> before the ERET. With this scheme, the instruction is executed with the
>>> exact same register context except for the PC (and DAIF) registers.
>>>
>>> Debug mask (PSTATE.D) is enabled only when single stepping a recursive
>>> kprobe, e.g.: during kprobes reenter so that probed instruction can be
>>> single stepped within the kprobe handler -exception- context.
>>> The recursion depth of kprobe is always 2, i.e. upon probe re-entry,
>>> any further re-entry is prevented by not calling handlers and the case
>>> counted as a missed kprobe).
>>>
>>> Single stepping from the x-o-l slot has a drawback for PC-relative accesses
>>> like branching and symbolic literals access as the offset from the new PC
>>> (slot address) may not be ensured to fit in the immediate value of
>>> the opcode. Such instructions need simulation, so reject
>>> probing them.
>>>
>>> Instructions generating exceptions or cpu mode change are rejected
>>> for probing.
>>>
>>> Exclusive load/store instructions are rejected too.  Additionally, the
>>> code is checked to see if it is inside an exclusive load/store sequence
>>> (code from Pratyush).
>>>
>>> System instructions are mostly enabled for stepping, except MSR/MRS
>>> accesses to "DAIF" flags in PSTATE, which are not safe for
>>> probing.
>>>
>>> This also changes arch/arm64/include/asm/ptrace.h to use
>>> include/asm-generic/ptrace.h.
>>>
>>> Thanks to Steve Capper and Pratyush Anand for several suggested
>>> Changes.
>>>
>>> Signed-off-by: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
>>> Signed-off-by: David A. Long <dave.long@linaro.org>
>>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>>> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
>>> ---
>>>   arch/arm64/Kconfig                      |   1 +
>>>   arch/arm64/include/asm/debug-monitors.h |   5 +
>>>   arch/arm64/include/asm/insn.h           |   2 +
>>>   arch/arm64/include/asm/kprobes.h        |  60 ++++
>>>   arch/arm64/include/asm/probes.h         |  34 +++
>>>   arch/arm64/include/asm/ptrace.h         |  14 +-
>>>   arch/arm64/kernel/Makefile              |   2 +-
>>>   arch/arm64/kernel/debug-monitors.c      |  16 +-
>>>   arch/arm64/kernel/probes/Makefile       |   1 +
>>>   arch/arm64/kernel/probes/decode-insn.c  | 143 +++++++++
>>>   arch/arm64/kernel/probes/decode-insn.h  |  34 +++
>>>   arch/arm64/kernel/probes/kprobes.c      | 525 ++++++++++++++++++++++++++++++++
>>>   arch/arm64/kernel/vmlinux.lds.S         |   1 +
>>>   arch/arm64/mm/fault.c                   |  26 ++
>>>   14 files changed, 859 insertions(+), 5 deletions(-)
>>>   create mode 100644 arch/arm64/include/asm/kprobes.h
>>>   create mode 100644 arch/arm64/include/asm/probes.h
>>>   create mode 100644 arch/arm64/kernel/probes/Makefile
>>>   create mode 100644 arch/arm64/kernel/probes/decode-insn.c
>>>   create mode 100644 arch/arm64/kernel/probes/decode-insn.h
>>>   create mode 100644 arch/arm64/kernel/probes/kprobes.c
>>>
>>
>> [...]
>>
>>> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
>>> new file mode 100644
>>> index 0000000..79c9511
>>> --- /dev/null
>>> +++ b/arch/arm64/include/asm/kprobes.h
>>> @@ -0,0 +1,60 @@
>>> +/*
>>> + * arch/arm64/include/asm/kprobes.h
>>> + *
>>> + * Copyright (C) 2013 Linaro Limited
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * General Public License for more details.
>>> + */
>>> +
>>> +#ifndef _ARM_KPROBES_H
>>> +#define _ARM_KPROBES_H
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/ptrace.h>
>>> +#include <linux/percpu.h>
>>> +
>>> +#define __ARCH_WANT_KPROBES_INSN_SLOT
>>> +#define MAX_INSN_SIZE			1
>>> +#define MAX_STACK_SIZE			128
>>
>> Where is that value coming from? Because even on my 6502, I have a 256
>> byte stack.
>>
> 
> Although I don't claim to know the original author's thoughts I would 
> guess it is based on the seven other existing implementations for 
> kprobes on various architectures, all of which appear to use either 64 
> or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the 
> whole stack.

I get that (this was supposed to be a humorous comment, but I guess
after spending too much time tracking this thing, my own sense of humour
was becoming limited).

My main worry is that whatever value you pick, it is always going to be
wrong. This is used to preserve arguments that are passed on the stack,
as opposed to passed by registers). We have no idea of what is getting
passed there so saving nothing, 128 bytes or 2kB is about the same. It
is always wrong.

A much better solution would be to check the frame pointer, and copy the
delta between FP and SP, assuming it fits inside the allocated buffer.
If it doesn't, or if FP is invalid, we just skip the hook, because we
can't reliably execute it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1448056

On 07/21/2016 01:23 PM, Marc Zyngier wrote:
> On 21/07/16 17:33, David Long wrote:
>> On 07/20/2016 12:09 PM, Marc Zyngier wrote:
>>> On 08/07/16 17:35, David Long wrote:
>>>> From: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
>>>>
>>>> Add support for basic kernel probes(kprobes) and jump probes
>>>> (jprobes) for ARM64.
>>>>
>>>> Kprobes utilizes software breakpoint and single step debug
>>>> exceptions supported on ARM v8.
>>>>
>>>> A software breakpoint is placed at the probe address to trap the
>>>> kernel execution into the kprobe handler.
>>>>
>>>> ARM v8 supports enabling single stepping before the break exception
>>>> return (ERET), with next PC in exception return address (ELR_EL1). The
>>>> kprobe handler prepares an executable memory slot for out-of-line
>>>> execution with a copy of the original instruction being probed, and
>>>> enables single stepping. The PC is set to the out-of-line slot address
>>>> before the ERET. With this scheme, the instruction is executed with the
>>>> exact same register context except for the PC (and DAIF) registers.
>>>>
>>>> Debug mask (PSTATE.D) is enabled only when single stepping a recursive
>>>> kprobe, e.g.: during kprobes reenter so that probed instruction can be
>>>> single stepped within the kprobe handler -exception- context.
>>>> The recursion depth of kprobe is always 2, i.e. upon probe re-entry,
>>>> any further re-entry is prevented by not calling handlers and the case
>>>> counted as a missed kprobe).
>>>>
>>>> Single stepping from the x-o-l slot has a drawback for PC-relative accesses
>>>> like branching and symbolic literals access as the offset from the new PC
>>>> (slot address) may not be ensured to fit in the immediate value of
>>>> the opcode. Such instructions need simulation, so reject
>>>> probing them.
>>>>
>>>> Instructions generating exceptions or cpu mode change are rejected
>>>> for probing.
>>>>
>>>> Exclusive load/store instructions are rejected too.  Additionally, the
>>>> code is checked to see if it is inside an exclusive load/store sequence
>>>> (code from Pratyush).
>>>>
>>>> System instructions are mostly enabled for stepping, except MSR/MRS
>>>> accesses to "DAIF" flags in PSTATE, which are not safe for
>>>> probing.
>>>>
>>>> This also changes arch/arm64/include/asm/ptrace.h to use
>>>> include/asm-generic/ptrace.h.
>>>>
>>>> Thanks to Steve Capper and Pratyush Anand for several suggested
>>>> Changes.
>>>>
>>>> Signed-off-by: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
>>>> Signed-off-by: David A. Long <dave.long@linaro.org>
>>>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>>>> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
>>>> ---
>>>>    arch/arm64/Kconfig                      |   1 +
>>>>    arch/arm64/include/asm/debug-monitors.h |   5 +
>>>>    arch/arm64/include/asm/insn.h           |   2 +
>>>>    arch/arm64/include/asm/kprobes.h        |  60 ++++
>>>>    arch/arm64/include/asm/probes.h         |  34 +++
>>>>    arch/arm64/include/asm/ptrace.h         |  14 +-
>>>>    arch/arm64/kernel/Makefile              |   2 +-
>>>>    arch/arm64/kernel/debug-monitors.c      |  16 +-
>>>>    arch/arm64/kernel/probes/Makefile       |   1 +
>>>>    arch/arm64/kernel/probes/decode-insn.c  | 143 +++++++++
>>>>    arch/arm64/kernel/probes/decode-insn.h  |  34 +++
>>>>    arch/arm64/kernel/probes/kprobes.c      | 525 ++++++++++++++++++++++++++++++++
>>>>    arch/arm64/kernel/vmlinux.lds.S         |   1 +
>>>>    arch/arm64/mm/fault.c                   |  26 ++
>>>>    14 files changed, 859 insertions(+), 5 deletions(-)
>>>>    create mode 100644 arch/arm64/include/asm/kprobes.h
>>>>    create mode 100644 arch/arm64/include/asm/probes.h
>>>>    create mode 100644 arch/arm64/kernel/probes/Makefile
>>>>    create mode 100644 arch/arm64/kernel/probes/decode-insn.c
>>>>    create mode 100644 arch/arm64/kernel/probes/decode-insn.h
>>>>    create mode 100644 arch/arm64/kernel/probes/kprobes.c
>>>>
>>>
>>> [...]
>>>
>>>> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
>>>> new file mode 100644
>>>> index 0000000..79c9511
>>>> --- /dev/null
>>>> +++ b/arch/arm64/include/asm/kprobes.h
>>>> @@ -0,0 +1,60 @@
>>>> +/*
>>>> + * arch/arm64/include/asm/kprobes.h
>>>> + *
>>>> + * Copyright (C) 2013 Linaro Limited
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * General Public License for more details.
>>>> + */
>>>> +
>>>> +#ifndef _ARM_KPROBES_H
>>>> +#define _ARM_KPROBES_H
>>>> +
>>>> +#include <linux/types.h>
>>>> +#include <linux/ptrace.h>
>>>> +#include <linux/percpu.h>
>>>> +
>>>> +#define __ARCH_WANT_KPROBES_INSN_SLOT
>>>> +#define MAX_INSN_SIZE			1
>>>> +#define MAX_STACK_SIZE			128
>>>
>>> Where is that value coming from? Because even on my 6502, I have a 256
>>> byte stack.
>>>
>>
>> Although I don't claim to know the original author's thoughts I would
>> guess it is based on the seven other existing implementations for
>> kprobes on various architectures, all of which appear to use either 64
>> or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the
>> whole stack.
>
> I get that (this was supposed to be a humorous comment, but I guess
> after spending too much time tracking this thing, my own sense of humour
> was becoming limited).
>

It was only meant to be factual.

> My main worry is that whatever value you pick, it is always going to be
> wrong. This is used to preserve arguments that are passed on the stack,
> as opposed to passed by registers). We have no idea of what is getting
> passed there so saving nothing, 128 bytes or 2kB is about the same. It
> is always wrong.
 >
> A much better solution would be to check the frame pointer, and copy the
> delta between FP and SP, assuming it fits inside the allocated buffer.
> If it doesn't, or if FP is invalid, we just skip the hook, because we
> can't reliably execute it.

Well, this is the way it works literally everywhere else. It is a 
documented limitation (Documentation/kprobes.txt). Said documentation 
may need to be changed along with the suggested fix.

While it might be nice if there were less of a limitation it doesn't 
feel wise to me to be making this change at this time. It feels like an 
enhancement to consider amongst future improvements for all architectures.

>
> Thanks,
>
> 	M.
>

Thanks,
-dl

[toc] | [prev] | [next] | [standalone]

#1448520

On Thu, Jul 21, 2016 at 02:33:52PM -0400, David Long wrote:
> On 07/21/2016 01:23 PM, Marc Zyngier wrote:
> >On 21/07/16 17:33, David Long wrote:
> >>On 07/20/2016 12:09 PM, Marc Zyngier wrote:
> >>>On 08/07/16 17:35, David Long wrote:
> >>>>+#define MAX_INSN_SIZE			1
> >>>>+#define MAX_STACK_SIZE			128
> >>>
> >>>Where is that value coming from? Because even on my 6502, I have a 256
> >>>byte stack.
> >>>
> >>
> >>Although I don't claim to know the original author's thoughts I would
> >>guess it is based on the seven other existing implementations for
> >>kprobes on various architectures, all of which appear to use either 64
> >>or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the
> >>whole stack.
[...]
> >My main worry is that whatever value you pick, it is always going to be
> >wrong. This is used to preserve arguments that are passed on the stack,
> >as opposed to passed by registers). We have no idea of what is getting
> >passed there so saving nothing, 128 bytes or 2kB is about the same. It
> >is always wrong.
> >
> >A much better solution would be to check the frame pointer, and copy the
> >delta between FP and SP, assuming it fits inside the allocated buffer.
> >If it doesn't, or if FP is invalid, we just skip the hook, because we
> >can't reliably execute it.
> 
> Well, this is the way it works literally everywhere else. It is a documented
> limitation (Documentation/kprobes.txt). Said documentation may need to be
> changed along with the suggested fix.

The document states: "Up to MAX_STACK_SIZE bytes are copied". That means
the arch code could always copy less but never more than MAX_STACK_SIZE.
What we are proposing is that we should try to guess how much to copy
based on the FP value (caller's frame) and, if larger than
MAX_STACK_SIZE, skip the probe hook entirely. I don't think this goes
against the kprobes.txt document but at least it (a) may improve the
performance slightly by avoiding unnecessary copy and (b) it avoids
undefined behaviour if we ever encounter a jprobe with arguments passed
on the stack beyond MAX_STACK_SIZE.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1448650

On 07/22/2016 06:16 AM, Catalin Marinas wrote:
> On Thu, Jul 21, 2016 at 02:33:52PM -0400, David Long wrote:
>> On 07/21/2016 01:23 PM, Marc Zyngier wrote:
>>> On 21/07/16 17:33, David Long wrote:
>>>> On 07/20/2016 12:09 PM, Marc Zyngier wrote:
>>>>> On 08/07/16 17:35, David Long wrote:
>>>>>> +#define MAX_INSN_SIZE			1
>>>>>> +#define MAX_STACK_SIZE			128
>>>>>
>>>>> Where is that value coming from? Because even on my 6502, I have a 256
>>>>> byte stack.
>>>>>
>>>>
>>>> Although I don't claim to know the original author's thoughts I would
>>>> guess it is based on the seven other existing implementations for
>>>> kprobes on various architectures, all of which appear to use either 64
>>>> or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the
>>>> whole stack.
> [...]
>>> My main worry is that whatever value you pick, it is always going to be
>>> wrong. This is used to preserve arguments that are passed on the stack,
>>> as opposed to passed by registers). We have no idea of what is getting
>>> passed there so saving nothing, 128 bytes or 2kB is about the same. It
>>> is always wrong.
>>>
>>> A much better solution would be to check the frame pointer, and copy the
>>> delta between FP and SP, assuming it fits inside the allocated buffer.
>>> If it doesn't, or if FP is invalid, we just skip the hook, because we
>>> can't reliably execute it.
>>
>> Well, this is the way it works literally everywhere else. It is a documented
>> limitation (Documentation/kprobes.txt). Said documentation may need to be
>> changed along with the suggested fix.
>
> The document states: "Up to MAX_STACK_SIZE bytes are copied". That means
> the arch code could always copy less but never more than MAX_STACK_SIZE.
> What we are proposing is that we should try to guess how much to copy
> based on the FP value (caller's frame) and, if larger than
> MAX_STACK_SIZE, skip the probe hook entirely. I don't think this goes
> against the kprobes.txt document but at least it (a) may improve the
> performance slightly by avoiding unnecessary copy and (b) it avoids
> undefined behaviour if we ever encounter a jprobe with arguments passed
> on the stack beyond MAX_STACK_SIZE.
>

OK, it sounds like an improvement. I do worry a little about unexpected 
side effects.  I'm just asking if we can accept the existing code as now 
complete enough (in that I believe it matches the other implementations) 
and make this enhancement something for the next release cycle, allowing 
the existing code to be exercised by a wider audience and providing 
ample time to test the new modification? I'd hate to get stuck in a mode 
where this patch gets repeatedly delayed for changes that go above and 
beyond the original design.

Thanks,
-dl

[toc] | [prev] | [next] | [standalone]

#1449664

On Fri, Jul 22, 2016 at 11:51:32AM -0400, David Long wrote:
> On 07/22/2016 06:16 AM, Catalin Marinas wrote:
> >On Thu, Jul 21, 2016 at 02:33:52PM -0400, David Long wrote:
> >>On 07/21/2016 01:23 PM, Marc Zyngier wrote:
> >>>On 21/07/16 17:33, David Long wrote:
> >>>>On 07/20/2016 12:09 PM, Marc Zyngier wrote:
> >>>>>On 08/07/16 17:35, David Long wrote:
> >>>>>>+#define MAX_INSN_SIZE			1
> >>>>>>+#define MAX_STACK_SIZE			128
> >>>>>
> >>>>>Where is that value coming from? Because even on my 6502, I have a 256
> >>>>>byte stack.
> >>>>>
> >>>>
> >>>>Although I don't claim to know the original author's thoughts I would
> >>>>guess it is based on the seven other existing implementations for
> >>>>kprobes on various architectures, all of which appear to use either 64
> >>>>or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the
> >>>>whole stack.
> >[...]
> >>>My main worry is that whatever value you pick, it is always going to be
> >>>wrong. This is used to preserve arguments that are passed on the stack,
> >>>as opposed to passed by registers). We have no idea of what is getting
> >>>passed there so saving nothing, 128 bytes or 2kB is about the same. It
> >>>is always wrong.
> >>>
> >>>A much better solution would be to check the frame pointer, and copy the
> >>>delta between FP and SP, assuming it fits inside the allocated buffer.
> >>>If it doesn't, or if FP is invalid, we just skip the hook, because we
> >>>can't reliably execute it.
> >>
> >>Well, this is the way it works literally everywhere else. It is a documented
> >>limitation (Documentation/kprobes.txt). Said documentation may need to be
> >>changed along with the suggested fix.
> >
> >The document states: "Up to MAX_STACK_SIZE bytes are copied". That means
> >the arch code could always copy less but never more than MAX_STACK_SIZE.
> >What we are proposing is that we should try to guess how much to copy
> >based on the FP value (caller's frame) and, if larger than
> >MAX_STACK_SIZE, skip the probe hook entirely. I don't think this goes
> >against the kprobes.txt document but at least it (a) may improve the
> >performance slightly by avoiding unnecessary copy and (b) it avoids
> >undefined behaviour if we ever encounter a jprobe with arguments passed
> >on the stack beyond MAX_STACK_SIZE.
> 
> OK, it sounds like an improvement. I do worry a little about unexpected side
> effects.

You get more unexpected side effects by not saving/restoring the whole
stack. We looked into this on Friday and came to the conclusion that
there is no safe way for kprobes to know which arguments passed on the
stack should be preserved, at least not with the current API.

Basically the AArch64 PCS states that for arguments passed on the stack
(e.g. they can't fit in registers), the caller allocates memory for them
(on its own stack) and passes the pointer to the callee. Unfortunately,
the frame pointer seems to be decremented correspondingly to cover the
arguments, so we don't really have a way to tell how much to copy.
Copying just the caller's stack frame isn't safe either since a
callee/caller receiving such argument on the stack may passed it down to
a callee without copying (I couldn't find anything in the PCS stating
that this isn't allowed).

> I'm just asking if we can accept the existing code as now complete
> enough (in that I believe it matches the other implementations) and make
> this enhancement something for the next release cycle, allowing the existing
> code to be exercised by a wider audience and providing ample time to test
> the new modification? I'd hate to get stuck in a mode where this patch gets
> repeatedly delayed for changes that go above and beyond the original design.

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

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1450130

On 07/25/2016 01:13 PM, Catalin Marinas wrote:
> On Fri, Jul 22, 2016 at 11:51:32AM -0400, David Long wrote:
>> On 07/22/2016 06:16 AM, Catalin Marinas wrote:
>>> On Thu, Jul 21, 2016 at 02:33:52PM -0400, David Long wrote:
>>>> On 07/21/2016 01:23 PM, Marc Zyngier wrote:
>>>>> On 21/07/16 17:33, David Long wrote:
>>>>>> On 07/20/2016 12:09 PM, Marc Zyngier wrote:
>>>>>>> On 08/07/16 17:35, David Long wrote:
>>>>>>>> +#define MAX_INSN_SIZE			1
>>>>>>>> +#define MAX_STACK_SIZE			128
>>>>>>>
>>>>>>> Where is that value coming from? Because even on my 6502, I have a 256
>>>>>>> byte stack.
>>>>>>>
>>>>>>
>>>>>> Although I don't claim to know the original author's thoughts I would
>>>>>> guess it is based on the seven other existing implementations for
>>>>>> kprobes on various architectures, all of which appear to use either 64
>>>>>> or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the
>>>>>> whole stack.
>>> [...]
>>>>> My main worry is that whatever value you pick, it is always going to be
>>>>> wrong. This is used to preserve arguments that are passed on the stack,
>>>>> as opposed to passed by registers). We have no idea of what is getting
>>>>> passed there so saving nothing, 128 bytes or 2kB is about the same. It
>>>>> is always wrong.
>>>>>
>>>>> A much better solution would be to check the frame pointer, and copy the
>>>>> delta between FP and SP, assuming it fits inside the allocated buffer.
>>>>> If it doesn't, or if FP is invalid, we just skip the hook, because we
>>>>> can't reliably execute it.
>>>>
>>>> Well, this is the way it works literally everywhere else. It is a documented
>>>> limitation (Documentation/kprobes.txt). Said documentation may need to be
>>>> changed along with the suggested fix.
>>>
>>> The document states: "Up to MAX_STACK_SIZE bytes are copied". That means
>>> the arch code could always copy less but never more than MAX_STACK_SIZE.
>>> What we are proposing is that we should try to guess how much to copy
>>> based on the FP value (caller's frame) and, if larger than
>>> MAX_STACK_SIZE, skip the probe hook entirely. I don't think this goes
>>> against the kprobes.txt document but at least it (a) may improve the
>>> performance slightly by avoiding unnecessary copy and (b) it avoids
>>> undefined behaviour if we ever encounter a jprobe with arguments passed
>>> on the stack beyond MAX_STACK_SIZE.
>>
>> OK, it sounds like an improvement. I do worry a little about unexpected side
>> effects.
>
> You get more unexpected side effects by not saving/restoring the whole
> stack. We looked into this on Friday and came to the conclusion that
> there is no safe way for kprobes to know which arguments passed on the
> stack should be preserved, at least not with the current API.
>
> Basically the AArch64 PCS states that for arguments passed on the stack
> (e.g. they can't fit in registers), the caller allocates memory for them
> (on its own stack) and passes the pointer to the callee. Unfortunately,
> the frame pointer seems to be decremented correspondingly to cover the
> arguments, so we don't really have a way to tell how much to copy.
> Copying just the caller's stack frame isn't safe either since a
> callee/caller receiving such argument on the stack may passed it down to
> a callee without copying (I couldn't find anything in the PCS stating
> that this isn't allowed).

OK, so I think we're pretty much back to our starting point.
>
>> I'm just asking if we can accept the existing code as now complete
>> enough (in that I believe it matches the other implementations) and make
>> this enhancement something for the next release cycle, allowing the existing
>> code to be exercised by a wider audience and providing ample time to test
>> the new modification? I'd hate to get stuck in a mode where this patch gets
>> repeatedly delayed for changes that go above and beyond the original design.
>
> 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.

I think this (unchecked) limitation for stack frames is something users 
of jprobes understand, or at least should understand from the 
documentation.  At any rate it doesn't sound like we have a way of 
improving it, and I think that's OK.

-dl

[toc] | [prev] | [next] | [standalone]

#1451251

On 25/07/16 23:27, David Long wrote:
> On 07/25/2016 01:13 PM, Catalin Marinas wrote:
>> On Fri, Jul 22, 2016 at 11:51:32AM -0400, David Long wrote:
>>> On 07/22/2016 06:16 AM, Catalin Marinas wrote:
>>>> On Thu, Jul 21, 2016 at 02:33:52PM -0400, David Long wrote:
>>>>> On 07/21/2016 01:23 PM, Marc Zyngier wrote:
>>>>>> On 21/07/16 17:33, David Long wrote:
>>>>>>> On 07/20/2016 12:09 PM, Marc Zyngier wrote:
>>>>>>>> On 08/07/16 17:35, David Long wrote:
>>>>>>>>> +#define MAX_INSN_SIZE            1
>>>>>>>>> +#define MAX_STACK_SIZE            128
>>>>>>>>
>>>>>>>> Where is that value coming from? Because even on my 6502, I have
>>>>>>>> a 256
>>>>>>>> byte stack.
>>>>>>>>
>>>>>>>
>>>>>>> Although I don't claim to know the original author's thoughts I
>>>>>>> would
>>>>>>> guess it is based on the seven other existing implementations for
>>>>>>> kprobes on various architectures, all of which appear to use
>>>>>>> either 64
>>>>>>> or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the
>>>>>>> whole stack.
>>>> [...]
>>>>>> My main worry is that whatever value you pick, it is always going
>>>>>> to be
>>>>>> wrong. This is used to preserve arguments that are passed on the
>>>>>> stack,
>>>>>> as opposed to passed by registers). We have no idea of what is
>>>>>> getting
>>>>>> passed there so saving nothing, 128 bytes or 2kB is about the
>>>>>> same. It
>>>>>> is always wrong.
>>>>>>
>>>>>> A much better solution would be to check the frame pointer, and
>>>>>> copy the
>>>>>> delta between FP and SP, assuming it fits inside the allocated
>>>>>> buffer.
>>>>>> If it doesn't, or if FP is invalid, we just skip the hook, because we
>>>>>> can't reliably execute it.
>>>>>
>>>>> Well, this is the way it works literally everywhere else. It is a
>>>>> documented
>>>>> limitation (Documentation/kprobes.txt). Said documentation may need
>>>>> to be
>>>>> changed along with the suggested fix.
>>>>
>>>> The document states: "Up to MAX_STACK_SIZE bytes are copied". That
>>>> means
>>>> the arch code could always copy less but never more than
>>>> MAX_STACK_SIZE.
>>>> What we are proposing is that we should try to guess how much to copy
>>>> based on the FP value (caller's frame) and, if larger than
>>>> MAX_STACK_SIZE, skip the probe hook entirely. I don't think this goes
>>>> against the kprobes.txt document but at least it (a) may improve the
>>>> performance slightly by avoiding unnecessary copy and (b) it avoids
>>>> undefined behaviour if we ever encounter a jprobe with arguments passed
>>>> on the stack beyond MAX_STACK_SIZE.
>>>
>>> OK, it sounds like an improvement. I do worry a little about
>>> unexpected side
>>> effects.
>>
>> You get more unexpected side effects by not saving/restoring the whole
>> stack. We looked into this on Friday and came to the conclusion that
>> there is no safe way for kprobes to know which arguments passed on the
>> stack should be preserved, at least not with the current API.
>>
>> Basically the AArch64 PCS states that for arguments passed on the stack
>> (e.g. they can't fit in registers), the caller allocates memory for them
>> (on its own stack) and passes the pointer to the callee. Unfortunately,
>> the frame pointer seems to be decremented correspondingly to cover the
>> arguments, so we don't really have a way to tell how much to copy.
>> Copying just the caller's stack frame isn't safe either since a
>> callee/caller receiving such argument on the stack may passed it down to
>> a callee without copying (I couldn't find anything in the PCS stating
>> that this isn't allowed).
>
> OK, so I think we're pretty much back to our starting point.
>>
>>> I'm just asking if we can accept the existing code as now complete
>>> enough (in that I believe it matches the other implementations) and make
>>> this enhancement something for the next release cycle, allowing the
>>> existing
>>> code to be exercised by a wider audience and providing ample time to
>>> test
>>> the new modification? I'd hate to get stuck in a mode where this
>>> patch gets
>>> repeatedly delayed for changes that go above and beyond the original
>>> design.
>>
>> 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.


> I think this (unchecked) limitation for stack frames is something users
> of jprobes understand, or at least should understand from the
> documentation.  At any rate it doesn't sound like we have a way of
> improving it, and I think that's OK.

I don't think that this limitation could be inferred from the current 
jprobes documentation. Most architectures (include arm64 when handling 
 >8 parameters) place arguments at the top of the stack. For these 
architectures we need only consider the memory consumed by the (padded) 
arguments in the function signature to determine if the jprobe will be safe.

On arm64 large structures/unions end up being allocated like normal 
local variables and need not be near the top of the stack. This gives 
the caller much greater flexibility and makes safety a property of the 
caller not the callee.

So if it turns out to be too slow to store the whole of the stack then 
it should at the very least be mentioned in the list of architecture 
support that jprobes on functions that take structure/union arguments 
 >16 bytes are unsafe/unsupported.


Daniel.

[toc] | [prev] | [next] | [standalone]

#1451552

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:
>>> On Fri, Jul 22, 2016 at 11:51:32AM -0400, David Long wrote:
>>>> On 07/22/2016 06:16 AM, Catalin Marinas wrote:
>>>>> On Thu, Jul 21, 2016 at 02:33:52PM -0400, David Long wrote:
>>>>>> On 07/21/2016 01:23 PM, Marc Zyngier wrote:
>>>>>>> On 21/07/16 17:33, David Long wrote:
>>>>>>>> On 07/20/2016 12:09 PM, Marc Zyngier wrote:
>>>>>>>>> On 08/07/16 17:35, David Long wrote:
>>>>>>>>>> +#define MAX_INSN_SIZE            1
>>>>>>>>>> +#define MAX_STACK_SIZE            128
>>>>>>>>>
>>>>>>>>> Where is that value coming from? Because even on my 6502, I have
>>>>>>>>> a 256
>>>>>>>>> byte stack.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Although I don't claim to know the original author's thoughts I
>>>>>>>> would
>>>>>>>> guess it is based on the seven other existing implementations for
>>>>>>>> kprobes on various architectures, all of which appear to use
>>>>>>>> either 64
>>>>>>>> or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the
>>>>>>>> whole stack.
>>>>> [...]
>>>>>>> My main worry is that whatever value you pick, it is always going
>>>>>>> to be
>>>>>>> wrong. This is used to preserve arguments that are passed on the
>>>>>>> stack,
>>>>>>> as opposed to passed by registers). We have no idea of what is
>>>>>>> getting
>>>>>>> passed there so saving nothing, 128 bytes or 2kB is about the
>>>>>>> same. It
>>>>>>> is always wrong.
>>>>>>>
>>>>>>> A much better solution would be to check the frame pointer, and
>>>>>>> copy the
>>>>>>> delta between FP and SP, assuming it fits inside the allocated
>>>>>>> buffer.
>>>>>>> If it doesn't, or if FP is invalid, we just skip the hook,
>>>>>>> because we
>>>>>>> can't reliably execute it.
>>>>>>
>>>>>> Well, this is the way it works literally everywhere else. It is a
>>>>>> documented
>>>>>> limitation (Documentation/kprobes.txt). Said documentation may need
>>>>>> to be
>>>>>> changed along with the suggested fix.
>>>>>
>>>>> The document states: "Up to MAX_STACK_SIZE bytes are copied". That
>>>>> means
>>>>> the arch code could always copy less but never more than
>>>>> MAX_STACK_SIZE.
>>>>> What we are proposing is that we should try to guess how much to copy
>>>>> based on the FP value (caller's frame) and, if larger than
>>>>> MAX_STACK_SIZE, skip the probe hook entirely. I don't think this goes
>>>>> against the kprobes.txt document but at least it (a) may improve the
>>>>> performance slightly by avoiding unnecessary copy and (b) it avoids
>>>>> undefined behaviour if we ever encounter a jprobe with arguments
>>>>> passed
>>>>> on the stack beyond MAX_STACK_SIZE.
>>>>
>>>> OK, it sounds like an improvement. I do worry a little about
>>>> unexpected side
>>>> effects.
>>>
>>> You get more unexpected side effects by not saving/restoring the whole
>>> stack. We looked into this on Friday and came to the conclusion that
>>> there is no safe way for kprobes to know which arguments passed on the
>>> stack should be preserved, at least not with the current API.
>>>
>>> Basically the AArch64 PCS states that for arguments passed on the stack
>>> (e.g. they can't fit in registers), the caller allocates memory for them
>>> (on its own stack) and passes the pointer to the callee. Unfortunately,
>>> the frame pointer seems to be decremented correspondingly to cover the
>>> arguments, so we don't really have a way to tell how much to copy.
>>> Copying just the caller's stack frame isn't safe either since a
>>> callee/caller receiving such argument on the stack may passed it down to
>>> a callee without copying (I couldn't find anything in the PCS stating
>>> that this isn't allowed).
>>
>> OK, so I think we're pretty much back to our starting point.
>>>
>>>> I'm just asking if we can accept the existing code as now complete
>>>> enough (in that I believe it matches the other implementations) and
>>>> make
>>>> this enhancement something for the next release cycle, allowing the
>>>> existing
>>>> code to be exercised by a wider audience and providing ample time to
>>>> test
>>>> the new modification? I'd hate to get stuck in a mode where this
>>>> patch gets
>>>> repeatedly delayed for changes that go above and beyond the original
>>>> design.
>>>
>>> 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 think this (unchecked) limitation for stack frames is something users
>> of jprobes understand, or at least should understand from the
>> documentation.  At any rate it doesn't sound like we have a way of
>> improving it, and I think that's OK.
>
> I don't think that this limitation could be inferred from the current
> jprobes documentation. Most architectures (include arm64 when handling
>  >8 parameters) place arguments at the top of the stack. For these
> architectures we need only consider the memory consumed by the (padded)
> arguments in the function signature to determine if the jprobe will be
> safe.
>
> On arm64 large structures/unions end up being allocated like normal
> local variables and need not be near the top of the stack. This gives
> the caller much greater flexibility and makes safety a property of the
> caller not the callee.
>

Yes, I had not fully appreciated how spread out the important parts of 
the stack frame could be, before now.

> So if it turns out to be too slow to store the whole of the stack then
> it should at the very least be mentioned in the list of architecture
> support that jprobes on functions that take structure/union arguments
>  >16 bytes are unsafe/unsupported.
>
>
> Daniel.

Thanks,
-dl

[toc] | [prev] | [next] | [standalone]

#1451966

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 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;
+
 	kcb->jprobe_saved_regs = *regs;
 	/*
 	 * As Linus pointed out, gcc assumes that the callee

Unfortunately, we don't really have a way to detect large composite
types passed as arguments, so we only have to rely on the documentation.

Can you please submit a patch that removes MAX_STACK_SIZE for arm64,
documents it and include the above hunk (once tested that it actually
does what it intends to).

Thanks.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1452335

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!


 > 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.

[toc] | [prev] | [next] | [standalone]

#1456164

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" <dave.long@linaro.org>
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 <dave.long@linaro.org>
---
 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(-)

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;
 }
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1457721

On 04/08/16 05:47, David Long wrote:
> From b451caa1adaf1d03e08a44b5dad3fca31cebd97a Mon Sep 17 00:00:00 2001
> From: "David A. Long" <dave.long@linaro.org>
> 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 <dave.long@linaro.org>
> ---
>  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(-)
>
> 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

Could sparc64 be added to this list?

   For the sparc folks who are new to the thread, we've previously
   established that the sparc64 ABI passes large structures by
   allocating them from the caller's stack frame and passing a pointer
   to the stack frame (i.e. arguments may not be at top of the stack).
   We also noticed that sparc code does not save/restore anything from
   the stack.


> +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.

I like this wording. So much so that it really would be great to repeat 
this in the Documentation/. Could this be included in the list of 
architecture support/restrictions?


Daniel.

[toc] | [prev] | [next] | [standalone]

#1457835

On 08/08/2016 07:13 AM, Daniel Thompson wrote:
> On 04/08/16 05:47, David Long wrote:
>> From b451caa1adaf1d03e08a44b5dad3fca31cebd97a Mon Sep 17 00:00:00 2001
>> From: "David A. Long" <dave.long@linaro.org>
>> 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 <dave.long@linaro.org>
>> ---
>>  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(-)
>>
>> 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
>
> Could sparc64 be added to this list?
>
>    For the sparc folks who are new to the thread, we've previously
>    established that the sparc64 ABI passes large structures by
>    allocating them from the caller's stack frame and passing a pointer
>    to the stack frame (i.e. arguments may not be at top of the stack).
>    We also noticed that sparc code does not save/restore anything from
>    the stack.
>

I was reluctant to do that in the context of late changes to v4.8 for 
arm64 but now that any changes for this are going in as a new patch it 
would indeed be useful to get involvement from sparc maintainers.

>
>> +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.
>
> I like this wording. So much so that it really would be great to repeat
> this in the Documentation/. Could this be included in the list of
> architecture support/restrictions?
>

Are you thinking specifically of the "5. Kprobes Features and 
Limitations" section in Documentation/kprobes.txt?

>
> Daniel.
>

Thanks,
-dl

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web