[PATCH v3 0/7] powerpc: a few kprobe fixes and refactoring

[PATCH v3 0/7] powerpc: a few kprobe fixes and refactoring

[Naveen N. Rao]

v2:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1375870.html

v3 changes:
- Patch 3/5 in the previous series ("powerpc: introduce a new helper to
  obtain function entry points") has been dropped from this series and
  will instead be posted as part of the KPROBES_ON_FTRACE patchset.
- Patch 5/5 in the previous series ("powerpc: kprobes: emulate
  instructions on kprobe handler re-entry") has been split into two
  patches as recommended by Masami. They represent patches 6/7 and 7/7
  in this series.
- Patches 3/7 and 4/7 are new in this series to address review comments
  from David Laight.


- Naveen


Naveen N. Rao (7):
  kprobes: convert kprobe_lookup_name() to a function
  powerpc: kprobes: fix handling of function offsets on ABIv2
  kprobes: validate the symbol name length
  powerpc: kprobes: use safer string functions in kprobe_lookup_name()
  powerpc: kprobes: factor out code to emulate instruction into a helper
  powerpc: kprobes: emulate instructions on kprobe handler re-entry
  powerpc: kprobes: remove duplicate saving of msr

 arch/powerpc/include/asm/kprobes.h |  53 -----------------
 arch/powerpc/kernel/kprobes.c      | 118 ++++++++++++++++++++++++++++++-------
 arch/powerpc/kernel/optprobes.c    |   4 +-
 include/linux/kprobes.h            |   2 +
 kernel/kprobes.c                   |  45 ++++++++++----
 kernel/trace/trace_kprobe.c        |   4 ++
 6 files changed, 137 insertions(+), 89 deletions(-)

-- 
2.12.1

[PATCH v3 1/7] kprobes: convert kprobe_lookup_name() to a function

[Naveen N. Rao]

The macro is now pretty long and ugly on powerpc. In the light of
further changes needed here, convert it to a __weak variant to be
over-ridden with a nicer looking function.

Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/kprobes.h | 53 ----------------------------------
 arch/powerpc/kernel/kprobes.c      | 58 ++++++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/optprobes.c    |  4 +--
 include/linux/kprobes.h            |  1 +
 kernel/kprobes.c                   | 20 ++++++-------
 5 files changed, 69 insertions(+), 67 deletions(-)

diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
index 0503c98b2117..a843884aafaf 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -61,59 +61,6 @@ extern kprobe_opcode_t optprobe_template_end[];
 #define MAX_OPTINSN_SIZE	(optprobe_template_end - optprobe_template_entry)
 #define RELATIVEJUMP_SIZE	sizeof(kprobe_opcode_t)	/* 4 bytes */
 
-#ifdef PPC64_ELF_ABI_v2
-/* PPC64 ABIv2 needs local entry point */
-#define kprobe_lookup_name(name, addr)					\
-{									\
-	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);		\
-	if (addr)							\
-		addr = (kprobe_opcode_t *)ppc_function_entry(addr);	\
-}
-#elif defined(PPC64_ELF_ABI_v1)
-/*
- * 64bit powerpc ABIv1 uses function descriptors:
- * - Check for the dot variant of the symbol first.
- * - If that fails, try looking up the symbol provided.
- *
- * This ensures we always get to the actual symbol and not the descriptor.
- * Also handle <module:symbol> format.
- */
-#define kprobe_lookup_name(name, addr)					\
-{									\
-	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];		\
-	const char *modsym;							\
-	bool dot_appended = false;					\
-	if ((modsym = strchr(name, ':')) != NULL) {			\
-		modsym++;						\
-		if (*modsym != '\0' && *modsym != '.') {		\
-			/* Convert to <module:.symbol> */		\
-			strncpy(dot_name, name, modsym - name);		\
-			dot_name[modsym - name] = '.';			\
-			dot_name[modsym - name + 1] = '\0';		\
-			strncat(dot_name, modsym,			\
-				sizeof(dot_name) - (modsym - name) - 2);\
-			dot_appended = true;				\
-		} else {						\
-			dot_name[0] = '\0';				\
-			strncat(dot_name, name, sizeof(dot_name) - 1);	\
-		}							\
-	} else if (name[0] != '.') {					\
-		dot_name[0] = '.';					\
-		dot_name[1] = '\0';					\
-		strncat(dot_name, name, KSYM_NAME_LEN - 2);		\
-		dot_appended = true;					\
-	} else {							\
-		dot_name[0] = '\0';					\
-		strncat(dot_name, name, KSYM_NAME_LEN - 1);		\
-	}								\
-	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);	\
-	if (!addr && dot_appended) {					\
-		/* Let's try the original non-dot symbol lookup	*/	\
-		addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);	\
-	}								\
-}
-#endif
-
 #define flush_insn_slot(p)	do { } while (0)
 #define kretprobe_blacklist_size 0
 
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 331751701fed..71c30025cc8a 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -42,6 +42,64 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
 
+kprobe_opcode_t *kprobe_lookup_name(const char *name)
+{
+	kprobe_opcode_t *addr;
+
+#ifdef PPC64_ELF_ABI_v2
+	/* PPC64 ABIv2 needs local entry point */
+	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
+	if (addr)
+		addr = (kprobe_opcode_t *)ppc_function_entry(addr);
+#elif defined(PPC64_ELF_ABI_v1)
+	/*
+	 * 64bit powerpc ABIv1 uses function descriptors:
+	 * - Check for the dot variant of the symbol first.
+	 * - If that fails, try looking up the symbol provided.
+	 *
+	 * This ensures we always get to the actual symbol and not
+	 * the descriptor.
+	 *
+	 * Also handle <module:symbol> format.
+	 */
+	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
+	const char *modsym;
+	bool dot_appended = false;
+	if ((modsym = strchr(name, ':')) != NULL) {
+		modsym++;
+		if (*modsym != '\0' && *modsym != '.') {
+			/* Convert to <module:.symbol> */
+			strncpy(dot_name, name, modsym - name);
+			dot_name[modsym - name] = '.';
+			dot_name[modsym - name + 1] = '\0';
+			strncat(dot_name, modsym,
+				sizeof(dot_name) - (modsym - name) - 2);
+			dot_appended = true;
+		} else {
+			dot_name[0] = '\0';
+			strncat(dot_name, name, sizeof(dot_name) - 1);
+		}
+	} else if (name[0] != '.') {
+		dot_name[0] = '.';
+		dot_name[1] = '\0';
+		strncat(dot_name, name, KSYM_NAME_LEN - 2);
+		dot_appended = true;
+	} else {
+		dot_name[0] = '\0';
+		strncat(dot_name, name, KSYM_NAME_LEN - 1);
+	}
+	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
+	if (!addr && dot_appended) {
+		/* Let's try the original non-dot symbol lookup	*/
+		addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
+	}
+#else
+	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
+#endif
+
+	return addr;
+}
+
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	int ret = 0;
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 2282bf4e63cd..aefe076d00e0 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -243,8 +243,8 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 	/*
 	 * 2. branch to optimized_callback() and emulate_step()
 	 */
-	kprobe_lookup_name("optimized_callback", op_callback_addr);
-	kprobe_lookup_name("emulate_step", emulate_step_addr);
+	op_callback_addr = kprobe_lookup_name("optimized_callback");
+	emulate_step_addr = kprobe_lookup_name("emulate_step");
 	if (!op_callback_addr || !emulate_step_addr) {
 		WARN(1, "kprobe_lookup_name() failed\n");
 		goto error;
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index c328e4f7dcad..16f153c84646 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -379,6 +379,7 @@ static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)
 	return this_cpu_ptr(&kprobe_ctlblk);
 }
 
+kprobe_opcode_t *kprobe_lookup_name(const char *name);
 int register_kprobe(struct kprobe *p);
 void unregister_kprobe(struct kprobe *p);
 int register_kprobes(struct kprobe **kps, int num);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 699c5bc51a92..f3421b6b47a3 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -58,15 +58,6 @@
 #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
 
 
-/*
- * Some oddball architectures like 64bit powerpc have function descriptors
- * so this must be overridable.
- */
-#ifndef kprobe_lookup_name
-#define kprobe_lookup_name(name, addr) \
-	addr = ((kprobe_opcode_t *)(kallsyms_lookup_name(name)))
-#endif
-
 static int kprobes_initialized;
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
@@ -81,6 +72,11 @@ static struct {
 	raw_spinlock_t lock ____cacheline_aligned_in_smp;
 } kretprobe_table_locks[KPROBE_TABLE_SIZE];
 
+kprobe_opcode_t * __weak kprobe_lookup_name(const char *name)
+{
+	return ((kprobe_opcode_t *)(kallsyms_lookup_name(name)));
+}
+
 static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
 {
 	return &(kretprobe_table_locks[hash].lock);
@@ -1400,7 +1396,7 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
 		goto invalid;
 
 	if (p->symbol_name) {
-		kprobe_lookup_name(p->symbol_name, addr);
+		addr = kprobe_lookup_name(p->symbol_name);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
 	}
@@ -2192,8 +2188,8 @@ static int __init init_kprobes(void)
 	if (kretprobe_blacklist_size) {
 		/* lookup the function address from its name */
 		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
-			kprobe_lookup_name(kretprobe_blacklist[i].name,
-					   kretprobe_blacklist[i].addr);
+			kretprobe_blacklist[i].addr =
+				kprobe_lookup_name(kretprobe_blacklist[i].name);
 			if (!kretprobe_blacklist[i].addr)
 				printk("kretprobe: lookup failed: %s\n",
 				       kretprobe_blacklist[i].name);
-- 
2.12.1

Re: [v3,1/7] kprobes: convert kprobe_lookup_name() to a function

[Michael Ellerman]

On Wed, 2017-04-19 at 12:51:00 UTC, "Naveen N. Rao" wrote:
> The macro is now pretty long and ugly on powerpc. In the light of
> further changes needed here, convert it to a __weak variant to be
> over-ridden with a nicer looking function.
> 
> Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/49e0b4658fe6aab5bf6bfe0738a86c

cheers

[PATCH v3 2/7] powerpc: kprobes: fix handling of function offsets on ABIv2

[Naveen N. Rao]

commit 239aeba76409 ("perf powerpc: Fix kprobe and kretprobe handling
with kallsyms on ppc64le") changed how we use the offset field in struct
kprobe on ABIv2. perf now offsets from the GEP (Global entry point) if an
offset is specified and otherwise chooses the LEP (Local entry point).

Fix the same in kernel for kprobe API users. We do this by extending
kprobe_lookup_name() to accept an additional parameter to indicate the
offset specified with the kprobe registration. If offset is 0, we return
the local function entry and return the global entry point otherwise.

With:
	# cd /sys/kernel/debug/tracing/
	# echo "p _do_fork" >> kprobe_events
	# echo "p _do_fork+0x10" >> kprobe_events

before this patch:
	# cat ../kprobes/list
	c0000000000d0748  k  _do_fork+0x8    [DISABLED]
	c0000000000d0758  k  _do_fork+0x18    [DISABLED]
	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]

and after:
	# cat ../kprobes/list
	c0000000000d04c8  k  _do_fork+0x8    [DISABLED]
	c0000000000d04d0  k  _do_fork+0x10    [DISABLED]
	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c   | 4 ++--
 arch/powerpc/kernel/optprobes.c | 4 ++--
 include/linux/kprobes.h         | 2 +-
 kernel/kprobes.c                | 7 ++++---
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 71c30025cc8a..97b5eed1f76d 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -42,14 +42,14 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
 
-kprobe_opcode_t *kprobe_lookup_name(const char *name)
+kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 {
 	kprobe_opcode_t *addr;
 
 #ifdef PPC64_ELF_ABI_v2
 	/* PPC64 ABIv2 needs local entry point */
 	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
-	if (addr)
+	if (addr && !offset)
 		addr = (kprobe_opcode_t *)ppc_function_entry(addr);
 #elif defined(PPC64_ELF_ABI_v1)
 	/*
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index aefe076d00e0..ce81a322251c 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -243,8 +243,8 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 	/*
 	 * 2. branch to optimized_callback() and emulate_step()
 	 */
-	op_callback_addr = kprobe_lookup_name("optimized_callback");
-	emulate_step_addr = kprobe_lookup_name("emulate_step");
+	op_callback_addr = kprobe_lookup_name("optimized_callback", 0);
+	emulate_step_addr = kprobe_lookup_name("emulate_step", 0);
 	if (!op_callback_addr || !emulate_step_addr) {
 		WARN(1, "kprobe_lookup_name() failed\n");
 		goto error;
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 16f153c84646..1f82a3db00b1 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -379,7 +379,7 @@ static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)
 	return this_cpu_ptr(&kprobe_ctlblk);
 }
 
-kprobe_opcode_t *kprobe_lookup_name(const char *name);
+kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset);
 int register_kprobe(struct kprobe *p);
 void unregister_kprobe(struct kprobe *p);
 int register_kprobes(struct kprobe **kps, int num);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index f3421b6b47a3..6a128f3a7ed1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -72,7 +72,8 @@ static struct {
 	raw_spinlock_t lock ____cacheline_aligned_in_smp;
 } kretprobe_table_locks[KPROBE_TABLE_SIZE];
 
-kprobe_opcode_t * __weak kprobe_lookup_name(const char *name)
+kprobe_opcode_t * __weak kprobe_lookup_name(const char *name,
+					unsigned int __unused)
 {
 	return ((kprobe_opcode_t *)(kallsyms_lookup_name(name)));
 }
@@ -1396,7 +1397,7 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
 		goto invalid;
 
 	if (p->symbol_name) {
-		addr = kprobe_lookup_name(p->symbol_name);
+		addr = kprobe_lookup_name(p->symbol_name, p->offset);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
 	}
@@ -2189,7 +2190,7 @@ static int __init init_kprobes(void)
 		/* lookup the function address from its name */
 		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
 			kretprobe_blacklist[i].addr =
-				kprobe_lookup_name(kretprobe_blacklist[i].name);
+				kprobe_lookup_name(kretprobe_blacklist[i].name, 0);
 			if (!kretprobe_blacklist[i].addr)
 				printk("kretprobe: lookup failed: %s\n",
 				       kretprobe_blacklist[i].name);
-- 
2.12.1

Re: [v3,2/7] powerpc: kprobes: fix handling of function offsets on ABIv2

[Michael Ellerman]

On Wed, 2017-04-19 at 12:51:01 UTC, "Naveen N. Rao" wrote:
> commit 239aeba76409 ("perf powerpc: Fix kprobe and kretprobe handling
> with kallsyms on ppc64le") changed how we use the offset field in struct
> kprobe on ABIv2. perf now offsets from the GEP (Global entry point) if an
> offset is specified and otherwise chooses the LEP (Local entry point).
> 
> Fix the same in kernel for kprobe API users. We do this by extending
> kprobe_lookup_name() to accept an additional parameter to indicate the
> offset specified with the kprobe registration. If offset is 0, we return
> the local function entry and return the global entry point otherwise.
> 
> With:
> 	# cd /sys/kernel/debug/tracing/
> 	# echo "p _do_fork" >> kprobe_events
> 	# echo "p _do_fork+0x10" >> kprobe_events
> 
> before this patch:
> 	# cat ../kprobes/list
> 	c0000000000d0748  k  _do_fork+0x8    [DISABLED]
> 	c0000000000d0758  k  _do_fork+0x18    [DISABLED]
> 	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]
> 
> and after:
> 	# cat ../kprobes/list
> 	c0000000000d04c8  k  _do_fork+0x8    [DISABLED]
> 	c0000000000d04d0  k  _do_fork+0x10    [DISABLED]
> 	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]
> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/290e3070762ac80e5fc4087d8c4de7

cheers

[PATCH v3 3/7] kprobes: validate the symbol name length

[Naveen N. Rao]

When a kprobe is being registered, we use the symbol_name field to
lookup the address where the probe should be placed. Since this is a
user-provided field, let's ensure that the length of the string is
within expected limits.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 include/linux/kprobes.h     |  1 +
 kernel/kprobes.c            | 24 ++++++++++++++++++++++++
 kernel/trace/trace_kprobe.c |  4 ++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 1f82a3db00b1..4ee10fef5135 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -405,6 +405,7 @@ int disable_kprobe(struct kprobe *kp);
 int enable_kprobe(struct kprobe *kp);
 
 void dump_kprobe(struct kprobe *kp);
+bool is_valid_kprobe_symbol_name(const char *name);
 
 #else /* !CONFIG_KPROBES: */
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 6a128f3a7ed1..bb86681c8a10 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
 	return false;
 }
 
+bool is_valid_kprobe_symbol_name(const char *name)
+{
+	size_t sym_len;
+	char *s;
+
+	s = strchr(name, ':');
+	if (s) {
+		sym_len = strnlen(s+1, KSYM_NAME_LEN);
+		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
+			return false;
+		sym_len = (size_t)(s - name);
+		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
+			return false;
+	} else {
+		sym_len = strnlen(name, MODULE_NAME_LEN);
+		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
+			return false;
+	}
+
+	return true;
+}
+
 /*
  * If we have a symbol_name argument, look it up and add the offset field
  * to it. This way, we can specify a relative address to a symbol.
@@ -1397,6 +1419,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
 		goto invalid;
 
 	if (p->symbol_name) {
+		if (!is_valid_kprobe_symbol_name(p->symbol_name))
+			return ERR_PTR(-EINVAL);
 		addr = kprobe_lookup_name(p->symbol_name, p->offset);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5f688cc724f0..bf73e5f31128 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
 			pr_info("Return probe must be used without offset.\n");
 			return -EINVAL;
 		}
+		if (!is_valid_kprobe_symbol_name(symbol)) {
+			pr_info("Symbol name is too long.\n");
+			return -EINVAL;
+		}
 	}
 	argc -= 2; argv += 2;
 
-- 
2.12.1

Re: [PATCH v3 3/7] kprobes: validate the symbol name length

[Masami Hiramatsu]

On Wed, 19 Apr 2017 18:21:02 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> When a kprobe is being registered, we use the symbol_name field to
> lookup the address where the probe should be placed. Since this is a
> user-provided field, let's ensure that the length of the string is
> within expected limits.

Would we really need this? Of course it may filter out longer
strings... anyway such name should be rejected by kallsyms.

[...]
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 6a128f3a7ed1..bb86681c8a10 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>  	return false;
>  }
>  
> +bool is_valid_kprobe_symbol_name(const char *name)

This just check the length of symbol_name buffer, and can contain
some invalid chars.

> +{
> +	size_t sym_len;
> +	char *s;
> +
> +	s = strchr(name, ':');
> +	if (s) {
> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);

If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.

> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
> +			return false;
> +		sym_len = (size_t)(s - name);
> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> +			return false;
> +	} else {
> +		sym_len = strnlen(name, MODULE_NAME_LEN);
> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)

Would you mean KSYM_NAME_LEN here?

> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * If we have a symbol_name argument, look it up and add the offset field
>   * to it. This way, we can specify a relative address to a symbol.
> @@ -1397,6 +1419,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>  		goto invalid;
>  
>  	if (p->symbol_name) {
> +		if (!is_valid_kprobe_symbol_name(p->symbol_name))
> +			return ERR_PTR(-EINVAL);
>  		addr = kprobe_lookup_name(p->symbol_name, p->offset);
>  		if (!addr)
>  			return ERR_PTR(-ENOENT);
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5f688cc724f0..bf73e5f31128 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
>  			pr_info("Return probe must be used without offset.\n");
>  			return -EINVAL;
>  		}
> +		if (!is_valid_kprobe_symbol_name(symbol)) {
> +			pr_info("Symbol name is too long.\n");
> +			return -EINVAL;
> +		}
>  	}
>  	argc -= 2; argv += 2;
>  
> -- 
> 2.12.1
> 

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 3/7] kprobes: validate the symbol name length

[Naveen N. Rao]

Excerpts from Masami Hiramatsu's message of April 19, 2017 20:07:
> On Wed, 19 Apr 2017 18:21:02 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> When a kprobe is being registered, we use the symbol_name field to
>> lookup the address where the probe should be placed. Since this is a
>> user-provided field, let's ensure that the length of the string is
>> within expected limits.
> 
> Would we really need this? Of course it may filter out longer
> strings... anyway such name should be rejected by kallsyms.

I felt this would be good to have generically, as kallsyms does many 
string operations on the symbol name, including an unbounded strchr().

> 
> [...]
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 6a128f3a7ed1..bb86681c8a10 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>>  	return false;
>>  }
>>  
>> +bool is_valid_kprobe_symbol_name(const char *name)
> 
> This just check the length of symbol_name buffer, and can contain
> some invalid chars.

Yes, I kept the function name generic incase we would like to do more 
validation in future, plus it's shorter than 
is_valid_kprobe_symbol_name_len() ;-)

> 
>> +{
>> +	size_t sym_len;
>> +	char *s;
>> +
>> +	s = strchr(name, ':');

Hmm.. this should be strnchr(). I re-factored the code that moved the 
strnlen() above this below. I'll fix this.

>> +	if (s) {
>> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
> 
> If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.

Hmm.. not sure I follow. Are you saying the check for sym_len <= 0 is 
not needed?

> 
>> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
>> +			return false;
>> +		sym_len = (size_t)(s - name);
>> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
>> +			return false;
>> +	} else {
>> +		sym_len = strnlen(name, MODULE_NAME_LEN);
>> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> 
> Would you mean KSYM_NAME_LEN here?

Oops... nice catch, Thanks!

- Naveen

> 
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>  /*
>>   * If we have a symbol_name argument, look it up and add the offset field
>>   * to it. This way, we can specify a relative address to a symbol.
>> @@ -1397,6 +1419,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>>  		goto invalid;
>>  
>>  	if (p->symbol_name) {
>> +		if (!is_valid_kprobe_symbol_name(p->symbol_name))
>> +			return ERR_PTR(-EINVAL);
>>  		addr = kprobe_lookup_name(p->symbol_name, p->offset);
>>  		if (!addr)
>>  			return ERR_PTR(-ENOENT);
>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
>> index 5f688cc724f0..bf73e5f31128 100644
>> --- a/kernel/trace/trace_kprobe.c
>> +++ b/kernel/trace/trace_kprobe.c
>> @@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
>>  			pr_info("Return probe must be used without offset.\n");
>>  			return -EINVAL;
>>  		}
>> +		if (!is_valid_kprobe_symbol_name(symbol)) {
>> +			pr_info("Symbol name is too long.\n");
>> +			return -EINVAL;
>> +		}
>>  	}
>>  	argc -= 2; argv += 2;
>>  
>> -- 
>> 2.12.1
>> 
> 
> Thanks,
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
> 
> 

Re: [PATCH v3 3/7] kprobes: validate the symbol name length

[Masami Hiramatsu]

On Wed, 19 Apr 2017 16:38:22 +0000
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Excerpts from Masami Hiramatsu's message of April 19, 2017 20:07:
> > On Wed, 19 Apr 2017 18:21:02 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> >> When a kprobe is being registered, we use the symbol_name field to
> >> lookup the address where the probe should be placed. Since this is a
> >> user-provided field, let's ensure that the length of the string is
> >> within expected limits.
> > 
> > Would we really need this? Of course it may filter out longer
> > strings... anyway such name should be rejected by kallsyms.
> 
> I felt this would be good to have generically, as kallsyms does many 
> string operations on the symbol name, including an unbounded strchr().

OK, so this is actually for performance reason.

> 
> > 
> > [...]
> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> >> index 6a128f3a7ed1..bb86681c8a10 100644
> >> --- a/kernel/kprobes.c
> >> +++ b/kernel/kprobes.c
> >> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
> >>  	return false;
> >>  }
> >>  
> >> +bool is_valid_kprobe_symbol_name(const char *name)
> > 
> > This just check the length of symbol_name buffer, and can contain
> > some invalid chars.
> 
> Yes, I kept the function name generic incase we would like to do more 
> validation in future, plus it's shorter than 
> is_valid_kprobe_symbol_name_len() ;-)

OK, if this is enough general, we'd better define this in
kernel/kallsyms.c or in kallsyms.h. Of course the function
should be called is_valid_symbol_name(). :-)

> >> +{
> >> +	size_t sym_len;
> >> +	char *s;
> >> +
> >> +	s = strchr(name, ':');
> 
> Hmm.. this should be strnchr(). I re-factored the code that moved the 
> strnlen() above this below. I'll fix this.
> 
> >> +	if (s) {
> >> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
> > 
> > If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.
> 
> Hmm.. not sure I follow. Are you saying the check for sym_len <= 0 is 
> not needed?

You can check sym_len != 0, but anyway, here we concern about
"longer" string (for performance reason), we can focus on
such case.
(BTW, could you also check the name != NULL at first?)

So, what I think it can be;

if (strnlen(s+1, KSYM_NAME_LEN) == KSYM_NAME_LEN ||
    (size_t)(s - name) >= MODULE_NAME_LEN)
	return false;

Thanks,

> >> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
> >> +			return false;
> >> +		sym_len = (size_t)(s - name);
> >> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> >> +			return false;
> >> +	} else {
> >> +		sym_len = strnlen(name, MODULE_NAME_LEN);
> >> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> > 
> > Would you mean KSYM_NAME_LEN here?
> 
> Oops... nice catch, Thanks!
> 
> - Naveen
> 
> > 
> >> +			return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +
> >>  /*
> >>   * If we have a symbol_name argument, look it up and add the offset field
> >>   * to it. This way, we can specify a relative address to a symbol.
> >> @@ -1397,6 +1419,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
> >>  		goto invalid;
> >>  
> >>  	if (p->symbol_name) {
> >> +		if (!is_valid_kprobe_symbol_name(p->symbol_name))
> >> +			return ERR_PTR(-EINVAL);
> >>  		addr = kprobe_lookup_name(p->symbol_name, p->offset);
> >>  		if (!addr)
> >>  			return ERR_PTR(-ENOENT);
> >> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> >> index 5f688cc724f0..bf73e5f31128 100644
> >> --- a/kernel/trace/trace_kprobe.c
> >> +++ b/kernel/trace/trace_kprobe.c
> >> @@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
> >>  			pr_info("Return probe must be used without offset.\n");
> >>  			return -EINVAL;
> >>  		}
> >> +		if (!is_valid_kprobe_symbol_name(symbol)) {
> >> +			pr_info("Symbol name is too long.\n");
> >> +			return -EINVAL;
> >> +		}
> >>  	}
> >>  	argc -= 2; argv += 2;
> >>  
> >> -- 
> >> 2.12.1
> >> 
> > 
> > Thanks,
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 3/7] kprobes: validate the symbol name length

[Naveen N. Rao]

Excerpts from Masami Hiramatsu's message of April 21, 2017 19:12:
> On Wed, 19 Apr 2017 16:38:22 +0000
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> Excerpts from Masami Hiramatsu's message of April 19, 2017 20:07:
>> > On Wed, 19 Apr 2017 18:21:02 +0530
>> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>> > 
>> >> When a kprobe is being registered, we use the symbol_name field to
>> >> lookup the address where the probe should be placed. Since this is a
>> >> user-provided field, let's ensure that the length of the string is
>> >> within expected limits.
>> > 
>> > Would we really need this? Of course it may filter out longer
>> > strings... anyway such name should be rejected by kallsyms.
>> 
>> I felt this would be good to have generically, as kallsyms does many 
>> string operations on the symbol name, including an unbounded 
>> strchr().
> 
> OK, so this is actually for performance reason.
> 
>> 
>> > 
>> > [...]
>> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> >> index 6a128f3a7ed1..bb86681c8a10 100644
>> >> --- a/kernel/kprobes.c
>> >> +++ b/kernel/kprobes.c
>> >> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>> >>  	return false;
>> >>  }
>> >>  
>> >> +bool is_valid_kprobe_symbol_name(const char *name)
>> > 
>> > This just check the length of symbol_name buffer, and can contain
>> > some invalid chars.
>> 
>> Yes, I kept the function name generic incase we would like to do more 
>> validation in future, plus it's shorter than 
>> is_valid_kprobe_symbol_name_len() ;-)
> 
> OK, if this is enough general, we'd better define this in
> kernel/kallsyms.c or in kallsyms.h. Of course the function
> should be called is_valid_symbol_name(). :-)

I actually think this should be done in kprobes itself. The primary 
intent is to perform such validation right when we first obtain the 
input from the user. In this case, however, kallsyms_lookup_name() is 
also an exported symbol, so I do think some validation there would be 
good to have as well.

> 
>> >> +{
>> >> +	size_t sym_len;
>> >> +	char *s;
>> >> +
>> >> +	s = strchr(name, ':');
>> 
>> Hmm.. this should be strnchr(). I re-factored the code that moved the 
>> strnlen() above this below. I'll fix this.
>> 
>> >> +	if (s) {
>> >> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
>> > 
>> > If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.
>> 
>> Hmm.. not sure I follow. Are you saying the check for sym_len <= 0 is 
>> not needed?
> 
> You can check sym_len != 0, but anyway, here we concern about
> "longer" string (for performance reason), we can focus on
> such case.
> (BTW, could you also check the name != NULL at first?)
> 
> So, what I think it can be;
> 
> if (strnlen(s+1, KSYM_NAME_LEN) == KSYM_NAME_LEN ||
>     (size_t)(s - name) >= MODULE_NAME_LEN)
> 	return false;

Sure, thanks. I clearly need to refactor this code better!

- Naveen

Re: [PATCH v3 3/7] kprobes: validate the symbol name length

[Masami Hiramatsu]

On Sun, 23 Apr 2017 15:44:32 +0000
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> >> >> +bool is_valid_kprobe_symbol_name(const char *name)
> >> > 
> >> > This just check the length of symbol_name buffer, and can contain
> >> > some invalid chars.
> >> 
> >> Yes, I kept the function name generic incase we would like to do more 
> >> validation in future, plus it's shorter than 
> >> is_valid_kprobe_symbol_name_len() ;-)
> > 
> > OK, if this is enough general, we'd better define this in
> > kernel/kallsyms.c or in kallsyms.h. Of course the function
> > should be called is_valid_symbol_name(). :-)
> 
> I actually think this should be done in kprobes itself. The primary 
> intent is to perform such validation right when we first obtain the 
> input from the user. In this case, however, kallsyms_lookup_name() is 
> also an exported symbol, so I do think some validation there would be 
> good to have as well.

IMHO, it is natural that kallsyms will know what is valid symbols.
Providing validation function by kprobes means kprobes also knows
that, and I concerns that may lead a double standard.

Thanks,

> >> >> +{
> >> >> +	size_t sym_len;
> >> >> +	char *s;
> >> >> +
> >> >> +	s = strchr(name, ':');
> >> 
> >> Hmm.. this should be strnchr(). I re-factored the code that moved the 
> >> strnlen() above this below. I'll fix this.
> >> 
> >> >> +	if (s) {
> >> >> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
> >> > 
> >> > If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.
> >> 
> >> Hmm.. not sure I follow. Are you saying the check for sym_len <= 0 is 
> >> not needed?
> > 
> > You can check sym_len != 0, but anyway, here we concern about
> > "longer" string (for performance reason), we can focus on
> > such case.
> > (BTW, could you also check the name != NULL at first?)
> > 
> > So, what I think it can be;
> > 
> > if (strnlen(s+1, KSYM_NAME_LEN) == KSYM_NAME_LEN ||
> >     (size_t)(s - name) >= MODULE_NAME_LEN)
> > 	return false;
> 
> Sure, thanks. I clearly need to refactor this code better!
> 
> - Naveen
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 3/7] kprobes: validate the symbol name length

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 6a128f3a7ed1..bb86681c8a10 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>  	return false;
>  }
>  
> +bool is_valid_kprobe_symbol_name(const char *name)
> +{
> +	size_t sym_len;
> +	char *s;
> +
> +	s = strchr(name, ':');
> +	if (s) {
> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
> +			return false;
> +		sym_len = (size_t)(s - name);
> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> +			return false;
> +	} else {
> +		sym_len = strnlen(name, MODULE_NAME_LEN);
> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> +			return false;
> +	}

I think this is probably more elaborate than it needs to be.

Why not just check the string is <= (MODULE_NAME_LEN + KSYM_NAME_LEN) ?

cheers

Re: [PATCH v3 3/7] kprobes: validate the symbol name length

[Naveen N. Rao]

Excerpts from Michael Ellerman's message of April 20, 2017 11:38:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 6a128f3a7ed1..bb86681c8a10 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>>  	return false;
>>  }
>>  
>> +bool is_valid_kprobe_symbol_name(const char *name)
>> +{
>> +	size_t sym_len;
>> +	char *s;
>> +
>> +	s = strchr(name, ':');
>> +	if (s) {
>> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
>> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
>> +			return false;
>> +		sym_len = (size_t)(s - name);
>> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
>> +			return false;
>> +	} else {
>> +		sym_len = strnlen(name, MODULE_NAME_LEN);
>> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
>> +			return false;
>> +	}
> 
> I think this is probably more elaborate than it needs to be.
> 
> Why not just check the string is <= (MODULE_NAME_LEN + KSYM_NAME_LEN) ?

Yes, that would be sufficient for now.

It's probably just me being paranoid, but I felt it's good to have 
stricter checks for user-provided strings, to guard against future 
changes to how we process this.

- Naveen

[PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

[Naveen N. Rao]

When a kprobe is being registered, we use the symbol_name field to
lookup the address where the probe should be placed. Since this is a
user-provided field, let's ensure that the length of the string is
within expected limits.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Masami, Michael,
Here's a simplified version that should hopefully be fine. I have
simplified the logic to keep the code compact and simple.

- Naveen


 include/linux/kprobes.h     |  1 +
 kernel/kprobes.c            | 30 ++++++++++++++++++++++++++++++
 kernel/trace/trace_kprobe.c |  4 ++++
 3 files changed, 35 insertions(+)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 1f82a3db00b1..4ee10fef5135 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -405,6 +405,7 @@ int disable_kprobe(struct kprobe *kp);
 int enable_kprobe(struct kprobe *kp);
 
 void dump_kprobe(struct kprobe *kp);
+bool is_valid_kprobe_symbol_name(const char *name);
 
 #else /* !CONFIG_KPROBES: */
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 6a128f3a7ed1..ff9b1ac72a38 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
 }
 
 /*
+ * We mainly want to ensure that the provided string is of a reasonable length
+ * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
+ * further.
+ * We don't worry about invalid characters as those will just prevent
+ * matching existing kallsyms.
+ */
+bool is_valid_kprobe_symbol_name(const char *name)
+{
+	size_t sym_len;
+	const char *s;
+
+	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
+	if (s) {
+		sym_len = (size_t)(s - name);
+		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)
+			return false;
+		s++;
+	} else
+		s = name;
+
+	sym_len = strnlen(s, KSYM_NAME_LEN);
+	if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
+		return false;
+
+	return true;
+}
+
+/*
  * If we have a symbol_name argument, look it up and add the offset field
  * to it. This way, we can specify a relative address to a symbol.
  * This returns encoded errors if it fails to look up symbol or invalid
@@ -1397,6 +1425,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
 		goto invalid;
 
 	if (p->symbol_name) {
+		if (!is_valid_kprobe_symbol_name(p->symbol_name))
+			return ERR_PTR(-EINVAL);
 		addr = kprobe_lookup_name(p->symbol_name, p->offset);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5f688cc724f0..bf73e5f31128 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
 			pr_info("Return probe must be used without offset.\n");
 			return -EINVAL;
 		}
+		if (!is_valid_kprobe_symbol_name(symbol)) {
+			pr_info("Symbol name is too long.\n");
+			return -EINVAL;
+		}
 	}
 	argc -= 2; argv += 2;
 
-- 
2.12.1

Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

[Paul Clarke]

a nit or two, below...

On 04/21/2017 07:32 AM, Naveen N. Rao wrote:
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 6a128f3a7ed1..ff9b1ac72a38 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
>  }
> 
>  /*
> + * We mainly want to ensure that the provided string is of a reasonable length
> + * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
> + * further.
> + * We don't worry about invalid characters as those will just prevent
> + * matching existing kallsyms.
> + */
> +bool is_valid_kprobe_symbol_name(const char *name)
> +{
> +	size_t sym_len;
> +	const char *s;
> +
> +	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
> +	if (s) {
> +		sym_len = (size_t)(s - name);
> +		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)

"sym_len <= 0" looks odd here, since sym_len is likely unsigned and would never be less than zero, anyway.

> +			return false;
> +		s++;
> +	} else
> +		s = name;
> +
> +	sym_len = strnlen(s, KSYM_NAME_LEN);
> +	if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)

here, too.

> +		return false;
> +
> +	return true;
> +}

PC

Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

[Naveen N. Rao]

Excerpts from Paul Clarke's message of April 21, 2017 18:41:
> a nit or two, below...
> 
> On 04/21/2017 07:32 AM, Naveen N. Rao wrote:
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 6a128f3a7ed1..ff9b1ac72a38 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
>>  }
>> 
>>  /*
>> + * We mainly want to ensure that the provided string is of a reasonable length
>> + * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
>> + * further.
>> + * We don't worry about invalid characters as those will just prevent
>> + * matching existing kallsyms.
>> + */
>> +bool is_valid_kprobe_symbol_name(const char *name)
>> +{
>> +	size_t sym_len;
>> +	const char *s;
>> +
>> +	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
>> +	if (s) {
>> +		sym_len = (size_t)(s - name);
>> +		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)
> 
> "sym_len <= 0" looks odd here, since sym_len is likely unsigned and would never be less than zero, anyway.

Ugh.. habits :/
I'll wait for Masami/Michael's feedback before re-spinning.

Thanks for the review,
- Naveen

Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

[Masami Hiramatsu]

On Fri, 21 Apr 2017 18:02:33 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> When a kprobe is being registered, we use the symbol_name field to
> lookup the address where the probe should be placed. Since this is a
> user-provided field, let's ensure that the length of the string is
> within expected limits.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Masami, Michael,
> Here's a simplified version that should hopefully be fine. I have
> simplified the logic to keep the code compact and simple.

Ok, so as I replied to older version,
 - Ensure name != NULL at first.
 - Define it in kernel/kallsyms.c as a general routine.

Since this validate the string which will be passed to kallsyms,
I think it should be provided by kallsyms.

Thanks, 

> 
> - Naveen
> 
> 
>  include/linux/kprobes.h     |  1 +
>  kernel/kprobes.c            | 30 ++++++++++++++++++++++++++++++
>  kernel/trace/trace_kprobe.c |  4 ++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 1f82a3db00b1..4ee10fef5135 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -405,6 +405,7 @@ int disable_kprobe(struct kprobe *kp);
>  int enable_kprobe(struct kprobe *kp);
>  
>  void dump_kprobe(struct kprobe *kp);
> +bool is_valid_kprobe_symbol_name(const char *name);
>  
>  #else /* !CONFIG_KPROBES: */
>  
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 6a128f3a7ed1..ff9b1ac72a38 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
>  }
>  
>  /*
> + * We mainly want to ensure that the provided string is of a reasonable length
> + * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
> + * further.
> + * We don't worry about invalid characters as those will just prevent
> + * matching existing kallsyms.
> + */
> +bool is_valid_kprobe_symbol_name(const char *name)
> +{
> +	size_t sym_len;
> +	const char *s;
> +
> +	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
> +	if (s) {
> +		sym_len = (size_t)(s - name);
> +		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)
> +			return false;
> +		s++;
> +	} else
> +		s = name;
> +
> +	sym_len = strnlen(s, KSYM_NAME_LEN);
> +	if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
>   * If we have a symbol_name argument, look it up and add the offset field
>   * to it. This way, we can specify a relative address to a symbol.
>   * This returns encoded errors if it fails to look up symbol or invalid
> @@ -1397,6 +1425,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>  		goto invalid;
>  
>  	if (p->symbol_name) {
> +		if (!is_valid_kprobe_symbol_name(p->symbol_name))
> +			return ERR_PTR(-EINVAL);
>  		addr = kprobe_lookup_name(p->symbol_name, p->offset);
>  		if (!addr)
>  			return ERR_PTR(-ENOENT);
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5f688cc724f0..bf73e5f31128 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
>  			pr_info("Return probe must be used without offset.\n");
>  			return -EINVAL;
>  		}
> +		if (!is_valid_kprobe_symbol_name(symbol)) {
> +			pr_info("Symbol name is too long.\n");
> +			return -EINVAL;
> +		}
>  	}
>  	argc -= 2; argv += 2;
>  
> -- 
> 2.12.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> When a kprobe is being registered, we use the symbol_name field to
> lookup the address where the probe should be placed. Since this is a
> user-provided field, let's ensure that the length of the string is
> within expected limits.

What are we actually trying to protect against here?

If you ignore powerpc for a moment, kprobe_lookup_name() is just
kallsyms_lookup_name().

All kallsyms_lookup_name() does with name is strcmp() it against a
legitimate symbol name which is at most KSYM_NAME_LEN.

So I don't think any of this validation helps in that case?

In the powerpc version of kprobe_lookup_name() we do need to do some
string juggling, for which it helps to know the input is sane. But I
think we should just make that code more robust by checking the input
before we do anything with it.

cheers

Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

[Naveen N. Rao]

Excerpts from Michael Ellerman's message of April 22, 2017 11:25:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
>> When a kprobe is being registered, we use the symbol_name field to
>> lookup the address where the probe should be placed. Since this is a
>> user-provided field, let's ensure that the length of the string is
>> within expected limits.
> 
> What are we actually trying to protect against here?
> 
> If you ignore powerpc for a moment, kprobe_lookup_name() is just
> kallsyms_lookup_name().
> 
> All kallsyms_lookup_name() does with name is strcmp() it against a
> legitimate symbol name which is at most KSYM_NAME_LEN.
> 
> So I don't think any of this validation helps in that case?

It does:
https://patchwork.kernel.org/patch/9695139/

It is far too easy to cause a OOPS due to the above, though this is 
root-only (for modules).

As I stated earlier, I think it is always a good practice to validate 
inputs right on entry, rather than later. Code gets refactored, 
different arch support gets added, and so on.

Doing this validation here ensures we don't have to worry about how we 
process this later, or if another arch has to over-ride 
kprobe_lookup_name().

> 
> In the powerpc version of kprobe_lookup_name() we do need to do some
> string juggling, for which it helps to know the input is sane. But I
> think we should just make that code more robust by checking the input
> before we do anything with it.

Ok, I will fold those tests in with the powerpc implementation for now 
and consider a patch against kallsyms_lookup_name(), like Masami 
recommends.


- Naveen

[PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

[Naveen N. Rao]

Convert usage of strchr()/strncpy()/strncat() to
strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.

Reported-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Changes: Additionally convert the strchr().


 arch/powerpc/kernel/kprobes.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 97b5eed1f76d..c73fb6e3b43f 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
 	const char *modsym;
 	bool dot_appended = false;
-	if ((modsym = strchr(name, ':')) != NULL) {
+	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {
 		modsym++;
 		if (*modsym != '\0' && *modsym != '.') {
 			/* Convert to <module:.symbol> */
-			strncpy(dot_name, name, modsym - name);
+			memcpy(dot_name, name, modsym - name);
 			dot_name[modsym - name] = '.';
 			dot_name[modsym - name + 1] = '\0';
-			strncat(dot_name, modsym,
-				sizeof(dot_name) - (modsym - name) - 2);
+			strlcat(dot_name, modsym, sizeof(dot_name));
 			dot_appended = true;
 		} else {
 			dot_name[0] = '\0';
-			strncat(dot_name, name, sizeof(dot_name) - 1);
+			strlcat(dot_name, name, sizeof(dot_name));
 		}
 	} else if (name[0] != '.') {
 		dot_name[0] = '.';
 		dot_name[1] = '\0';
-		strncat(dot_name, name, KSYM_NAME_LEN - 2);
+		strlcat(dot_name, name, sizeof(dot_name));
 		dot_appended = true;
 	} else {
 		dot_name[0] = '\0';
-		strncat(dot_name, name, KSYM_NAME_LEN - 1);
+		strlcat(dot_name, name, sizeof(dot_name));
 	}
 	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
 	if (!addr && dot_appended) {
-- 
2.12.1

Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

[Paul Clarke]

On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
> Convert usage of strchr()/strncpy()/strncat() to
> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.
> 
> Reported-by: David Laight <David.Laight@ACULAB.COM>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Changes: Additionally convert the strchr().
> 
> 
>  arch/powerpc/kernel/kprobes.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 97b5eed1f76d..c73fb6e3b43f 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>  	const char *modsym;
>  	bool dot_appended = false;
> -	if ((modsym = strchr(name, ':')) != NULL) {
> +	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {
>  		modsym++;
>  		if (*modsym != '\0' && *modsym != '.') {
>  			/* Convert to <module:.symbol> */
> -			strncpy(dot_name, name, modsym - name);
> +			memcpy(dot_name, name, modsym - name);
>  			dot_name[modsym - name] = '.';
>  			dot_name[modsym - name + 1] = '\0';
> -			strncat(dot_name, modsym,
> -				sizeof(dot_name) - (modsym - name) - 2);
> +			strlcat(dot_name, modsym, sizeof(dot_name));

Would it be more efficient here to replace this:
--
dot_name[modsym - name + 1] = '\0';
strlcat(dot_name, modsym, sizeof(dot_name));
--

with this:
strncpy(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN);

(So you aren't rescanning dot_name to find the end, when you already know the end position?)

>  			dot_appended = true;
>  		} else {
>  			dot_name[0] = '\0';
> -			strncat(dot_name, name, sizeof(dot_name) - 1);
> +			strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(dot_name, name, sizeof(dot_name));

(and remove the null termination immediately above)

>  		}
>  	} else if (name[0] != '.') {
>  		dot_name[0] = '.';
>  		dot_name[1] = '\0';
> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
> +		strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(&dot_name[1], name, sizeof(dot_name));

(and remove the null termination immediately above)

>  		dot_appended = true;
>  	} else {
>  		dot_name[0] = '\0';
> -		strncat(dot_name, name, KSYM_NAME_LEN - 1);
> +		strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(dot_name, name, sizeof(dot_name));

(and remove the null termination immediately above)

>  	}

PC

Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

[Paul Clarke]

On 04/21/2017 08:33 AM, Paul Clarke wrote:
> On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
>>  	} else if (name[0] != '.') {
>>  		dot_name[0] = '.';
>>  		dot_name[1] = '\0';
>> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
>> +		strlcat(dot_name, name, sizeof(dot_name));
> 
> and here do:
> strncpy(&dot_name[1], name, sizeof(dot_name));

oops.  s/sizeof(dot_name)/sizeof(dot_name) - 1/

PC

Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

[Paul Clarke]

Sent too soon.  The suggestions don't guarantee null termination.  Refined, below. (Sorry for the noise.)

On 04/21/2017 08:33 AM, Paul Clarke wrote:
> On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
>> Convert usage of strchr()/strncpy()/strncat() to
>> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.

>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>> index 97b5eed1f76d..c73fb6e3b43f 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>>  	const char *modsym;
>>  	bool dot_appended = false;
>> -	if ((modsym = strchr(name, ':')) != NULL) {
>> +	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {
>>  		modsym++;
>>  		if (*modsym != '\0' && *modsym != '.') {
>>  			/* Convert to <module:.symbol> */
>> -			strncpy(dot_name, name, modsym - name);
>> +			memcpy(dot_name, name, modsym - name);
>>  			dot_name[modsym - name] = '.';
>>  			dot_name[modsym - name + 1] = '\0';
>> -			strncat(dot_name, modsym,
>> -				sizeof(dot_name) - (modsym - name) - 2);
>> +			strlcat(dot_name, modsym, sizeof(dot_name));
> 
> Would it be more efficient here to replace this:
> --
> dot_name[modsym - name + 1] = '\0';
> strlcat(dot_name, modsym, sizeof(dot_name));
> --
> 
> with this:
> strncpy(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN);

keep the null termination, and just adjust the starting point for strlcat:
--
dot_name[modsym - name + 1] = '\0';
strlcat(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN);
--

> (So you aren't rescanning dot_name to find the end, when you already know the end position?)
> 
>>  			dot_appended = true;
>>  		} else {
>>  			dot_name[0] = '\0';
>> -			strncat(dot_name, name, sizeof(dot_name) - 1);
>> +			strlcat(dot_name, name, sizeof(dot_name));
> 
> and here do:
> strncpy(dot_name, name, sizeof(dot_name));
> 
> (and remove the null termination immediately above)

nevermind.  :-)

> 
>>  		}
>>  	} else if (name[0] != '.') {
>>  		dot_name[0] = '.';
>>  		dot_name[1] = '\0';
>> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
>> +		strlcat(dot_name, name, sizeof(dot_name));
> 
> and here do:
> strncpy(&dot_name[1], name, sizeof(dot_name));
> 
> (and remove the null termination immediately above)
> 

nevermind.  :-)

>>  		dot_appended = true;
>>  	} else {
>>  		dot_name[0] = '\0';
>> -		strncat(dot_name, name, KSYM_NAME_LEN - 1);
>> +		strlcat(dot_name, name, sizeof(dot_name));
> 
> and here do:
> strncpy(dot_name, name, sizeof(dot_name));
> 
> (and remove the null termination immediately above)
> 
>>  	}

nevermind.  :-)

PC

Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

[Naveen N. Rao]

Excerpts from Paul Clarke's message of April 21, 2017 19:22:
> Sent too soon.  The suggestions don't guarantee null termination.  Refined, below. (Sorry for the noise.)

Yeah, the string operations here are a bit of a minefield...

> 
> On 04/21/2017 08:33 AM, Paul Clarke wrote:
>> On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
>>> Convert usage of strchr()/strncpy()/strncat() to
>>> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.
> 
>>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>>> index 97b5eed1f76d..c73fb6e3b43f 100644
>>> --- a/arch/powerpc/kernel/kprobes.c
>>> +++ b/arch/powerpc/kernel/kprobes.c
>>> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>>>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>>>  	const char *modsym;
>>>  	bool dot_appended = false;
>>> -	if ((modsym = strchr(name, ':')) != NULL) {
>>> +	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {

... as I just realized this is an epic FAIL! ;/

I will take my time to redo this.

- Naveen

[PATCH v3 4/7] powerpc: kprobes: use safer string functions in kprobe_lookup_name()

[Naveen N. Rao]

Convert usage of strncpy()/strncat() to memcpy()/strlcat() for simpler
and safer string manipulation.

Reported-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 97b5eed1f76d..d743bacefa8c 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -69,24 +69,23 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 		modsym++;
 		if (*modsym != '\0' && *modsym != '.') {
 			/* Convert to <module:.symbol> */
-			strncpy(dot_name, name, modsym - name);
+			memcpy(dot_name, name, modsym - name);
 			dot_name[modsym - name] = '.';
 			dot_name[modsym - name + 1] = '\0';
-			strncat(dot_name, modsym,
-				sizeof(dot_name) - (modsym - name) - 2);
+			strlcat(dot_name, modsym, sizeof(dot_name));
 			dot_appended = true;
 		} else {
 			dot_name[0] = '\0';
-			strncat(dot_name, name, sizeof(dot_name) - 1);
+			strlcat(dot_name, name, sizeof(dot_name));
 		}
 	} else if (name[0] != '.') {
 		dot_name[0] = '.';
 		dot_name[1] = '\0';
-		strncat(dot_name, name, KSYM_NAME_LEN - 2);
+		strlcat(dot_name, name, sizeof(dot_name));
 		dot_appended = true;
 	} else {
 		dot_name[0] = '\0';
-		strncat(dot_name, name, KSYM_NAME_LEN - 1);
+		strlcat(dot_name, name, sizeof(dot_name));
 	}
 	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
 	if (!addr && dot_appended) {
-- 
2.12.1

RE: [PATCH v3 4/7] powerpc: kprobes: use safer string functions in kprobe_lookup_name()

[David Laight]

From: Naveen N. Rao
> Sent: 19 April 2017 13:51
...
>  			dot_name[0] = '\0';
> -			strncat(dot_name, name, sizeof(dot_name) - 1);
> +			strlcat(dot_name, name, sizeof(dot_name));
...

Is that really zeroing the first byte just so it can append to it?

	David

[PATCH v3 5/7] powerpc: kprobes: factor out code to emulate instruction into a helper

[Naveen N. Rao]

No functional changes.

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 52 ++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index d743bacefa8c..46e8c1e03ce4 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -206,6 +206,35 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 	regs->link = (unsigned long)kretprobe_trampoline;
 }
 
+int __kprobes try_to_emulate(struct kprobe *p, struct pt_regs *regs)
+{
+	int ret;
+	unsigned int insn = *p->ainsn.insn;
+
+	/* regs->nip is also adjusted if emulate_step returns 1 */
+	ret = emulate_step(regs, insn);
+	if (ret > 0) {
+		/*
+		 * Once this instruction has been boosted
+		 * successfully, set the boostable flag
+		 */
+		if (unlikely(p->ainsn.boostable == 0))
+			p->ainsn.boostable = 1;
+	} else if (ret < 0) {
+		/*
+		 * We don't allow kprobes on mtmsr(d)/rfi(d), etc.
+		 * So, we should never get here... but, its still
+		 * good to catch them, just in case...
+		 */
+		printk("Can't step on instruction %x\n", insn);
+		BUG();
+	} else if (ret == 0)
+		/* This instruction can't be boosted */
+		p->ainsn.boostable = -1;
+
+	return ret;
+}
+
 int __kprobes kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *p;
@@ -301,18 +330,9 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
 
 ss_probe:
 	if (p->ainsn.boostable >= 0) {
-		unsigned int insn = *p->ainsn.insn;
+		ret = try_to_emulate(p, regs);
 
-		/* regs->nip is also adjusted if emulate_step returns 1 */
-		ret = emulate_step(regs, insn);
 		if (ret > 0) {
-			/*
-			 * Once this instruction has been boosted
-			 * successfully, set the boostable flag
-			 */
-			if (unlikely(p->ainsn.boostable == 0))
-				p->ainsn.boostable = 1;
-
 			if (p->post_handler)
 				p->post_handler(p, regs, 0);
 
@@ -320,17 +340,7 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
 			reset_current_kprobe();
 			preempt_enable_no_resched();
 			return 1;
-		} else if (ret < 0) {
-			/*
-			 * We don't allow kprobes on mtmsr(d)/rfi(d), etc.
-			 * So, we should never get here... but, its still
-			 * good to catch them, just in case...
-			 */
-			printk("Can't step on instruction %x\n", insn);
-			BUG();
-		} else if (ret == 0)
-			/* This instruction can't be boosted */
-			p->ainsn.boostable = -1;
+		}
 	}
 	prepare_singlestep(p, regs);
 	kcb->kprobe_status = KPROBE_HIT_SS;
-- 
2.12.1

Re: [PATCH v3 5/7] powerpc: kprobes: factor out code to emulate instruction into a helper

[Masami Hiramatsu]

On Wed, 19 Apr 2017 18:21:04 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

Factor out code to emulate instruction into a try_to_emulate()
helper function. This makes ...

> No functional changes.

Thanks,

> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 52 ++++++++++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index d743bacefa8c..46e8c1e03ce4 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -206,6 +206,35 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  	regs->link = (unsigned long)kretprobe_trampoline;
>  }
>  
> +int __kprobes try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> +{
> +	int ret;
> +	unsigned int insn = *p->ainsn.insn;
> +
> +	/* regs->nip is also adjusted if emulate_step returns 1 */
> +	ret = emulate_step(regs, insn);
> +	if (ret > 0) {
> +		/*
> +		 * Once this instruction has been boosted
> +		 * successfully, set the boostable flag
> +		 */
> +		if (unlikely(p->ainsn.boostable == 0))
> +			p->ainsn.boostable = 1;
> +	} else if (ret < 0) {
> +		/*
> +		 * We don't allow kprobes on mtmsr(d)/rfi(d), etc.
> +		 * So, we should never get here... but, its still
> +		 * good to catch them, just in case...
> +		 */
> +		printk("Can't step on instruction %x\n", insn);
> +		BUG();
> +	} else if (ret == 0)
> +		/* This instruction can't be boosted */
> +		p->ainsn.boostable = -1;
> +
> +	return ret;
> +}
> +
>  int __kprobes kprobe_handler(struct pt_regs *regs)
>  {
>  	struct kprobe *p;
> @@ -301,18 +330,9 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
>  
>  ss_probe:
>  	if (p->ainsn.boostable >= 0) {
> -		unsigned int insn = *p->ainsn.insn;
> +		ret = try_to_emulate(p, regs);
>  
> -		/* regs->nip is also adjusted if emulate_step returns 1 */
> -		ret = emulate_step(regs, insn);
>  		if (ret > 0) {
> -			/*
> -			 * Once this instruction has been boosted
> -			 * successfully, set the boostable flag
> -			 */
> -			if (unlikely(p->ainsn.boostable == 0))
> -				p->ainsn.boostable = 1;
> -
>  			if (p->post_handler)
>  				p->post_handler(p, regs, 0);
>  
> @@ -320,17 +340,7 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
>  			reset_current_kprobe();
>  			preempt_enable_no_resched();
>  			return 1;
> -		} else if (ret < 0) {
> -			/*
> -			 * We don't allow kprobes on mtmsr(d)/rfi(d), etc.
> -			 * So, we should never get here... but, its still
> -			 * good to catch them, just in case...
> -			 */
> -			printk("Can't step on instruction %x\n", insn);
> -			BUG();
> -		} else if (ret == 0)
> -			/* This instruction can't be boosted */
> -			p->ainsn.boostable = -1;
> +		}
>  	}
>  	prepare_singlestep(p, regs);
>  	kcb->kprobe_status = KPROBE_HIT_SS;
> -- 
> 2.12.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [v3, 5/7] powerpc: kprobes: factor out code to emulate instruction into a helper

[Michael Ellerman]

On Wed, 2017-04-19 at 12:51:04 UTC, "Naveen N. Rao" wrote:
> No functional changes.
> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/1cabd2f8f720a0ed612139547acb65

cheers

[PATCH v3 6/7] powerpc: kprobes: emulate instructions on kprobe handler re-entry

[Naveen N. Rao]

On kprobe handler re-entry, try to emulate the instruction rather than
single stepping always.

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 46e8c1e03ce4..067e9863bfdf 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -276,6 +276,14 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
 			kprobes_inc_nmissed_count(p);
 			prepare_singlestep(p, regs);
 			kcb->kprobe_status = KPROBE_REENTER;
+			if (p->ainsn.boostable >= 0) {
+				ret = try_to_emulate(p, regs);
+
+				if (ret > 0) {
+					restore_previous_kprobe(kcb);
+					return 1;
+				}
+			}
 			return 1;
 		} else {
 			if (*addr != BREAKPOINT_INSTRUCTION) {
-- 
2.12.1

Re: [PATCH v3 6/7] powerpc: kprobes: emulate instructions on kprobe handler re-entry

[Masami Hiramatsu]

BTW, as I pointed, 5/7 and 6/7 should be merged since this actually
makes meaningful change.

Thank you,

On Wed, 19 Apr 2017 18:21:05 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On kprobe handler re-entry, try to emulate the instruction rather than
> single stepping always.
> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 46e8c1e03ce4..067e9863bfdf 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -276,6 +276,14 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
>  			kprobes_inc_nmissed_count(p);
>  			prepare_singlestep(p, regs);
>  			kcb->kprobe_status = KPROBE_REENTER;
> +			if (p->ainsn.boostable >= 0) {
> +				ret = try_to_emulate(p, regs);
> +
> +				if (ret > 0) {
> +					restore_previous_kprobe(kcb);
> +					return 1;
> +				}
> +			}
>  			return 1;
>  		} else {
>  			if (*addr != BREAKPOINT_INSTRUCTION) {
> -- 
> 2.12.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 6/7] powerpc: kprobes: emulate instructions on kprobe handler re-entry

[Naveen N. Rao]

Excerpts from Masami Hiramatsu's message of April 19, 2017 20:13:
> 
> BTW, as I pointed, 5/7 and 6/7 should be merged since this actually
> makes meaningful change.

Yes, sorry if I wasn't clear in my previous reply in the (!) previous 
patch series.

Since this has to go through the powerpc tree, I followed this since I 
felt that Michael Ellerman prefers to keep functional changes separate 
from refactoring. I'm fine with either approach.

Michael?

Thanks!
- Naveen

> 
> Thank you,
> 
> On Wed, 19 Apr 2017 18:21:05 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> On kprobe handler re-entry, try to emulate the instruction rather than
>> single stepping always.
>> 
>> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/kernel/kprobes.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>> index 46e8c1e03ce4..067e9863bfdf 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -276,6 +276,14 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
>>  			kprobes_inc_nmissed_count(p);
>>  			prepare_singlestep(p, regs);
>>  			kcb->kprobe_status = KPROBE_REENTER;
>> +			if (p->ainsn.boostable >= 0) {
>> +				ret = try_to_emulate(p, regs);
>> +
>> +				if (ret > 0) {
>> +					restore_previous_kprobe(kcb);
>> +					return 1;
>> +				}
>> +			}
>>  			return 1;
>>  		} else {
>>  			if (*addr != BREAKPOINT_INSTRUCTION) {
>> -- 
>> 2.12.1
>> 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
> 
> 

Re: [PATCH v3 6/7] powerpc: kprobes: emulate instructions on kprobe handler re-entry

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> Excerpts from Masami Hiramatsu's message of April 19, 2017 20:13:
>> 
>> BTW, as I pointed, 5/7 and 6/7 should be merged since this actually
>> makes meaningful change.
>
> Yes, sorry if I wasn't clear in my previous reply in the (!) previous 
> patch series.
>
> Since this has to go through the powerpc tree, I followed this since I 
> felt that Michael Ellerman prefers to keep functional changes separate 
> from refactoring. I'm fine with either approach.
>
> Michael?

Yeah I'd definitely like this to be two patches. Otherwise when reading
the combined diff it's much harder to see the change.

cheers

Re: [PATCH v3 6/7] powerpc: kprobes: emulate instructions on kprobe handler re-entry

[Masami Hiramatsu]

On Thu, 20 Apr 2017 16:11:10 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > Excerpts from Masami Hiramatsu's message of April 19, 2017 20:13:
> >> 
> >> BTW, as I pointed, 5/7 and 6/7 should be merged since this actually
> >> makes meaningful change.
> >
> > Yes, sorry if I wasn't clear in my previous reply in the (!) previous 
> > patch series.
> >
> > Since this has to go through the powerpc tree, I followed this since I 
> > felt that Michael Ellerman prefers to keep functional changes separate 
> > from refactoring. I'm fine with either approach.
> >
> > Michael?
> 
> Yeah I'd definitely like this to be two patches. Otherwise when reading
> the combined diff it's much harder to see the change.

OK, let it be so.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [v3, 6/7] powerpc: kprobes: emulate instructions on kprobe handler re-entry

[Michael Ellerman]

On Wed, 2017-04-19 at 12:51:05 UTC, "Naveen N. Rao" wrote:
> On kprobe handler re-entry, try to emulate the instruction rather than
> single stepping always.
> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/22d8b3dec214cd43a773f621f95d25

cheers

[PATCH v3 7/7] powerpc: kprobes: remove duplicate saving of msr

[Naveen N. Rao]

set_current_kprobe() already saves regs->msr into kprobe_saved_msr. Remove
the redundant save.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 067e9863bfdf..5c0a1ffcbcf9 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -272,7 +272,6 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
 			 */
 			save_previous_kprobe(kcb);
 			set_current_kprobe(p, regs, kcb);
-			kcb->kprobe_saved_msr = regs->msr;
 			kprobes_inc_nmissed_count(p);
 			prepare_singlestep(p, regs);
 			kcb->kprobe_status = KPROBE_REENTER;
-- 
2.12.1

Re: [PATCH v3 7/7] powerpc: kprobes: remove duplicate saving of msr

[Masami Hiramatsu]

On Wed, 19 Apr 2017 18:21:06 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> set_current_kprobe() already saves regs->msr into kprobe_saved_msr. Remove
> the redundant save.
> 

Looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 067e9863bfdf..5c0a1ffcbcf9 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -272,7 +272,6 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
>  			 */
>  			save_previous_kprobe(kcb);
>  			set_current_kprobe(p, regs, kcb);
> -			kcb->kprobe_saved_msr = regs->msr;
>  			kprobes_inc_nmissed_count(p);
>  			prepare_singlestep(p, regs);
>  			kcb->kprobe_status = KPROBE_REENTER;
> -- 
> 2.12.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [v3,7/7] powerpc: kprobes: remove duplicate saving of msr

[Michael Ellerman]

On Wed, 2017-04-19 at 12:51:06 UTC, "Naveen N. Rao" wrote:
> set_current_kprobe() already saves regs->msr into kprobe_saved_msr. Remove
> the redundant save.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/d08f8a28bcc8c2004a7186839148fc

cheers