| 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
| ||
|
Date: Thu, 27 Aug 2009 18:30:03 +0200
From: Frederic Weisbecker <fweisbec@...il.com>
To: Masami Hiramatsu <mhiramat@...hat.com>
Cc: Ingo Molnar <mingo@...e.hu>, LKML <linux-kernel@...r.kernel.org>,
Ananth N Mavinakayanahalli <ananth@...ibm.com>
Subject: Re: [PATCH 18/18] tracing/kprobes: Dump the culprit kprobe in case
of kprobe recursion
On Thu, Aug 27, 2009 at 11:52:09AM -0400, Masami Hiramatsu wrote:
> Frederic Weisbecker wrote:
>> On Thu, Aug 27, 2009 at 11:30:24AM -0400, Masami Hiramatsu wrote:
>>> Hi Frederic,
>>>
>>> Frederic Weisbecker wrote:
>>>> Kprobes can enter into a probing recursion, ie: a kprobe that does an
>>>> endless loop because one of its core mechanism function used during
>>>> probing is also probed itself.
>>>>
>>>> This patch helps pinpointing the kprobe that raised such recursion
>>>> by dumping it and raising a BUG instead of a warning (we also disarm
>>>> the kprobe to try avoiding recursion in BUG itself). Having a BUG
>>>> instead of a warning stops the stacktrace in the right place and
>>>> doesn't pollute the logs with hundreds of traces that eventually end
>>>> up in a stack overflow.
>>>
>>> Thanks, but I also found similar bug cases.
>>>
>>>>
>>>> Signed-off-by: Frederic Weisbecker<fweisbec@...il.com>
>>>> Cc: Masami Hiramatsu<mhiramat@...hat.com>
>>>> Cc: Ananth N Mavinakayanahalli<ananth@...ibm.com>
>>>> ---
>>>> arch/x86/kernel/kprobes.c | 8 ++++++--
>>>> include/linux/kprobes.h | 2 ++
>>>> kernel/kprobes.c | 7 +++++++
>>>> 3 files changed, 15 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
>>>> index 16ae961..ecee3d2 100644
>>>> --- a/arch/x86/kernel/kprobes.c
>>>> +++ b/arch/x86/kernel/kprobes.c
>>>> @@ -490,9 +490,13 @@ static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
>>>
>>> Before this, kprobes checks p != kprobe_running(), but it's a
>>> meaningless branch. Hitting a kprobe while KPROBES_HIT_SS always
>>> treated as unrecoverable.
>>
>>
>>
>> Yeah, but that's the place where a probe ends up when bad reentrancy happens
>> right?
>
> No, a place which is shared by kprobes and other subsystems, will cause a
> problem.
>
> for example, I found an irq_return case which will be p == kprobe_running()
> on x86-64.
>
> -> <some irq occurs>
> -> irq_return
> -> <hit int3>
> -> do_int3
> -> <handling kprobe (set kprobe_running)>
> -> irq_return (from do_int3)
> -> <hit int3>
> -> do_int3
> <handling kprobe (kprobe_running == p)> <- here!
>
Oh right.
> Perhaps, the original code assumes that it will be caused by an int3
> which another subsystem inserted on out-of-line singlestep buffer
> if the hitting probe is same as current probe.
>
> However, in that case, int3 hitting address is on the out-of-line
> buffer and should be different from first (current) int3 address.
I see.
> So, I think this part should also be removed.
>
> if (p == kprobe_running()) {
> regs->flags &= ~X86_EFLAGS_TF;
> regs->flags |= kcb->kprobe_saved_flags;
> return 0;
> } else {
>
> Thank you,
So my patch is useless? Or is it also useful to detect real
recursion? (despite of such corner cases)
>>
>>
>>
>>>> /* A probe has been hit in the codepath leading up
>>>> * to, or just after, single-stepping of a probed
>>>> * instruction. This entire codepath should strictly
>>>> - * reside in .kprobes.text section. Raise a warning
>>>> - * to highlight this peculiar case.
>>>> + * reside in .kprobes.text section.
>>>> + * Raise a BUG or we'll continue in an endless
>>>> + * reentering loop and eventually a stack overflow.
>>>> */
>>>> + arch_disarm_kprobe(p);
>>>> + dump_kprobe(p);
>>>> + BUG();
>>>> }
>>>> default:
>>>> /* impossible cases */
>>>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>>>> index bcd9c07..87eb79c 100644
>>>> --- a/include/linux/kprobes.h
>>>> +++ b/include/linux/kprobes.h
>>>> @@ -296,6 +296,8 @@ void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>>>> int disable_kprobe(struct kprobe *kp);
>>>> int enable_kprobe(struct kprobe *kp);
>>>>
>>>> +void dump_kprobe(struct kprobe *kp);
>>>> +
>>>> #else /* !CONFIG_KPROBES: */
>>>>
>>>> static inline int kprobes_built_in(void)
>>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>>>> index ef177d6..f72e96c 100644
>>>> --- a/kernel/kprobes.c
>>>> +++ b/kernel/kprobes.c
>>>> @@ -1141,6 +1141,13 @@ static void __kprobes kill_kprobe(struct kprobe *p)
>>>> arch_remove_kprobe(p);
>>>> }
>>>>
>>>> +void __kprobes dump_kprobe(struct kprobe *kp)
>>>> +{
>>>> + printk(KERN_WARNING "Dumping kprobe:\n");
>>>> + printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
>>>> + kp->symbol_name, kp->addr, kp->offset);
>>>> +}
>>>
>>> Since kp->symbol_name + kp->offset = kp->addr, I recommend to show it
>>> as "Kprobe at %s+%x:<%p>\n", kp->symbol_name, kp->offset, kp->addr.
>>
>>
>> Ok I'll fix this, thanks.
>>
>>
>>>> +
>>>> /* Module notifier call back, checking kprobes on the module */
>>>> static int __kprobes kprobes_module_callback(struct notifier_block *nb,
>>>> unsigned long val, void *data)
>>>
>>> Thank you,
>>>
> --
> Masami Hiramatsu
>
> Software Engineer
> Hitachi Computer Products (America), Inc.
> Software Solutions Division
>
> e-mail: mhiramat@...hat.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