kprobes/x86: RO text code bugfix and cleanup
cover kprobes/x86: RO text code bugfix and cleanup
Hi,
This series fixes a kprobe-x86 bug related to RO text and cleans up addressof operators.
The first one is an obvious bug that misses to set memory RO when the function fails. I've just add set_memory_ro() on error exit.
And the second one is just a cleanup patch to remove addressof operators ("&") since it is meaningless anymore.
Please apply these, since this series is independent from another bugfix related to optprobe discussing in other thread.
Thanks,
---
Masami Hiramatsu (2):
kprobes/x86: Don't forget to set memory back to RO on failure kprobes/x86: Remove addressof operators
arch/x86/include/asm/kprobes.h | 4 ++-- arch/x86/kernel/kprobes/core.c | 4 +++- arch/x86/kernel/kprobes/opt.c | 9 +++++---- 3 files changed, 10 insertions(+), 7 deletions(-)
--
Masami Hiramatsu <[email protected]>
PATCH 1/2 kprobes/x86: Don't forget to set memory back to RO on failure
Do not forget to set kprobes insn buffer memory back to RO on failure path. Without this fix, if there is an unexpected error on copying instructions, kprobes insn buffer kept RW, which can allow unexpected modifying instruction buffer.
--- arch/x86/kernel/kprobes/core.c | 4 +++- arch/x86/kernel/kprobes/opt.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
arch/x86/kernel/kprobes/core.c +3 -1
@@ -435,8 +435,10 @@ static int arch_copy_kprobe(struct kprobe *p)/* Copy an instruction with recovering if other optprobe modifies it.*/ len =__copy_instruction (p->ainsn.insn, p->addr, &insn); -if (!len) +if (!len) { +set_memory_ro ((unsigned long )p->ainsn.insn &PAGE_MASK ,1 );return -EINVAL ; + }/* * __copy_instruction can modify the displacement of the instruction,
arch/x86/kernel/kprobes/opt.c +1 -0
@@ -368,6 +368,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, ret =copy_optimized_instructions (buf +TMPL_END_IDX , op->kp.addr);if (ret <0 ) {__arch_remove_optimized_kprobe (op,0 ); +set_memory_ro ((unsigned long )buf &PAGE_MASK ,1 );return ret; } op->optinsn.size = ret;
Re: [PATCH -tip 1/2] kprobes/x86: Don't forget to set memory back to RO on failure
Quoted 18 lines · Masami Hiramatsu
Do not forget to set kprobes insn buffer memory back to RO on failure path. Without this fix, if there is an unexpected error on copying instructions, kprobes insn buffer kept RW, which can allow unexpected modifying instruction buffer.
Fixes d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")Signed-off-by Masami Hiramatsu <[email protected]>--- arch/x86/kernel/kprobes/core.c | 4 +++- arch/x86/kernel/kprobes/opt.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)arch/x86/kernel/kprobes/core.c +0
-0
arch/x86/kernel/kprobes/core.c static int arch_copy_kprobe(struct kprobe *p) · line 441
Quoted 8 lines
arch/x86/kernel/kprobes/core.c +3
-1@@ -435,8 +435,10 @@ static int arch_copy_kprobe(struct kprobe *p)/* Copy an instruction with recovering if other optprobe modifies it.*/ len =__copy_instruction (p->ainsn.insn, p->addr, &insn); -if (!len) +if (!len) { +set_memory_ro ((unsigned long )p->ainsn.insn &PAGE_MASK ,1 );return -EINVAL ; + }
So variable usage in the arch_copy_kprobe() is really awful: 'p->ainsn.insn' is repeated 6 times!
Please consolidate all that via a helper variable.
Also, regarding the merits of the patch: do we know that the page in question was RO before? If it was RW we'll unexpectedly mark it RO here in the failure path ...
arch/x86/kernel/kprobes/opt.c int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, · line 374
Quoted 11 lines
index
69ea0bc1cfa3..853614560a4f100644arch/x86/kernel/kprobes/opt.c +1
-0@@ -368,6 +368,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, ret =copy_optimized_instructions (buf +TMPL_END_IDX , op->kp.addr);if (ret <0 ) {__arch_remove_optimized_kprobe (op,0 ); +set_memory_ro ((unsigned long )buf &PAGE_MASK ,1 );return ret; } op->optinsn.size = ret;
Ditto.
Thanks,
Ingo
Re: [PATCH -tip 1/2] kprobes/x86: Don't forget to set memory back to RO on failure
Quoted 33 lines · On Thu, 10 Aug 2017 17:29:56 +0200 Ingo Molnar <[email protected]> wrote:
Quoted 18 lines · Masami Hiramatsu
Do not forget to set kprobes insn buffer memory back to RO on failure path. Without this fix, if there is an unexpected error on copying instructions, kprobes insn buffer kept RW, which can allow unexpected modifying instruction buffer.
Fixes d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")Signed-off-by Masami Hiramatsu <[email protected]>--- arch/x86/kernel/kprobes/core.c | 4 +++- arch/x86/kernel/kprobes/opt.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)arch/x86/kernel/kprobes/core.c +0
-0arch/x86/kernel/kprobes/core.c static int arch_copy_kprobe(struct kprobe *p) · line 441
Quoted 8 lines
arch/x86/kernel/kprobes/core.c +3
-1@@ -435,8 +435,10 @@ static int arch_copy_kprobe(struct kprobe *p)/* Copy an instruction with recovering if other optprobe modifies it.*/ len =__copy_instruction (p->ainsn.insn, p->addr, &insn); -if (!len) +if (!len) { +set_memory_ro ((unsigned long )p->ainsn.insn &PAGE_MASK ,1 );return -EINVAL ; + }So variable usage in the
arch_copy_kprobe()is really awful: 'p->ainsn.insn' is repeated 6 times!
Quoted 1 line
Please consolidate all that via a helper variable.
OK, I'll cleanup it soon.
Quoted 3 lines
Also, regarding the merits of the patch: do we know that the page in question was RO before? If it was RW we'll unexpectedly mark it RO here in the failure path ...
No need to take care previous state, this page has to be RO after setup (even if it was failed), since the page is shared by other kprobes. If we missed it, insn buffers for other kprobes will be left in RW state.
Quoted 5 lines
Quoted 5 lines
index
69ea0bc1cfa3..853614560a4f100644arch/x86/kernel/kprobes/opt.c +0
-0@@ -368,6 +368,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, ret =copy_optimized_instructions (buf +TMPL_END_IDX , op->kp.addr);
Quoted 8 lines
arch/x86/kernel/kprobes/opt.c int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, · line 374
Quoted 6 lines
arch/x86/kernel/kprobes/opt.c +1
-0if (ret <0 ) {__arch_remove_optimized_kprobe (op,0 ); +set_memory_ro ((unsigned long )buf &PAGE_MASK ,1 );return ret; } op->optinsn.size = ret;Ditto.
As same as above, this page is shared by other optprobes.
Thank you,
Quoted 4 lines
Thanks,
Ingo
Signature · 1 line
Masami Hiramatsu <[email protected]>
PATCH 2/2 kprobes/x86: Remove addressof operators
In reply to Masami HiramatsuSince commit 54a7d50b9205 ("x86: mark kprobe templates as character arrays, not single characters") changes optprobe_template_* to arrays, we can remove addressof operators from those symbols.
--- arch/x86/include/asm/kprobes.h | 4 ++-- arch/x86/kernel/kprobes/opt.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)
arch/x86/include/asm/kprobes.h +2 -2
@@ -58,8 +58,8 @@ extern __visible kprobe_opcode_t optprobe_template_call[]; extern __visible kprobe_opcode_t optprobe_template_end[];#define MAX_OPTIMIZED_LENGTH (MAX_INSN_SIZE + RELATIVE_ADDR_SIZE)#define MAX_OPTINSN_SIZE \ - (((unsigned long)&optprobe_template_end - \ - (unsigned long)&optprobe_template_entry) + \ + (((unsigned long)optprobe_template_end - \ + (unsigned long)optprobe_template_entry) + \ MAX_OPTIMIZED_LENGTH + RELATIVEJUMP_SIZE)extern const int kretprobe_blacklist_size;
arch/x86/kernel/kprobes/opt.c +4 -4
@@ -141,11 +141,11 @@ void optprobe_template_func(void);STACK_FRAME_NON_STANDARD (optprobe_template_func);#define TMPL_MOVE_IDX \ - ((long)&optprobe_template_val - (long)&optprobe_template_entry) + ((long)optprobe_template_val - (long)optprobe_template_entry)#define TMPL_CALL_IDX \ - ((long)&optprobe_template_call - (long)&optprobe_template_entry) + ((long)optprobe_template_call - (long)optprobe_template_entry)#define TMPL_END_IDX \ - ((long)&optprobe_template_end - (long)&optprobe_template_entry) + ((long)optprobe_template_end - (long)optprobe_template_entry)#define INT3_SIZE sizeof(kprobe_opcode_t)
@@ -374,7 +374,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, op->optinsn.size = ret;/* Copy arch-dep-instance from template */ -memcpy (buf, &optprobe_template_entry,TMPL_END_IDX ); +memcpy (buf, optprobe_template_entry,TMPL_END_IDX );/* Set probe information */ synthesize_set_arg1 (buf +TMPL_MOVE_IDX , (unsigned long )op);