| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <477BB516.8040507@redhat.com>
Date: Wed, 02 Jan 2008 11:00:22 -0500
From: Masami Hiramatsu <mhiramat@...hat.com>
To: Abhishek Sagar <sagar.abhishek@...il.com>, ananth@...ibm.com,
jkenisto@...ibm.com
CC: Harvey Harrison <harvey.harrison@...il.com>,
Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
LKML <linux-kernel@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>, qbarnes@...il.com
Subject: Re: [PATCH] x86: kprobes change kprobe_handler flow
Hi,
Abhishek Sagar wrote:
>>> static int __kprobes kprobe_handler(struct pt_regs *regs)
>>> {
>>> - struct kprobe *p;
>>> int ret = 0;
>>> kprobe_opcode_t *addr;
>>> + struct kprobe *p, *cur;
>>> struct kprobe_ctlblk *kcb;
>>>
>>> addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));
>>> + if (*addr != BREAKPOINT_INSTRUCTION) {
>>> + /*
>>> + * The breakpoint instruction was removed right
>>> + * after we hit it. Another cpu has removed
>>> + * either a probepoint or a debugger breakpoint
>>> + * at this address. In either case, no further
>>> + * handling of this interrupt is appropriate.
>>> + * Back up over the (now missing) int3 and run
>>> + * the original instruction.
>>> + */
>>> + regs->eip -= sizeof(kprobe_opcode_t);
>>> + return 1;
>>> + }
>
> I have moved the above breakpoint removal check at the beginning of
> the function. The current check is for '!p' case only, whereas IMO
> this check should be performed for all cases. An external debugger may
> very well plant and remove a breakpoint on an address which has probe
> on it. This check is a race nevertheless, so its relative placing
> within kprobe_handler() would be best where its duplication can be
> avoided.
I think there is another reason; now we have disable_all_kprobes() which
disarms(removes BREAKPOINT instruction) all kprobes. So, this should be commented.
By the way, IMHO, if a debugger causes it, that might be a bug.
Since when the debugger removes a breakpoint, it should prevent the exception
caused by the breakpoint on other processors. In other words, the debugger
should work as transparently as possible.
>>> - /* Check we're not actually recursing */
>>> - if (kprobe_running()) {
>>> - p = get_kprobe(addr);
>>> - if (p) {
>>> - if (kcb->kprobe_status == KPROBE_HIT_SS &&
>>> - *p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
>>> - regs->eflags &= ~TF_MASK;
>>> - regs->eflags |= kcb->kprobe_saved_eflags;
>>> - goto no_kprobe;
>>> - }
>>> - /* We have reentered the kprobe_handler(), since
>>> - * another probe was hit while within the handler.
>>> - * We here save the original kprobes variables and
>>> - * just single step on the instruction of the new probe
>>> - * without calling any user handlers.
>>> - */
>>> - save_previous_kprobe(kcb);
>>> - set_current_kprobe(p, regs, kcb);
>>> - kprobes_inc_nmissed_count(p);
>>> - prepare_singlestep(p, regs);
>>> - kcb->kprobe_status = KPROBE_REENTER;
>>> - return 1;
>>> - } else {
>>> - if (*addr != BREAKPOINT_INSTRUCTION) {
>>> - /* The breakpoint instruction was removed by
>>> - * another cpu right after we hit, no further
>>> - * handling of this interrupt is appropriate
>>> - */
>>> - regs->eip -= sizeof(kprobe_opcode_t);
>>> + if (p) {
>>> + if (cur) {
>>> + switch (kcb->kprobe_status) {
>>> + case KPROBE_HIT_ACTIVE:
>>> + case KPROBE_HIT_SSDONE:
>>> + /* a probe has been hit inside a
>>> + * user handler */
>>> + save_previous_kprobe(kcb);
>>> + set_current_kprobe(p, regs, kcb);
>>> + kprobes_inc_nmissed_count(p);
>>> + prepare_singlestep(p, regs);
>>> + kcb->kprobe_status = KPROBE_REENTER;
>>> ret = 1;
>>> - goto no_kprobe;
>>> - }
>>> - p = __get_cpu_var(current_kprobe);
>>> - if (p->break_handler && p->break_handler(p, regs)) {
>>> - goto ss_probe;
>>> + break;
>>> + case KPROBE_HIT_SS:
>>> + if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
>>> + regs->eflags &= ~TF_MASK;
>>> + regs->eflags |=
>>> + kcb->kprobe_saved_eflags;
>>> + } else {
>>> + /* BUG? */
>>> + }
I think whole of this case might have a bug. Since "p" is a recursing kprobe on
the instruction buffer of the running kprobe, *p->ainsn.insn is the next singlestep
target. I think KPROBE_HIT_SS should be treated as same as KPROBE_HIT_ACTIVE.
If the author thought a situation that a user inserts a kprobe on a breakpoint which
has been set by other debugger, we have to check it in !p case, because
get_kprobe(running_kprobe->ainsn.insn) will return NULL in that case.
>>> + break;
>>> + default:
>>> + /* impossible cases */
>>> + BUG();
>>> }
>>> - }
>
> Replaced deeply nested if-elses with a switch.
Originally, if (kcb->kprobe_status==KPROBE_HIT_SS) && !(*p->ainsn.insn == BREAKPOINT_INSTRUCTION),
it increments nmissed_count and change the status to KPROBE_REENTER. Please trace the flow carefully.
>
> Please let me know if there are any changes on which you would like
> further clarification.
>
>> --
>> Masami Hiramatsu
>>
>> Software Engineer
>> Hitachi Computer Products (America) Inc.
>> Software Solutions Division
>>
>> e-mail: mhiramat@...hat.com, masami.hiramatsu.pt@...achi.com
> --
>
> Thanks,
> Abhishek Sagar
Thank you,
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: mhiramat@...hat.com, masami.hiramatsu.pt@...achi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists