Re: [PATCH -tip v14 03/12] kprobes: checks probe address is instruction boudary on x86

2009-08-18 Thread Frederic Weisbecker
On Thu, Aug 13, 2009 at 04:34:28PM -0400, Masami Hiramatsu wrote:
 Ensure safeness of inserting kprobes by checking whether the specified
 address is at the first byte of a instruction on x86.
 This is done by decoding probed function from its head to the probe point.
 
 Signed-off-by: Masami Hiramatsu mhira...@redhat.com
 Acked-by: Ananth N Mavinakayanahalli ana...@in.ibm.com
 Cc: Avi Kivity a...@redhat.com
 Cc: Andi Kleen a...@linux.intel.com
 Cc: Christoph Hellwig h...@infradead.org
 Cc: Frank Ch. Eigler f...@redhat.com
 Cc: Frederic Weisbecker fweis...@gmail.com
 Cc: H. Peter Anvin h...@zytor.com
 Cc: Ingo Molnar mi...@elte.hu
 Cc: Jason Baron jba...@redhat.com
 Cc: Jim Keniston jkeni...@us.ibm.com
 Cc: K.Prasad pra...@linux.vnet.ibm.com
 Cc: Lai Jiangshan la...@cn.fujitsu.com
 Cc: Li Zefan l...@cn.fujitsu.com
 Cc: Przemysław Pawełczyk przemys...@pawelczyk.it
 Cc: Roland McGrath rol...@redhat.com
 Cc: Sam Ravnborg s...@ravnborg.org
 Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com
 Cc: Steven Rostedt rost...@goodmis.org
 Cc: Tom Zanussi tzanu...@gmail.com
 Cc: Vegard Nossum vegard.nos...@gmail.com
 ---
 
  arch/x86/kernel/kprobes.c |   69 
 +
  1 files changed, 69 insertions(+), 0 deletions(-)
 
 diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
 index b5b1848..80d493f 100644
 --- a/arch/x86/kernel/kprobes.c
 +++ b/arch/x86/kernel/kprobes.c
 @@ -48,6 +48,7 @@
  #include linux/preempt.h
  #include linux/module.h
  #include linux/kdebug.h
 +#include linux/kallsyms.h
  
  #include asm/cacheflush.h
  #include asm/desc.h
 @@ -55,6 +56,7 @@
  #include asm/uaccess.h
  #include asm/alternative.h
  #include asm/debugreg.h
 +#include asm/insn.h
  
  void jprobe_return_end(void);
  
 @@ -245,6 +247,71 @@ retry:
   }
  }
  
 +/* Recover the probed instruction at addr for further analysis. */
 +static int recover_probed_instruction(kprobe_opcode_t *buf, unsigned long 
 addr)
 +{
 + struct kprobe *kp;
 + kp = get_kprobe((void *)addr);
 + if (!kp)
 + return -EINVAL;
 +
 + /*
 +  *  Basically, kp-ainsn.insn has an original instruction.
 +  *  However, RIP-relative instruction can not do single-stepping
 +  *  at different place, fix_riprel() tweaks the displacement of
 +  *  that instruction. In that case, we can't recover the instruction
 +  *  from the kp-ainsn.insn.
 +  *
 +  *  On the other hand, kp-opcode has a copy of the first byte of
 +  *  the probed instruction, which is overwritten by int3. And
 +  *  the instruction at kp-addr is not modified by kprobes except
 +  *  for the first byte, we can recover the original instruction
 +  *  from it and kp-opcode.
 +  */
 + memcpy(buf, kp-addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
 + buf[0] = kp-opcode;
 + return 0;
 +}
 +
 +/* Dummy buffers for kallsyms_lookup */
 +static char __dummy_buf[KSYM_NAME_LEN];
 +
 +/* Check if paddr is at an instruction boundary */
 +static int __kprobes can_probe(unsigned long paddr)
 +{
 + int ret;
 + unsigned long addr, offset = 0;
 + struct insn insn;
 + kprobe_opcode_t buf[MAX_INSN_SIZE];
 +
 + if (!kallsyms_lookup(paddr, NULL, offset, NULL, __dummy_buf))
 + return 0;
 +
 + /* Decode instructions */
 + addr = paddr - offset;
 + while (addr  paddr) {
 + kernel_insn_init(insn, (void *)addr);
 + insn_get_opcode(insn);
 +
 + /* Check if the instruction has been modified. */
 + if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION) {
 + ret = recover_probed_instruction(buf, addr);



I'm confused about the reason of this recovering. Is it to remove
kprobes behind the current setting one in the current function?

If such cleanup is needed for whatever reason, I wonder what happens
to the corresponding kprobe structure, why isn't it using the arch_disarm_
helper to patch back?

(Questions that may prove my solid misunderstanding of the kprobes code ;-)

Frederic.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH -tip v14 03/12] kprobes: checks probe address is instruction boudary on x86

2009-08-18 Thread Masami Hiramatsu
Frederic Weisbecker wrote:
 On Thu, Aug 13, 2009 at 04:34:28PM -0400, Masami Hiramatsu wrote:
 Ensure safeness of inserting kprobes by checking whether the specified
 address is at the first byte of a instruction on x86.
 This is done by decoding probed function from its head to the probe point.

 Signed-off-by: Masami Hiramatsu mhira...@redhat.com
 Acked-by: Ananth N Mavinakayanahalli ana...@in.ibm.com
 Cc: Avi Kivity a...@redhat.com
 Cc: Andi Kleen a...@linux.intel.com
 Cc: Christoph Hellwig h...@infradead.org
 Cc: Frank Ch. Eigler f...@redhat.com
 Cc: Frederic Weisbecker fweis...@gmail.com
 Cc: H. Peter Anvin h...@zytor.com
 Cc: Ingo Molnar mi...@elte.hu
 Cc: Jason Baron jba...@redhat.com
 Cc: Jim Keniston jkeni...@us.ibm.com
 Cc: K.Prasad pra...@linux.vnet.ibm.com
 Cc: Lai Jiangshan la...@cn.fujitsu.com
 Cc: Li Zefan l...@cn.fujitsu.com
 Cc: Przemysław Pawełczyk przemys...@pawelczyk.it
 Cc: Roland McGrath rol...@redhat.com
 Cc: Sam Ravnborg s...@ravnborg.org
 Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com
 Cc: Steven Rostedt rost...@goodmis.org
 Cc: Tom Zanussi tzanu...@gmail.com
 Cc: Vegard Nossum vegard.nos...@gmail.com
 ---

  arch/x86/kernel/kprobes.c |   69 
 +
  1 files changed, 69 insertions(+), 0 deletions(-)

 diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
 index b5b1848..80d493f 100644
 --- a/arch/x86/kernel/kprobes.c
 +++ b/arch/x86/kernel/kprobes.c
 @@ -48,6 +48,7 @@
  #include linux/preempt.h
  #include linux/module.h
  #include linux/kdebug.h
 +#include linux/kallsyms.h
  
  #include asm/cacheflush.h
  #include asm/desc.h
 @@ -55,6 +56,7 @@
  #include asm/uaccess.h
  #include asm/alternative.h
  #include asm/debugreg.h
 +#include asm/insn.h
  
  void jprobe_return_end(void);
  
 @@ -245,6 +247,71 @@ retry:
  }
  }
  
 +/* Recover the probed instruction at addr for further analysis. */
 +static int recover_probed_instruction(kprobe_opcode_t *buf, unsigned long 
 addr)
 +{
 +struct kprobe *kp;
 +kp = get_kprobe((void *)addr);
 +if (!kp)
 +return -EINVAL;
 +
 +/*
 + *  Basically, kp-ainsn.insn has an original instruction.
 + *  However, RIP-relative instruction can not do single-stepping
 + *  at different place, fix_riprel() tweaks the displacement of
 + *  that instruction. In that case, we can't recover the instruction
 + *  from the kp-ainsn.insn.
 + *
 + *  On the other hand, kp-opcode has a copy of the first byte of
 + *  the probed instruction, which is overwritten by int3. And
 + *  the instruction at kp-addr is not modified by kprobes except
 + *  for the first byte, we can recover the original instruction
 + *  from it and kp-opcode.
 + */
 +memcpy(buf, kp-addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
 +buf[0] = kp-opcode;
 +return 0;
 +}
 +
 +/* Dummy buffers for kallsyms_lookup */
 +static char __dummy_buf[KSYM_NAME_LEN];
 +
 +/* Check if paddr is at an instruction boundary */
 +static int __kprobes can_probe(unsigned long paddr)
 +{
 +int ret;
 +unsigned long addr, offset = 0;
 +struct insn insn;
 +kprobe_opcode_t buf[MAX_INSN_SIZE];
 +
 +if (!kallsyms_lookup(paddr, NULL, offset, NULL, __dummy_buf))
 +return 0;
 +
 +/* Decode instructions */
 +addr = paddr - offset;
 +while (addr  paddr) {
 +kernel_insn_init(insn, (void *)addr);
 +insn_get_opcode(insn);
 +
 +/* Check if the instruction has been modified. */
 +if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION) {
 +ret = recover_probed_instruction(buf, addr);
 
 
 
 I'm confused about the reason of this recovering. Is it to remove
 kprobes behind the current setting one in the current function?

No, it recovers just an instruction which is probed by a kprobe,
because we need to know the first byte of this instruction for
decoding it.

Perhaps we'd better to have more generic interface (text_peek?)
for it because another subsystem (e.g. kgdb) may want to insert int3...

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhira...@redhat.com

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH -tip v14 03/12] kprobes: checks probe address is instruction boudary on x86

2009-08-18 Thread Frederic Weisbecker
On Tue, Aug 18, 2009 at 07:17:39PM -0400, Masami Hiramatsu wrote:
 Frederic Weisbecker wrote:
  +  while (addr  paddr) {
  +  kernel_insn_init(insn, (void *)addr);
  +  insn_get_opcode(insn);
  +
  +  /* Check if the instruction has been modified. */
  +  if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION) {
  +  ret = recover_probed_instruction(buf, addr);
  
  
  
  I'm confused about the reason of this recovering. Is it to remove
  kprobes behind the current setting one in the current function?
 
 No, it recovers just an instruction which is probed by a kprobe,
 because we need to know the first byte of this instruction for
 decoding it.
 
 Perhaps we'd better to have more generic interface (text_peek?)
 for it because another subsystem (e.g. kgdb) may want to insert int3...
 
 Thank you,


Aah, I see now, it's to keep a sane check of the instructions
boundaries without int 3 artifacts in the middle.

But in that case, you should re-arm the breakpoint after your
check, right?

Or may be you could do the check without repatching?
May be by doing a copy of insn.opcode.bytes and replacing bytes[0]
with what a random kprobe has stolen?

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH -tip v14 03/12] kprobes: checks probe address is instruction boudary on x86

2009-08-18 Thread Masami Hiramatsu
Frederic Weisbecker wrote:
 On Tue, Aug 18, 2009 at 07:17:39PM -0400, Masami Hiramatsu wrote:
 Frederic Weisbecker wrote:
 +  while (addr  paddr) {
 +  kernel_insn_init(insn, (void *)addr);
 +  insn_get_opcode(insn);
 +
 +  /* Check if the instruction has been modified. */
 +  if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION) {
 +  ret = recover_probed_instruction(buf, addr);



 I'm confused about the reason of this recovering. Is it to remove
 kprobes behind the current setting one in the current function?

 No, it recovers just an instruction which is probed by a kprobe,
 because we need to know the first byte of this instruction for
 decoding it.

Ah, sorry, it was not accurate. the function recovers an instruction
on the buffer(buf), not on the real kernel text. :)


 Perhaps we'd better to have more generic interface (text_peek?)
 for it because another subsystem (e.g. kgdb) may want to insert int3...

 Thank you,
 
 
 Aah, I see now, it's to keep a sane check of the instructions
 boundaries without int 3 artifacts in the middle.
 
 But in that case, you should re-arm the breakpoint after your
 check, right?
 
 Or may be you could do the check without repatching?

Yes, it doesn't modify kernel text, just recover an original
instruction from kernel text and backup byte on a buffer.

 May be by doing a copy of insn.opcode.bytes and replacing bytes[0]
 with what a random kprobe has stolen?

Hm, no, this function is protected from other kprobes by kprobe_mutex.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhira...@redhat.com

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH -tip v14 03/12] kprobes: checks probe address is instruction boudary on x86

2009-08-13 Thread Masami Hiramatsu
Ensure safeness of inserting kprobes by checking whether the specified
address is at the first byte of a instruction on x86.
This is done by decoding probed function from its head to the probe point.

Signed-off-by: Masami Hiramatsu mhira...@redhat.com
Acked-by: Ananth N Mavinakayanahalli ana...@in.ibm.com
Cc: Avi Kivity a...@redhat.com
Cc: Andi Kleen a...@linux.intel.com
Cc: Christoph Hellwig h...@infradead.org
Cc: Frank Ch. Eigler f...@redhat.com
Cc: Frederic Weisbecker fweis...@gmail.com
Cc: H. Peter Anvin h...@zytor.com
Cc: Ingo Molnar mi...@elte.hu
Cc: Jason Baron jba...@redhat.com
Cc: Jim Keniston jkeni...@us.ibm.com
Cc: K.Prasad pra...@linux.vnet.ibm.com
Cc: Lai Jiangshan la...@cn.fujitsu.com
Cc: Li Zefan l...@cn.fujitsu.com
Cc: Przemysław Pawełczyk przemys...@pawelczyk.it
Cc: Roland McGrath rol...@redhat.com
Cc: Sam Ravnborg s...@ravnborg.org
Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com
Cc: Steven Rostedt rost...@goodmis.org
Cc: Tom Zanussi tzanu...@gmail.com
Cc: Vegard Nossum vegard.nos...@gmail.com
---

 arch/x86/kernel/kprobes.c |   69 +
 1 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index b5b1848..80d493f 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -48,6 +48,7 @@
 #include linux/preempt.h
 #include linux/module.h
 #include linux/kdebug.h
+#include linux/kallsyms.h
 
 #include asm/cacheflush.h
 #include asm/desc.h
@@ -55,6 +56,7 @@
 #include asm/uaccess.h
 #include asm/alternative.h
 #include asm/debugreg.h
+#include asm/insn.h
 
 void jprobe_return_end(void);
 
@@ -245,6 +247,71 @@ retry:
}
 }
 
+/* Recover the probed instruction at addr for further analysis. */
+static int recover_probed_instruction(kprobe_opcode_t *buf, unsigned long addr)
+{
+   struct kprobe *kp;
+   kp = get_kprobe((void *)addr);
+   if (!kp)
+   return -EINVAL;
+
+   /*
+*  Basically, kp-ainsn.insn has an original instruction.
+*  However, RIP-relative instruction can not do single-stepping
+*  at different place, fix_riprel() tweaks the displacement of
+*  that instruction. In that case, we can't recover the instruction
+*  from the kp-ainsn.insn.
+*
+*  On the other hand, kp-opcode has a copy of the first byte of
+*  the probed instruction, which is overwritten by int3. And
+*  the instruction at kp-addr is not modified by kprobes except
+*  for the first byte, we can recover the original instruction
+*  from it and kp-opcode.
+*/
+   memcpy(buf, kp-addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+   buf[0] = kp-opcode;
+   return 0;
+}
+
+/* Dummy buffers for kallsyms_lookup */
+static char __dummy_buf[KSYM_NAME_LEN];
+
+/* Check if paddr is at an instruction boundary */
+static int __kprobes can_probe(unsigned long paddr)
+{
+   int ret;
+   unsigned long addr, offset = 0;
+   struct insn insn;
+   kprobe_opcode_t buf[MAX_INSN_SIZE];
+
+   if (!kallsyms_lookup(paddr, NULL, offset, NULL, __dummy_buf))
+   return 0;
+
+   /* Decode instructions */
+   addr = paddr - offset;
+   while (addr  paddr) {
+   kernel_insn_init(insn, (void *)addr);
+   insn_get_opcode(insn);
+
+   /* Check if the instruction has been modified. */
+   if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION) {
+   ret = recover_probed_instruction(buf, addr);
+   if (ret)
+   /*
+* Another debugging subsystem might insert
+* this breakpoint. In that case, we can't
+* recover it.
+*/
+   return 0;
+   kernel_insn_init(insn, buf);
+   }
+   insn_get_length(insn);
+   addr += insn.length;
+   }
+
+   return (addr == paddr);
+}
+
 /*
  * Returns non-zero if opcode modifies the interrupt flag.
  */
@@ -360,6 +427,8 @@ static void __kprobes arch_copy_kprobe(struct kprobe *p)
 
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
+   if (!can_probe((unsigned long)p-addr))
+   return -EILSEQ;
/* insn: must be on special executable page on x86. */
p-ainsn.insn = get_insn_slot();
if (!p-ainsn.insn)


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhira...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html