linux-kernel - Re: [PATCH] x86: kprobes change kprobe

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <477D184A.8090009@redhat.com>
Date:	Thu, 03 Jan 2008 12:15:54 -0500
From:	Masami Hiramatsu <mhiramat@...hat.com>
To:	Abhishek Sagar <sagar.abhishek@...il.com>
CC:	Ingo Molnar <mingo@...e.hu>,
	Harvey Harrison <harvey.harrison@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>, qbarnes@...il.com,
	ananth@...ibm.com, jkenisto@...ibm.com
Subject: Re: [PATCH] x86: kprobes change kprobe_handler flow

Hi Abhishek,

Masami Hiramatsu wrote:
...
>>>> +     case KPROBE_HIT_SS:
>>>> +             if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
>>>> +                     regs->flags &= ~TF_MASK;
>>>> +                     regs->flags |= kcb->kprobe_saved_flags;
>>>> +             } else {
>>>> +                     /* BUG? */
>>>> +             }
>>>> +             break;
>>> If my thought is correct, we don't need to use swich-case here,
>>> Because there are only 2 cases, KPROBE_HIT_SSDONE (x86-64 only)
>>> or others.
>>> As a result, this function just setups re-entrance.
>> As you've also pointed out in your previous reply, this case is
>> peculiar and therefore I believe it should be marked as a BUG(). I've
>> left the original case, if (kcb->kprobe_status==KPROBE_HIT_SS) &&
>> (*p->ainsn.insn == BREAKPOINT_INSTRUCTION), untouched and is handled
>> as it was before. However, if (kcb->kprobe_status==KPROBE_HIT_SS) &&
>> !(*p->ainsn.insn == BREAKPOINT_INSTRUCTION), then instead of
>> incrementing nmissed count like before, it should cry out a BUG. This
>> is not an ordinary recursive probe handling case which should update
>> the nmissed count.
> 
> Hmm, I can not agree, because it is possible to insert a kprobe
> into kprobe's instruction buffer. If it should be a bug, we must
> check it when registering the kprobe.

I discussed it with other maintainers and knew that current kprobes
does not allow user to insert a kprobe to another kprobe's instruction
buffer, because register_kprobe ensures the insertion address is text.
Now I changed my mind. I think that case (p && kprobe_running() &&
kcb->kprobe_status==KPROBE_HIT_SS) is BUG(), even if (*p->ainsn.insn ==
BREAKPOINT_INSTRUCTION).

> (And also, in *p->ainsn.insn == BREAKPOINT_INSTRUCTION case, I doubt
> that the kernel can handle this "orphaned" breakpoint, because the
> breakpoint address has been changed.)

I also changed my mind. In this case, the kernel debugger can retrieve
correct breakpoint address by using kprobe_running() as below.
---
kp = kprobe_running();
if (kp)
	addr = kp->addr;
else
	addr = regs->ip;
---

The last discussion point is that we should restore flags or not if
(!p && kprobe_running() && kcb->kprobe_status==KPROBE_HIT_SS).
I think we do not need to do that if the debugger premises that
kprobes exists.

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/