* [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly @ 2018-08-23 17:16 Masami Hiramatsu 2018-08-24 1:41 ` Steven Rostedt ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Masami Hiramatsu @ 2018-08-23 17:16 UTC (permalink / raw) To: Ingo Molnar Cc: Steven Rostedt, Masami Hiramatsu, Ravi Bangoria, Arnaldo Carvalho de Melo, Michael Rodin, linux-kernel Since copy_optimized_instructions() misses to update real RIP address while copying several instructions to working buffer, it adjusts RIP-relative instruction with wrong RIP address for the 2nd and subsequent instructions. This may break the kernel (like kernel freeze) because probed instruction can refer a wrong data. For example, putting kprobes on cpumask_next hit this bug. cpumask_next is normally like below if CONFIG_CPUMASK_OFFSTACK=y (in this case nr_cpumask_bits is an alias of nr_cpu_ids) <cpumask_next>: 48 89 f0 mov %rsi,%rax 8b 35 7b fb e2 00 mov 0xe2fb7b(%rip),%esi # ffffffff82db9e64 <nr_cpu_ids> 55 push %rbp ... If we put a kprobe on it and optimized with jump, it becomes like this. e9 95 7d 07 1e jmpq 0xffffffffa000207a 7b fb jnp 0xffffffff81f8a2e2 <cpumask_next+2> e2 00 loop 0xffffffff81f8a2e9 <cpumask_next+9> 55 push %rbp This shows first 2 "mov" instructions are copied to trampoline buffer at 0xffffffffa000207a. Here is the disassembled result. (skipped optprobe template instructions) Dump of assembler code from 0xffffffffa000207a to 0xffffffffa00020ea: 54 push %rsp ... 48 83 c4 08 add $0x8,%rsp 9d popfq 48 89 f0 mov %rsi,%rax 8b 35 82 7d db e2 mov -0x1d24827e(%rip),%esi # 0xffffffff82db9e67 <nr_cpu_ids+3> As it shows, the 2nd mov accesses *(nr_cpu_ids+3) instead of *nr_cpu_ids. This leads a kernel freeze because cpumask_next() always returns 0 and for_each_cpu() never ended. Fixing this by adding len correctly to real RIP address while copying. Fixes: 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()") Reported-by: Michael Rodin <michael@rodin.online> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Cc: stable@vger.kernel.org --- arch/x86/kernel/kprobes/opt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c index eaf02f2e7300..e92672b8b490 100644 --- a/arch/x86/kernel/kprobes/opt.c +++ b/arch/x86/kernel/kprobes/opt.c @@ -189,7 +189,8 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) int len = 0, ret; while (len < RELATIVEJUMP_SIZE) { - ret = __copy_instruction(dest + len, src + len, real, &insn); + ret = __copy_instruction(dest + len, src + len, real + len, + &insn); if (!ret || !can_boost(&insn, src + len)) return -EINVAL; len += ret; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly 2018-08-23 17:16 [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly Masami Hiramatsu @ 2018-08-24 1:41 ` Steven Rostedt 2018-08-24 7:57 ` Masami Hiramatsu 2018-12-04 8:13 ` Ingo Molnar 2018-12-04 23:05 ` [tip:perf/urgent] kprobes/x86: Fix instruction patching corruption when copying more than one RIP-relative instruction tip-bot for Masami Hiramatsu 2 siblings, 1 reply; 7+ messages in thread From: Steven Rostedt @ 2018-08-24 1:41 UTC (permalink / raw) To: Masami Hiramatsu Cc: Ingo Molnar, Ravi Bangoria, Arnaldo Carvalho de Melo, Michael Rodin, linux-kernel On Fri, 24 Aug 2018 02:16:12 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > Dump of assembler code from 0xffffffffa000207a to 0xffffffffa00020ea: > 54 push %rsp > ... > 48 83 c4 08 add $0x8,%rsp > 9d popfq > 48 89 f0 mov %rsi,%rax > 8b 35 82 7d db e2 mov -0x1d24827e(%rip),%esi > # 0xffffffff82db9e67 <nr_cpu_ids+3> > > As it shows, the 2nd mov accesses *(nr_cpu_ids+3) instead of > *nr_cpu_ids. This leads a kernel freeze because cpumask_next() > always returns 0 and for_each_cpu() never ended. Ouch! Nice catch. > > Fixing this by adding len correctly to real RIP address while > copying. > > Fixes: 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()") > Reported-by: Michael Rodin <michael@rodin.online> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Cc: stable@vger.kernel.org > --- > arch/x86/kernel/kprobes/opt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c > index eaf02f2e7300..e92672b8b490 100644 > --- a/arch/x86/kernel/kprobes/opt.c > +++ b/arch/x86/kernel/kprobes/opt.c > @@ -189,7 +189,8 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) > int len = 0, ret; > > while (len < RELATIVEJUMP_SIZE) { > - ret = __copy_instruction(dest + len, src + len, real, &insn); > + ret = __copy_instruction(dest + len, src + len, real + len, > + &insn); > if (!ret || !can_boost(&insn, src + len)) > return -EINVAL; > len += ret; Looking at the change that broke this we have: > -static int copy_optimized_instructions(u8 *dest, u8 *src) > +static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) > { > struct insn insn; > int len = 0, ret; > > while (len < RELATIVEJUMP_SIZE) { > - ret = __copy_instruction(dest + len, src + len, &insn); > + ret = __copy_instruction(dest + len, src + len, real, &insn); Where "real" was added as a parameter to __copy_instruction. Note that we pass in "dest + len" but not "real + len" as you patch fixes. __copy_instruction was changed by the bad commit with: > -int __copy_instruction(u8 *dest, u8 *src, struct insn *insn) > +int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn) > { > kprobe_opcode_t buf[MAX_INSN_SIZE]; > unsigned long recovered_insn = > @@ -387,11 +388,11 @@ int __copy_instruction(u8 *dest, u8 *src, struct insn *insn) > * have given. > */ > newdisp = (u8 *) src + (s64) insn->displacement.value > - - (u8 *) dest; > + - (u8 *) real; "real" replaces "dest", which was the first parameter to __copy_instruction. > return 0; And: > int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, > struct kprobe *__unused) > { > - u8 *buf; > - int ret; > + u8 *buf = NULL, *slot; > + int ret, len; > long rel; > > if (!can_optimize((unsigned long)op->kp.addr)) > return -EILSEQ; > > - op->optinsn.insn = get_optinsn_slot(); > - if (!op->optinsn.insn) > + buf = kzalloc(MAX_OPTINSN_SIZE, GFP_KERNEL); > + if (!buf) > return -ENOMEM; > > + op->optinsn.insn = slot = get_optinsn_slot(); > + if (!slot) { > + ret = -ENOMEM; > + goto out; > + } > + > /* > * Verify if the address gap is in 2GB range, because this uses > * a relative jump. > */ > - rel = (long)op->optinsn.insn - (long)op->kp.addr + RELATIVEJUMP_SIZE; > + rel = (long)slot - (long)op->kp.addr + RELATIVEJUMP_SIZE; > if (abs(rel) > 0x7fffffff) { > - __arch_remove_optimized_kprobe(op, 0); > - return -ERANGE; > + ret = -ERANGE; > + goto err; > } > > - buf = (u8 *)op->optinsn.insn; "slot" is equivalent to the old "buf". > - set_memory_rw((unsigned long)buf & PAGE_MASK, 1); > + /* Copy arch-dep-instance from template */ > + memcpy(buf, &optprobe_template_entry, TMPL_END_IDX); > > /* Copy instructions into the out-of-line buffer */ > - ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr); > - if (ret < 0) { > - __arch_remove_optimized_kprobe(op, 0); > - return ret; > - } > + ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr, > + slot + TMPL_END_IDX); We pass in "real" as "slot + TMPL_END_IDX" and "dest" as "buf + TMPL_END_IDX", thus to make it be equivalent to the code before this commit, "real" should have "+ len" added to it in order to be equivalent to what was there before. That said... Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > + if (ret < 0) > + goto err; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly 2018-08-24 1:41 ` Steven Rostedt @ 2018-08-24 7:57 ` Masami Hiramatsu 2018-09-03 3:38 ` Masami Hiramatsu 0 siblings, 1 reply; 7+ messages in thread From: Masami Hiramatsu @ 2018-08-24 7:57 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, Ravi Bangoria, Arnaldo Carvalho de Melo, Michael Rodin, linux-kernel On Thu, 23 Aug 2018 21:41:09 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 24 Aug 2018 02:16:12 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > Dump of assembler code from 0xffffffffa000207a to 0xffffffffa00020ea: > > 54 push %rsp > > ... > > 48 83 c4 08 add $0x8,%rsp > > 9d popfq > > 48 89 f0 mov %rsi,%rax > > 8b 35 82 7d db e2 mov -0x1d24827e(%rip),%esi > > # 0xffffffff82db9e67 <nr_cpu_ids+3> > > > > As it shows, the 2nd mov accesses *(nr_cpu_ids+3) instead of > > *nr_cpu_ids. This leads a kernel freeze because cpumask_next() > > always returns 0 and for_each_cpu() never ended. > > Ouch! Nice catch. > > > > > Fixing this by adding len correctly to real RIP address while > > copying. > > > > Fixes: 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()") > > Reported-by: Michael Rodin <michael@rodin.online> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: stable@vger.kernel.org > > --- > > arch/x86/kernel/kprobes/opt.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c > > index eaf02f2e7300..e92672b8b490 100644 > > --- a/arch/x86/kernel/kprobes/opt.c > > +++ b/arch/x86/kernel/kprobes/opt.c > > @@ -189,7 +189,8 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) > > int len = 0, ret; > > > > while (len < RELATIVEJUMP_SIZE) { > > - ret = __copy_instruction(dest + len, src + len, real, &insn); > > + ret = __copy_instruction(dest + len, src + len, real + len, > > + &insn); > > if (!ret || !can_boost(&insn, src + len)) > > return -EINVAL; > > len += ret; > > Looking at the change that broke this we have: > > > -static int copy_optimized_instructions(u8 *dest, u8 *src) > > +static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) > > { > > struct insn insn; > > int len = 0, ret; > > > > while (len < RELATIVEJUMP_SIZE) { > > - ret = __copy_instruction(dest + len, src + len, &insn); > > + ret = __copy_instruction(dest + len, src + len, real, &insn); > > Where "real" was added as a parameter to __copy_instruction. Note that > we pass in "dest + len" but not "real + len" as you patch fixes. > __copy_instruction was changed by the bad commit with: > > > -int __copy_instruction(u8 *dest, u8 *src, struct insn *insn) > > +int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn) > > { > > kprobe_opcode_t buf[MAX_INSN_SIZE]; > > unsigned long recovered_insn = > > @@ -387,11 +388,11 @@ int __copy_instruction(u8 *dest, u8 *src, struct insn *insn) > > * have given. > > */ > > newdisp = (u8 *) src + (s64) insn->displacement.value > > - - (u8 *) dest; > > + - (u8 *) real; > > "real" replaces "dest", which was the first parameter to __copy_instruction. > > > return 0; > > And: > > > int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, > > struct kprobe *__unused) > > { > > - u8 *buf; > > - int ret; > > + u8 *buf = NULL, *slot; > > + int ret, len; > > long rel; > > > > if (!can_optimize((unsigned long)op->kp.addr)) > > return -EILSEQ; > > > > - op->optinsn.insn = get_optinsn_slot(); > > - if (!op->optinsn.insn) > > + buf = kzalloc(MAX_OPTINSN_SIZE, GFP_KERNEL); > > + if (!buf) > > return -ENOMEM; > > > > + op->optinsn.insn = slot = get_optinsn_slot(); > > + if (!slot) { > > + ret = -ENOMEM; > > + goto out; > > + } > > + > > /* > > * Verify if the address gap is in 2GB range, because this uses > > * a relative jump. > > */ > > - rel = (long)op->optinsn.insn - (long)op->kp.addr + RELATIVEJUMP_SIZE; > > + rel = (long)slot - (long)op->kp.addr + RELATIVEJUMP_SIZE; > > if (abs(rel) > 0x7fffffff) { > > - __arch_remove_optimized_kprobe(op, 0); > > - return -ERANGE; > > + ret = -ERANGE; > > + goto err; > > } > > > > - buf = (u8 *)op->optinsn.insn; > > "slot" is equivalent to the old "buf". > > > - set_memory_rw((unsigned long)buf & PAGE_MASK, 1); > > + /* Copy arch-dep-instance from template */ > > + memcpy(buf, &optprobe_template_entry, TMPL_END_IDX); > > > > /* Copy instructions into the out-of-line buffer */ > > - ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr); > > - if (ret < 0) { > > - __arch_remove_optimized_kprobe(op, 0); > > - return ret; > > - } > > + ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr, > > + slot + TMPL_END_IDX); > > We pass in "real" as "slot + TMPL_END_IDX" and "dest" as "buf + > TMPL_END_IDX", thus to make it be equivalent to the code before this > commit, "real" should have "+ len" added to it in order to be > equivalent to what was there before. Right! The broken commit splits trampoline buffer into "temporary" destination buffer and "real" trampoline buffer, and use the "real" address for RIP-relative adjustment. However, I forgot to introduce update the "real" address in the copying loop. > > That said... > > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Thanks! -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly 2018-08-24 7:57 ` Masami Hiramatsu @ 2018-09-03 3:38 ` Masami Hiramatsu 0 siblings, 0 replies; 7+ messages in thread From: Masami Hiramatsu @ 2018-09-03 3:38 UTC (permalink / raw) To: Masami Hiramatsu, Ingo Molnar Cc: Steven Rostedt, Ingo Molnar, Ravi Bangoria, Arnaldo Carvalho de Melo, Michael Rodin, linux-kernel Hi Ingo, Could you pick this fix to urgent branch? Thank you, On Fri, 24 Aug 2018 16:57:19 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Thu, 23 Aug 2018 21:41:09 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Fri, 24 Aug 2018 02:16:12 +0900 > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > Dump of assembler code from 0xffffffffa000207a to 0xffffffffa00020ea: > > > 54 push %rsp > > > ... > > > 48 83 c4 08 add $0x8,%rsp > > > 9d popfq > > > 48 89 f0 mov %rsi,%rax > > > 8b 35 82 7d db e2 mov -0x1d24827e(%rip),%esi > > > # 0xffffffff82db9e67 <nr_cpu_ids+3> > > > > > > As it shows, the 2nd mov accesses *(nr_cpu_ids+3) instead of > > > *nr_cpu_ids. This leads a kernel freeze because cpumask_next() > > > always returns 0 and for_each_cpu() never ended. > > > > Ouch! Nice catch. > > > > > > > > Fixing this by adding len correctly to real RIP address while > > > copying. > > > > > > Fixes: 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()") > > > Reported-by: Michael Rodin <michael@rodin.online> > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > > Cc: stable@vger.kernel.org > > > --- > > > arch/x86/kernel/kprobes/opt.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c > > > index eaf02f2e7300..e92672b8b490 100644 > > > --- a/arch/x86/kernel/kprobes/opt.c > > > +++ b/arch/x86/kernel/kprobes/opt.c > > > @@ -189,7 +189,8 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) > > > int len = 0, ret; > > > > > > while (len < RELATIVEJUMP_SIZE) { > > > - ret = __copy_instruction(dest + len, src + len, real, &insn); > > > + ret = __copy_instruction(dest + len, src + len, real + len, > > > + &insn); > > > if (!ret || !can_boost(&insn, src + len)) > > > return -EINVAL; > > > len += ret; > > > > Looking at the change that broke this we have: > > > > > -static int copy_optimized_instructions(u8 *dest, u8 *src) > > > +static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) > > > { > > > struct insn insn; > > > int len = 0, ret; > > > > > > while (len < RELATIVEJUMP_SIZE) { > > > - ret = __copy_instruction(dest + len, src + len, &insn); > > > + ret = __copy_instruction(dest + len, src + len, real, &insn); > > > > Where "real" was added as a parameter to __copy_instruction. Note that > > we pass in "dest + len" but not "real + len" as you patch fixes. > > __copy_instruction was changed by the bad commit with: > > > > > -int __copy_instruction(u8 *dest, u8 *src, struct insn *insn) > > > +int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn) > > > { > > > kprobe_opcode_t buf[MAX_INSN_SIZE]; > > > unsigned long recovered_insn = > > > @@ -387,11 +388,11 @@ int __copy_instruction(u8 *dest, u8 *src, struct insn *insn) > > > * have given. > > > */ > > > newdisp = (u8 *) src + (s64) insn->displacement.value > > > - - (u8 *) dest; > > > + - (u8 *) real; > > > > "real" replaces "dest", which was the first parameter to __copy_instruction. > > > > > return 0; > > > > And: > > > > > int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, > > > struct kprobe *__unused) > > > { > > > - u8 *buf; > > > - int ret; > > > + u8 *buf = NULL, *slot; > > > + int ret, len; > > > long rel; > > > > > > if (!can_optimize((unsigned long)op->kp.addr)) > > > return -EILSEQ; > > > > > > - op->optinsn.insn = get_optinsn_slot(); > > > - if (!op->optinsn.insn) > > > + buf = kzalloc(MAX_OPTINSN_SIZE, GFP_KERNEL); > > > + if (!buf) > > > return -ENOMEM; > > > > > > + op->optinsn.insn = slot = get_optinsn_slot(); > > > + if (!slot) { > > > + ret = -ENOMEM; > > > + goto out; > > > + } > > > + > > > /* > > > * Verify if the address gap is in 2GB range, because this uses > > > * a relative jump. > > > */ > > > - rel = (long)op->optinsn.insn - (long)op->kp.addr + RELATIVEJUMP_SIZE; > > > + rel = (long)slot - (long)op->kp.addr + RELATIVEJUMP_SIZE; > > > if (abs(rel) > 0x7fffffff) { > > > - __arch_remove_optimized_kprobe(op, 0); > > > - return -ERANGE; > > > + ret = -ERANGE; > > > + goto err; > > > } > > > > > > - buf = (u8 *)op->optinsn.insn; > > > > "slot" is equivalent to the old "buf". > > > > > - set_memory_rw((unsigned long)buf & PAGE_MASK, 1); > > > + /* Copy arch-dep-instance from template */ > > > + memcpy(buf, &optprobe_template_entry, TMPL_END_IDX); > > > > > > /* Copy instructions into the out-of-line buffer */ > > > - ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr); > > > - if (ret < 0) { > > > - __arch_remove_optimized_kprobe(op, 0); > > > - return ret; > > > - } > > > + ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr, > > > + slot + TMPL_END_IDX); > > > > We pass in "real" as "slot + TMPL_END_IDX" and "dest" as "buf + > > TMPL_END_IDX", thus to make it be equivalent to the code before this > > commit, "real" should have "+ len" added to it in order to be > > equivalent to what was there before. > > Right! The broken commit splits trampoline buffer into > "temporary" destination buffer and "real" trampoline buffer, > and use the "real" address for RIP-relative adjustment. > However, I forgot to introduce update the "real" address > in the copying loop. > > > > > That said... > > > > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > Thanks! > > > -- > Masami Hiramatsu <mhiramat@kernel.org> -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly 2018-08-23 17:16 [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly Masami Hiramatsu 2018-08-24 1:41 ` Steven Rostedt @ 2018-12-04 8:13 ` Ingo Molnar 2018-12-04 13:49 ` Masami Hiramatsu 2018-12-04 23:05 ` [tip:perf/urgent] kprobes/x86: Fix instruction patching corruption when copying more than one RIP-relative instruction tip-bot for Masami Hiramatsu 2 siblings, 1 reply; 7+ messages in thread From: Ingo Molnar @ 2018-12-04 8:13 UTC (permalink / raw) To: Masami Hiramatsu Cc: Ingo Molnar, Steven Rostedt, Ravi Bangoria, Arnaldo Carvalho de Melo, Michael Rodin, linux-kernel * Masami Hiramatsu <mhiramat@kernel.org> wrote: > Since copy_optimized_instructions() misses to update real RIP > address while copying several instructions to working buffer, > it adjusts RIP-relative instruction with wrong RIP address for > the 2nd and subsequent instructions. > > This may break the kernel (like kernel freeze) because > probed instruction can refer a wrong data. For example, > putting kprobes on cpumask_next hit this bug. > > cpumask_next is normally like below if CONFIG_CPUMASK_OFFSTACK=y > (in this case nr_cpumask_bits is an alias of nr_cpu_ids) > > <cpumask_next>: > 48 89 f0 mov %rsi,%rax > 8b 35 7b fb e2 00 mov 0xe2fb7b(%rip),%esi > # ffffffff82db9e64 <nr_cpu_ids> > 55 push %rbp > ... > > If we put a kprobe on it and optimized with jump, it becomes > like this. > > e9 95 7d 07 1e jmpq 0xffffffffa000207a > 7b fb jnp 0xffffffff81f8a2e2 <cpumask_next+2> > e2 00 loop 0xffffffff81f8a2e9 <cpumask_next+9> > 55 push %rbp > > This shows first 2 "mov" instructions are copied to trampoline > buffer at 0xffffffffa000207a. Here is the disassembled result. > (skipped optprobe template instructions) > > Dump of assembler code from 0xffffffffa000207a to 0xffffffffa00020ea: > 54 push %rsp > ... > 48 83 c4 08 add $0x8,%rsp > 9d popfq > 48 89 f0 mov %rsi,%rax > 8b 35 82 7d db e2 mov -0x1d24827e(%rip),%esi > # 0xffffffff82db9e67 <nr_cpu_ids+3> > > As it shows, the 2nd mov accesses *(nr_cpu_ids+3) instead of > *nr_cpu_ids. This leads a kernel freeze because cpumask_next() > always returns 0 and for_each_cpu() never ended. > > Fixing this by adding len correctly to real RIP address while > copying. > > Fixes: 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()") > Reported-by: Michael Rodin <michael@rodin.online> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Cc: stable@vger.kernel.org > --- > arch/x86/kernel/kprobes/opt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c > index eaf02f2e7300..e92672b8b490 100644 > --- a/arch/x86/kernel/kprobes/opt.c > +++ b/arch/x86/kernel/kprobes/opt.c > @@ -189,7 +189,8 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) > int len = 0, ret; > > while (len < RELATIVEJUMP_SIZE) { > - ret = __copy_instruction(dest + len, src + len, real, &insn); > + ret = __copy_instruction(dest + len, src + len, real + len, > + &insn); I applied this, except that I unbroke the line: please ignore checkpatch in such cases where the cure is worse than the disease ... I.e. even if slightly over 80 cols, this is more readable: ret = __copy_instruction(dest + len, src + len, real + len, &insn); Thanks, Ingo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly 2018-12-04 8:13 ` Ingo Molnar @ 2018-12-04 13:49 ` Masami Hiramatsu 0 siblings, 0 replies; 7+ messages in thread From: Masami Hiramatsu @ 2018-12-04 13:49 UTC (permalink / raw) To: Ingo Molnar Cc: Ingo Molnar, Steven Rostedt, Ravi Bangoria, Arnaldo Carvalho de Melo, Michael Rodin, linux-kernel On Tue, 4 Dec 2018 09:13:35 +0100 Ingo Molnar <mingo@kernel.org> wrote: > > * Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > Since copy_optimized_instructions() misses to update real RIP > > address while copying several instructions to working buffer, > > it adjusts RIP-relative instruction with wrong RIP address for > > the 2nd and subsequent instructions. > > > > This may break the kernel (like kernel freeze) because > > probed instruction can refer a wrong data. For example, > > putting kprobes on cpumask_next hit this bug. > > > > cpumask_next is normally like below if CONFIG_CPUMASK_OFFSTACK=y > > (in this case nr_cpumask_bits is an alias of nr_cpu_ids) > > > > <cpumask_next>: > > 48 89 f0 mov %rsi,%rax > > 8b 35 7b fb e2 00 mov 0xe2fb7b(%rip),%esi > > # ffffffff82db9e64 <nr_cpu_ids> > > 55 push %rbp > > ... > > > > If we put a kprobe on it and optimized with jump, it becomes > > like this. > > > > e9 95 7d 07 1e jmpq 0xffffffffa000207a > > 7b fb jnp 0xffffffff81f8a2e2 <cpumask_next+2> > > e2 00 loop 0xffffffff81f8a2e9 <cpumask_next+9> > > 55 push %rbp > > > > This shows first 2 "mov" instructions are copied to trampoline > > buffer at 0xffffffffa000207a. Here is the disassembled result. > > (skipped optprobe template instructions) > > > > Dump of assembler code from 0xffffffffa000207a to 0xffffffffa00020ea: > > 54 push %rsp > > ... > > 48 83 c4 08 add $0x8,%rsp > > 9d popfq > > 48 89 f0 mov %rsi,%rax > > 8b 35 82 7d db e2 mov -0x1d24827e(%rip),%esi > > # 0xffffffff82db9e67 <nr_cpu_ids+3> > > > > As it shows, the 2nd mov accesses *(nr_cpu_ids+3) instead of > > *nr_cpu_ids. This leads a kernel freeze because cpumask_next() > > always returns 0 and for_each_cpu() never ended. > > > > Fixing this by adding len correctly to real RIP address while > > copying. > > > > Fixes: 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()") > > Reported-by: Michael Rodin <michael@rodin.online> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: stable@vger.kernel.org > > --- > > arch/x86/kernel/kprobes/opt.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c > > index eaf02f2e7300..e92672b8b490 100644 > > --- a/arch/x86/kernel/kprobes/opt.c > > +++ b/arch/x86/kernel/kprobes/opt.c > > @@ -189,7 +189,8 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) > > int len = 0, ret; > > > > while (len < RELATIVEJUMP_SIZE) { > > - ret = __copy_instruction(dest + len, src + len, real, &insn); > > + ret = __copy_instruction(dest + len, src + len, real + len, > > + &insn); > > I applied this, except that I unbroke the line: please ignore checkpatch > in such cases where the cure is worse than the disease ... Thanks Ingo! > > I.e. even if slightly over 80 cols, this is more readable: > > ret = __copy_instruction(dest + len, src + len, real + len, &insn); Got it. BTW, this ugry "+ len" repeat would be avoided. I would better make a new inline function to wrap it. Thank you, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip:perf/urgent] kprobes/x86: Fix instruction patching corruption when copying more than one RIP-relative instruction 2018-08-23 17:16 [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly Masami Hiramatsu 2018-08-24 1:41 ` Steven Rostedt 2018-12-04 8:13 ` Ingo Molnar @ 2018-12-04 23:05 ` tip-bot for Masami Hiramatsu 2 siblings, 0 replies; 7+ messages in thread From: tip-bot for Masami Hiramatsu @ 2018-12-04 23:05 UTC (permalink / raw) To: linux-tip-commits Cc: torvalds, tglx, ravi.bangoria, linux-kernel, rostedt, acme, mhiramat, hpa, peterz, michael, mingo Commit-ID: 43a1b0cb4cd6dbfd3cd9c10da663368394d299d8 Gitweb: https://git.kernel.org/tip/43a1b0cb4cd6dbfd3cd9c10da663368394d299d8 Author: Masami Hiramatsu <mhiramat@kernel.org> AuthorDate: Fri, 24 Aug 2018 02:16:12 +0900 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Tue, 4 Dec 2018 09:35:20 +0100 kprobes/x86: Fix instruction patching corruption when copying more than one RIP-relative instruction After copy_optimized_instructions() copies several instructions to the working buffer it tries to fix up the real RIP address, but it adjusts the RIP-relative instruction with an incorrect RIP address for the 2nd and subsequent instructions due to a bug in the logic. This will break the kernel pretty badly (with likely outcomes such as a kernel freeze, a crash, or worse) because probed instructions can refer to the wrong data. For example putting kprobes on cpumask_next() typically hits this bug. cpumask_next() is normally like below if CONFIG_CPUMASK_OFFSTACK=y (in this case nr_cpumask_bits is an alias of nr_cpu_ids): <cpumask_next>: 48 89 f0 mov %rsi,%rax 8b 35 7b fb e2 00 mov 0xe2fb7b(%rip),%esi # ffffffff82db9e64 <nr_cpu_ids> 55 push %rbp ... If we put a kprobe on it and it gets jump-optimized, it gets patched by the kprobes code like this: <cpumask_next>: e9 95 7d 07 1e jmpq 0xffffffffa000207a 7b fb jnp 0xffffffff81f8a2e2 <cpumask_next+2> e2 00 loop 0xffffffff81f8a2e9 <cpumask_next+9> 55 push %rbp This shows that the first two MOV instructions were copied to a trampoline buffer at 0xffffffffa000207a. Here is the disassembled result of the trampoline, skipping the optprobe template instructions: # Dump of assembly code from 0xffffffffa000207a to 0xffffffffa00020ea: 54 push %rsp ... 48 83 c4 08 add $0x8,%rsp 9d popfq 48 89 f0 mov %rsi,%rax 8b 35 82 7d db e2 mov -0x1d24827e(%rip),%esi # 0xffffffff82db9e67 <nr_cpu_ids+3> This dump shows that the second MOV accesses *(nr_cpu_ids+3) instead of the original *nr_cpu_ids. This leads to a kernel freeze because cpumask_next() always returns 0 and for_each_cpu() never ends. Fix this by adding 'len' correctly to the real RIP address while copying. [ mingo: Improved the changelog. ] Reported-by: Michael Rodin <michael@rodin.online> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: stable@vger.kernel.org # v4.15+ Fixes: 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()") Link: http://lkml.kernel.org/r/153504457253.22602.1314289671019919596.stgit@devbox Signed-off-by: Ingo Molnar <mingo@kernel.org> --- arch/x86/kernel/kprobes/opt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c index 40b16b270656..6adf6e6c2933 100644 --- a/arch/x86/kernel/kprobes/opt.c +++ b/arch/x86/kernel/kprobes/opt.c @@ -189,7 +189,7 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real) int len = 0, ret; while (len < RELATIVEJUMP_SIZE) { - ret = __copy_instruction(dest + len, src + len, real, &insn); + ret = __copy_instruction(dest + len, src + len, real + len, &insn); if (!ret || !can_boost(&insn, src + len)) return -EINVAL; len += ret; ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-04 23:06 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-23 17:16 [BUGFIX PATCH -tip] kprobes/x86: Fix to copy RIP relative instruction correctly Masami Hiramatsu 2018-08-24 1:41 ` Steven Rostedt 2018-08-24 7:57 ` Masami Hiramatsu 2018-09-03 3:38 ` Masami Hiramatsu 2018-12-04 8:13 ` Ingo Molnar 2018-12-04 13:49 ` Masami Hiramatsu 2018-12-04 23:05 ` [tip:perf/urgent] kprobes/x86: Fix instruction patching corruption when copying more than one RIP-relative instruction tip-bot for Masami Hiramatsu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.