LinuxLists.cc - [PATCH v3 0/7] powerpc: a few kprobe fixes and refactoring

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Naveen N. Rao <[email protected]>
---
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

2017-04-19 12:51:37

[permalink] [raw]

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

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 <[email protected]>
---
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

2017-04-19 12:51:41

[permalink] [raw]

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

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

Reported-by: David Laight <[email protected]>
Signed-off-by: Naveen N. Rao <[email protected]>
---
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

2017-04-19 12:52:18

[permalink] [raw]

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

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 <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Naveen N. Rao <[email protected]>
---
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

2017-04-19 12:52:03

[permalink] [raw]

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

No functional changes.

Acked-by: Ananth N Mavinakayanahalli <[email protected]>
Signed-off-by: Naveen N. Rao <[email protected]>
---
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

2017-04-19 12:52:42

[permalink] [raw]

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

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

Acked-by: Ananth N Mavinakayanahalli <[email protected]>
Signed-off-by: Naveen N. Rao <[email protected]>
---
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

2017-04-19 12:54:59

[permalink] [raw]

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

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

Signed-off-by: Naveen N. Rao <[email protected]>
---
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

2017-04-19 14:40:50

[permalink] [raw]

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

On Wed, 19 Apr 2017 18:21:04 +0530
"Naveen N. Rao" <[email protected]> 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 <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>
> ---
> 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 <[email protected]>

2017-04-19 14:43:33

[permalink] [raw]

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

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" <[email protected]> wrote:

> On kprobe handler re-entry, try to emulate the instruction rather than
> single stepping always.
>
> Acked-by: Ananth N Mavinakayanahalli <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>
> ---
> 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 <[email protected]>

2017-04-19 14:43:59

[permalink] [raw]

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

On Wed, 19 Apr 2017 18:21:06 +0530
"Naveen N. Rao" <[email protected]> wrote:

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

Looks good to me.

Reviewed-by: Masami Hiramatsu <[email protected]>

Thank you,

> Signed-off-by: Naveen N. Rao <[email protected]>
> ---
> 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 <[email protected]>

2017-04-19 15:31:17

[permalink] [raw]

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

On Wed, 19 Apr 2017 18:21:02 +0530
"Naveen N. Rao" <[email protected]> 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 <[email protected]>

2017-04-19 16:39:29

[permalink] [raw]

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

Excerpts from Masami Hiramatsu's message of April 19, 2017 20:07:
> On Wed, 19 Apr 2017 18:21:02 +0530
> "Naveen N. Rao" <[email protected]> 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 <[email protected]>
>
>

2017-04-19 16:44:00

[permalink] [raw]

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

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" <[email protected]> wrote:
>
>> On kprobe handler re-entry, try to emulate the instruction rather than
>> single stepping always.
>>
>> Acked-by: Ananth N Mavinakayanahalli <[email protected]>
>> Signed-off-by: Naveen N. Rao <[email protected]>
>> ---
>> 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 <[email protected]>
>
>

2017-04-20 06:08:56

[permalink] [raw]

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

"Naveen N. Rao" <[email protected]> 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

2017-04-20 06:11:15

[permalink] [raw]

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

"Naveen N. Rao" <[email protected]> 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

2017-04-20 07:21:09

[permalink] [raw]

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

Excerpts from Michael Ellerman's message of April 20, 2017 11:38:
> "Naveen N. Rao" <[email protected]> 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

2017-04-21 12:33:43

[permalink] [raw]

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

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 <[email protected]>
---
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

2017-04-21 12:34:26

[permalink] [raw]

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

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

Reported-by: David Laight <[email protected]>
Signed-off-by: Naveen N. Rao <[email protected]>
---
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

2017-04-21 13:11:55

[permalink] [raw]

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

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

2017-04-21 13:26:04

[permalink] [raw]

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

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

2017-04-21 13:33:35

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>
> ---
> 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

2017-04-21 13:36:59

[permalink] [raw]

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

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

2017-04-21 13:42:51

[permalink] [raw]

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

On Wed, 19 Apr 2017 16:38:22 +0000
"Naveen N. Rao" <[email protected]> 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" <[email protected]> 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 <[email protected]>
> >
> >
>

--
Masami Hiramatsu <[email protected]>

2017-04-21 13:48:17

[permalink] [raw]

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

On Thu, 20 Apr 2017 16:11:10 +1000
Michael Ellerman <[email protected]> wrote:

> "Naveen N. Rao" <[email protected]> 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 <[email protected]>

Thanks,

--
Masami Hiramatsu <[email protected]>

2017-04-21 13:52:20

[permalink] [raw]

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

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

2017-04-21 13:54:13

[permalink] [raw]

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

On Fri, 21 Apr 2017 18:02:33 +0530
"Naveen N. Rao" <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>

2017-04-21 15:52:53

by David Laight

[permalink] [raw]

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

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

2017-04-22 05:55:26

[permalink] [raw]

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

"Naveen N. Rao" <[email protected]> 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

2017-04-23 11:53:28

[permalink] [raw]

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

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 <[email protected]>
> Reviewed-by: Masami Hiramatsu <[email protected]>

Applied to powerpc next, thanks.

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

cheers

2017-04-23 15:45:41

[permalink] [raw]

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

Excerpts from Masami Hiramatsu's message of April 21, 2017 19:12:
> On Wed, 19 Apr 2017 16:38:22 +0000
> "Naveen N. Rao" <[email protected]> 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" <[email protected]> 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

2017-04-23 17:12:45

[permalink] [raw]

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

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

2017-04-23 17:42:21

[permalink] [raw]

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

Excerpts from Michael Ellerman's message of April 22, 2017 11:25:
> "Naveen N. Rao" <[email protected]> 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

2017-04-24 22:48:09

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>

Applied to powerpc next, thanks.

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

cheers

2017-04-24 22:48:58

[permalink] [raw]

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

On Wed, 2017-04-19 at 12:51:04 UTC, "Naveen N. Rao" wrote:
> No functional changes.
>
> Acked-by: Ananth N Mavinakayanahalli <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>

Applied to powerpc next, thanks.

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

cheers

2017-04-24 22:49:04

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>

Applied to powerpc next, thanks.

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

cheers

2017-04-24 22:48:48

[permalink] [raw]

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

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 <[email protected]>
> Acked-by: Masami Hiramatsu <[email protected]>
> Signed-off-by: Naveen N. Rao <[email protected]>

Applied to powerpc next, thanks.

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

cheers

2017-04-25 03:18:49