Steven Rostedt wrote:
> On Fri, 8 May 2009, 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 <mhiramat@xxxxxxxxxx>
>> Cc: Ananth N Mavinakayanahalli <ananth@xxxxxxxxxx>
>> Cc: Jim Keniston <jkenisto@xxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxx>
>> ---
>>
>> arch/x86/kernel/kprobes.c | 54 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 54 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
>> index 7b5169d..3d5e85f 100644
>> --- a/arch/x86/kernel/kprobes.c
>> +++ b/arch/x86/kernel/kprobes.c
>> @@ -48,12 +48,14 @@
>> #include <linux/preempt.h>
>> #include <linux/module.h>
>> #include <linux/kdebug.h>
>> +#include <linux/kallsyms.h>
>>
>> #include <asm/cacheflush.h>
>> #include <asm/desc.h>
>> #include <asm/pgtable.h>
>> #include <asm/uaccess.h>
>> #include <asm/alternative.h>
>> +#include <asm/insn.h>
>>
>> void jprobe_return_end(void);
>>
>> @@ -244,6 +246,56 @@ 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;
>> +
>
> I'm just doing a casual scan of the patch set.
Thank you!
>
>> + /*
>> + * Don't use p->ainsn.insn, which could be modified -- e.g.,
>
> This comment talks about "p", what's that? It's not used in this function.
oops, this should be kp.
>
>> + * by fix_riprel().
>> + */
>> + memcpy(buf, kp->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
>> + buf[0] = kp->opcode;
>
> Why is it OK to copy addr to "buf" and then rewrite the first character of
> buf? Does it have something to do with the above "p"?
Yes, each kprobe copied probed instruction to kp->ainsn.insn,
which is an executable buffer for single stepping.
So, 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.
> I don't mean to be critical here, but I've been doing "Mother Day"
> activities all weekend and for some reason that was also the best time for
> everyone to Cc me on patches. I'm way behind in my email, and it would be
> nice if the comments described why things that "look" wrong are not.
>
>
>> + 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];
>> +
>> + /* Lookup symbol including addr */
>
> The above comment is very close to a "add one to i" for i++ type of
> comment.
Agreed.
>
>> + if (!kallsyms_lookup(paddr, NULL, &offset, NULL, __dummy_buf))
>> + return 0;
>> +
>> + /* Decode instructions */
>> + addr = paddr - offset;
>> + while (addr < paddr) {
>> + insn_init_kernel(&insn, (void *)addr);
>> + insn_get_opcode(&insn);
>> + if (OPCODE1(&insn) == BREAKPOINT_INSTRUCTION) {
>> + ret = recover_probed_instruction(buf, addr);
>
> Oh, the above puts back the original op code. That is why it is OK?
Oops, no. I have to use get_kprobe() instead. Thanks!
>
> I'd comment that a little bit more. Just so that reviewers have an easier
> idea of what is happening.
>
>> + if (ret)
>> + return 0;
>> + insn_init_kernel(&insn, buf);
>
> insn_init_kernel? Is that like a text poke or something?
it's a wrapper of insn_init() which initialize struct insn.
Thank you,
>> + }
>> + insn_get_length(&insn);
>> + addr += insn.length;
>> + }
>> +
>> + return (addr == paddr);
>> +}
>> +
>> /*
>> * Returns non-zero if opcode modifies the interrupt flag.
>> */
>> @@ -359,6 +411,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();
>
> Oh look, I found the phantom "p"!
>
> -- Steve
>
>> if (!p->ainsn.insn)
>>
>>
>> --
>> Masami Hiramatsu
>>
>> Software Engineer
>> Hitachi Computer Products (America) Inc.
>> Software Solutions Division
>>
>> e-mail: mhiramat@xxxxxxxxxx
>>
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: mhiramat@xxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
