Re: [PATCH 3/3] Make jprobes a little safer for users
On 6/26/07, Michael Ellerman <[EMAIL PROTECTED]> wrote:
It did occur to me that someone might be doing something crazy like
branching to code that's not in the kernel/module text - but I was
hoping that wouldn't be the case. I'm not sure what ITCM is?
The reference to tightly coupled memory (ITCM) was just to have you
consider the possibility of the jprobe handler being outside kernel
text region. Totally paranoid really.
> > int __kprobes register_jprobe(struct jprobe *jp)
> > {
> > + unsigned long addr = arch_deref_entry_point(jp->entry);
> > +
> > + if (!kernel_text_address(addr))
> > + return -EINVAL;
>
> Seems like you're checking for the jprobe handler to be within
> kernel/module range. Why not narrow this down to just module range
> (!module_text_address(addr), say)? Core kernel functions would not be
> ending with a 'jprobe_return()' anyway.
There's jprobe code in net/ipv4/tcp_probe.c and net/dccp/probe.c that
can be builtin or modular, so I think kernel_text_address() is right.
Ok..thanks for that clarification.
--
Abhishek Sagar
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, 26 Jun 2007 16:03:58 +1000 Michael Ellerman <[EMAIL PROTECTED]> wrote:
> On Tue, 2007-06-26 at 07:53 +0200, Christoph Hellwig wrote:
> > On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
> > > I realise jprobes are a razor-blades-included type of interface, but
> > > that doesn't mean we can't try and make them safer to use. This guy I
> > > know once wrote code like this:
> > >
> > > struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
> > >
> > > And then his kernel exploded. Oops.
> > >
> > > This patch adds an arch hook, arch_deref_entry_point() (I don't like it
> > > either)
> > > which takes the void * in a struct jprobe, and gives back the text address
> > > that it represents.
> > >
> > > We can then use that in register_jprobe() to check that the entry point
> > > we're passed is actually in the kernel text, rather than just some random
> > > value.
> >
> > Please don't add more weak functions, they're utterly horrible for
> > anyone trying to understand the code. Otherwise this looks fine to me.
>
> What do you recommend instead? #define ARCH_HAS_FOO_BAR ?
o lord, save us, no.
> I don't see what's utterly horrible about them.
Me either.
> The fact that they're
> weak is fairly reasonable documentation that they're overridden
> somewhere else. And grep/cscope/ctags will find both the weak and
> non-weak versions for you?
yup.
In this case we could just require that each jprobes-supporting
architecture implement arch_deref_entry_point().
Or one could do the Linus trick. In each architecture which implements
arch_deref_entry_point() do:
#define arch_deref_entry_point arch_deref_entry_point
in the per-arch header file then, in non-arch code, do
#ifndef arch_deref_entry_point
static unsigned long arch_deref_entry_point(...)
{
}
#endif
That's just the ARCH_HAS_FOO_BAR thing, only less fugly.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, 2007-06-26 at 11:49 +0530, Abhishek Sagar wrote:
> On 6/26/07, Michael Ellerman <[EMAIL PROTECTED]> wrote:
>
> > We can then use that in register_jprobe() to check that the entry point
> > we're passed is actually in the kernel text, rather than just some random
> > value.
>
> A similar cleanup is possible even for return probes then. I wonder if
> there are any kprobe related scenarios where the executable code may
> be located outside the core kernel text region (e.g, ITCM?). In that
> case would it also be wrong to assume that the jprobe handler may be
> situated outside the kernel core text / module region? Would it then
> make sense to move this check from register_jprobe() to the arch
> dependent code?
It did occur to me that someone might be doing something crazy like
branching to code that's not in the kernel/module text - but I was
hoping that wouldn't be the case. I'm not sure what ITCM is?
> > int __kprobes register_jprobe(struct jprobe *jp)
> > {
> > + unsigned long addr = arch_deref_entry_point(jp->entry);
> > +
> > + if (!kernel_text_address(addr))
> > + return -EINVAL;
>
> Seems like you're checking for the jprobe handler to be within
> kernel/module range. Why not narrow this down to just module range
> (!module_text_address(addr), say)? Core kernel functions would not be
> ending with a 'jprobe_return()' anyway.
There's jprobe code in net/ipv4/tcp_probe.c and net/dccp/probe.c that
can be builtin or modular, so I think kernel_text_address() is right.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 3/3] Make jprobes a little safer for users
On 6/26/07, Michael Ellerman <[EMAIL PROTECTED]> wrote:
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
A similar cleanup is possible even for return probes then. I wonder if
there are any kprobe related scenarios where the executable code may
be located outside the core kernel text region (e.g, ITCM?). In that
case would it also be wrong to assume that the jprobe handler may be
situated outside the kernel core text / module region? Would it then
make sense to move this check from register_jprobe() to the arch
dependent code?
int __kprobes register_jprobe(struct jprobe *jp)
{
+ unsigned long addr = arch_deref_entry_point(jp->entry);
+
+ if (!kernel_text_address(addr))
+ return -EINVAL;
Seems like you're checking for the jprobe handler to be within
kernel/module range. Why not narrow this down to just module range
(!module_text_address(addr), say)? Core kernel functions would not be
ending with a 'jprobe_return()' anyway.
--
Abhishek Sagar
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, 2007-06-26 at 07:53 +0200, Christoph Hellwig wrote:
> On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
> > I realise jprobes are a razor-blades-included type of interface, but
> > that doesn't mean we can't try and make them safer to use. This guy I
> > know once wrote code like this:
> >
> > struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
> >
> > And then his kernel exploded. Oops.
> >
> > This patch adds an arch hook, arch_deref_entry_point() (I don't like it
> > either)
> > which takes the void * in a struct jprobe, and gives back the text address
> > that it represents.
> >
> > We can then use that in register_jprobe() to check that the entry point
> > we're passed is actually in the kernel text, rather than just some random
> > value.
>
> Please don't add more weak functions, they're utterly horrible for
> anyone trying to understand the code. Otherwise this looks fine to me.
What do you recommend instead? #define ARCH_HAS_FOO_BAR ?
I don't see what's utterly horrible about them. The fact that they're
weak is fairly reasonable documentation that they're overridden
somewhere else. And grep/cscope/ctags will find both the weak and
non-weak versions for you?
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, 2007-06-26 at 07:53 +0200, Christoph Hellwig wrote:
On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
I realise jprobes are a razor-blades-included type of interface, but
that doesn't mean we can't try and make them safer to use. This guy I
know once wrote code like this:
struct jprobe jp = { .kp.symbol_name = foo, .entry = jprobe_foo };
And then his kernel exploded. Oops.
This patch adds an arch hook, arch_deref_entry_point() (I don't like it
either)
which takes the void * in a struct jprobe, and gives back the text address
that it represents.
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
Please don't add more weak functions, they're utterly horrible for
anyone trying to understand the code. Otherwise this looks fine to me.
What do you recommend instead? #define ARCH_HAS_FOO_BAR ?
I don't see what's utterly horrible about them. The fact that they're
weak is fairly reasonable documentation that they're overridden
somewhere else. And grep/cscope/ctags will find both the weak and
non-weak versions for you?
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 3/3] Make jprobes a little safer for users
On 6/26/07, Michael Ellerman [EMAIL PROTECTED] wrote:
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
A similar cleanup is possible even for return probes then. I wonder if
there are any kprobe related scenarios where the executable code may
be located outside the core kernel text region (e.g, ITCM?). In that
case would it also be wrong to assume that the jprobe handler may be
situated outside the kernel core text / module region? Would it then
make sense to move this check from register_jprobe() to the arch
dependent code?
int __kprobes register_jprobe(struct jprobe *jp)
{
+ unsigned long addr = arch_deref_entry_point(jp-entry);
+
+ if (!kernel_text_address(addr))
+ return -EINVAL;
Seems like you're checking for the jprobe handler to be within
kernel/module range. Why not narrow this down to just module range
(!module_text_address(addr), say)? Core kernel functions would not be
ending with a 'jprobe_return()' anyway.
--
Abhishek Sagar
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, 2007-06-26 at 11:49 +0530, Abhishek Sagar wrote:
On 6/26/07, Michael Ellerman [EMAIL PROTECTED] wrote:
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
A similar cleanup is possible even for return probes then. I wonder if
there are any kprobe related scenarios where the executable code may
be located outside the core kernel text region (e.g, ITCM?). In that
case would it also be wrong to assume that the jprobe handler may be
situated outside the kernel core text / module region? Would it then
make sense to move this check from register_jprobe() to the arch
dependent code?
It did occur to me that someone might be doing something crazy like
branching to code that's not in the kernel/module text - but I was
hoping that wouldn't be the case. I'm not sure what ITCM is?
int __kprobes register_jprobe(struct jprobe *jp)
{
+ unsigned long addr = arch_deref_entry_point(jp-entry);
+
+ if (!kernel_text_address(addr))
+ return -EINVAL;
Seems like you're checking for the jprobe handler to be within
kernel/module range. Why not narrow this down to just module range
(!module_text_address(addr), say)? Core kernel functions would not be
ending with a 'jprobe_return()' anyway.
There's jprobe code in net/ipv4/tcp_probe.c and net/dccp/probe.c that
can be builtin or modular, so I think kernel_text_address() is right.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, 26 Jun 2007 16:03:58 +1000 Michael Ellerman [EMAIL PROTECTED] wrote:
On Tue, 2007-06-26 at 07:53 +0200, Christoph Hellwig wrote:
On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
I realise jprobes are a razor-blades-included type of interface, but
that doesn't mean we can't try and make them safer to use. This guy I
know once wrote code like this:
struct jprobe jp = { .kp.symbol_name = foo, .entry = jprobe_foo };
And then his kernel exploded. Oops.
This patch adds an arch hook, arch_deref_entry_point() (I don't like it
either)
which takes the void * in a struct jprobe, and gives back the text address
that it represents.
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
Please don't add more weak functions, they're utterly horrible for
anyone trying to understand the code. Otherwise this looks fine to me.
What do you recommend instead? #define ARCH_HAS_FOO_BAR ?
o lord, save us, no.
I don't see what's utterly horrible about them.
Me either.
The fact that they're
weak is fairly reasonable documentation that they're overridden
somewhere else. And grep/cscope/ctags will find both the weak and
non-weak versions for you?
yup.
In this case we could just require that each jprobes-supporting
architecture implement arch_deref_entry_point().
Or one could do the Linus trick. In each architecture which implements
arch_deref_entry_point() do:
#define arch_deref_entry_point arch_deref_entry_point
in the per-arch header file then, in non-arch code, do
#ifndef arch_deref_entry_point
static unsigned long arch_deref_entry_point(...)
{
generic implementation
}
#endif
That's just the ARCH_HAS_FOO_BAR thing, only less fugly.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On 6/26/07, Michael Ellerman [EMAIL PROTECTED] wrote:
It did occur to me that someone might be doing something crazy like
branching to code that's not in the kernel/module text - but I was
hoping that wouldn't be the case. I'm not sure what ITCM is?
The reference to tightly coupled memory (ITCM) was just to have you
consider the possibility of the jprobe handler being outside kernel
text region. Totally paranoid really.
int __kprobes register_jprobe(struct jprobe *jp)
{
+ unsigned long addr = arch_deref_entry_point(jp-entry);
+
+ if (!kernel_text_address(addr))
+ return -EINVAL;
Seems like you're checking for the jprobe handler to be within
kernel/module range. Why not narrow this down to just module range
(!module_text_address(addr), say)? Core kernel functions would not be
ending with a 'jprobe_return()' anyway.
There's jprobe code in net/ipv4/tcp_probe.c and net/dccp/probe.c that
can be builtin or modular, so I think kernel_text_address() is right.
Ok..thanks for that clarification.
--
Abhishek Sagar
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
> I realise jprobes are a razor-blades-included type of interface, but
> that doesn't mean we can't try and make them safer to use. This guy I
> know once wrote code like this:
>
> struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
>
> And then his kernel exploded. Oops.
>
> This patch adds an arch hook, arch_deref_entry_point() (I don't like it
> either)
> which takes the void * in a struct jprobe, and gives back the text address
> that it represents.
>
> We can then use that in register_jprobe() to check that the entry point
> we're passed is actually in the kernel text, rather than just some random
> value.
Please don't add more weak functions, they're utterly horrible for
anyone trying to understand the code. Otherwise this looks fine to me.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On Mon, 2007-06-25 at 19:00 -0700, Andrew Morton wrote:
> On Tue, 26 Jun 2007 11:48:51 +1000 (EST) Michael Ellerman <[EMAIL PROTECTED]>
> wrote:
>
> > I realise jprobes are a razor-blades-included type of interface, but
> > that doesn't mean we can't try and make them safer to use. This guy I
> > know once wrote code like this:
> >
> > struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
> >
> > And then his kernel exploded. Oops.
> >
> > This patch adds an arch hook, arch_deref_entry_point() (I don't like it
> > either)
> > which takes the void * in a struct jprobe, and gives back the text address
> > that it represents.
> >
> > We can then use that in register_jprobe() to check that the entry point
> > we're passed is actually in the kernel text, rather than just some random
> > value.
> >
> > Signed-off-by: Michael Ellerman <[EMAIL PROTECTED]>
> > ---
> > arch/ia64/kernel/kprobes.c|7 ++-
> > arch/powerpc/kernel/kprobes.c | 11 ---
> > kernel/kprobes.c |9 +
>
> We're missing a declaration of arch_deref_entry_point() in some header file?
Yeah I guess. It's declared weak in kernel/kprobes.c, but there should
be a definition somewhere to make sure the three versions don't get out
of sync. I'll send a patch.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, 26 Jun 2007 11:48:51 +1000 (EST) Michael Ellerman <[EMAIL PROTECTED]>
wrote:
> I realise jprobes are a razor-blades-included type of interface, but
> that doesn't mean we can't try and make them safer to use. This guy I
> know once wrote code like this:
>
> struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
>
> And then his kernel exploded. Oops.
>
> This patch adds an arch hook, arch_deref_entry_point() (I don't like it
> either)
> which takes the void * in a struct jprobe, and gives back the text address
> that it represents.
>
> We can then use that in register_jprobe() to check that the entry point
> we're passed is actually in the kernel text, rather than just some random
> value.
>
> Signed-off-by: Michael Ellerman <[EMAIL PROTECTED]>
> ---
> arch/ia64/kernel/kprobes.c|7 ++-
> arch/powerpc/kernel/kprobes.c | 11 ---
> kernel/kprobes.c |9 +
We're missing a declaration of arch_deref_entry_point() in some header file?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] Make jprobes a little safer for users
I realise jprobes are a razor-blades-included type of interface, but
that doesn't mean we can't try and make them safer to use. This guy I
know once wrote code like this:
struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
And then his kernel exploded. Oops.
This patch adds an arch hook, arch_deref_entry_point() (I don't like it either)
which takes the void * in a struct jprobe, and gives back the text address
that it represents.
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
Signed-off-by: Michael Ellerman <[EMAIL PROTECTED]>
---
arch/ia64/kernel/kprobes.c|7 ++-
arch/powerpc/kernel/kprobes.c | 11 ---
kernel/kprobes.c |9 +
3 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index 5bc46f1..5dc98b5 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -936,10 +936,15 @@ static void ia64_get_bsp_cfm(struct unw_frame_info *info,
void *arg)
return;
}
+unsigned long arch_deref_entry_point(void *entry)
+{
+ return ((struct fnptr *)entry)->ip;
+}
+
int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
{
struct jprobe *jp = container_of(p, struct jprobe, kp);
- unsigned long addr = ((struct fnptr *)(jp->entry))->ip;
+ unsigned long addr = arch_deref_entry_point(jp->entry);
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
struct param_bsp_cfm pa;
int bytes;
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 0c96611..440f5a8 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -492,6 +492,13 @@ int __kprobes kprobe_exceptions_notify(struct
notifier_block *self,
return ret;
}
+#ifdef CONFIG_PPC64
+unsigned long arch_deref_entry_point(void *entry)
+{
+ return (unsigned long)(((func_descr_t *)entry)->entry);
+}
+#endif
+
int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
{
struct jprobe *jp = container_of(p, struct jprobe, kp);
@@ -500,11 +507,9 @@ int __kprobes setjmp_pre_handler(struct kprobe *p, struct
pt_regs *regs)
memcpy(>jprobe_saved_regs, regs, sizeof(struct pt_regs));
/* setup return addr to the jprobe handler routine */
+ regs->nip = arch_deref_entry_point(jp->entry);
#ifdef CONFIG_PPC64
- regs->nip = (unsigned long)(((func_descr_t *)jp->entry)->entry);
regs->gpr[2] = (unsigned long)(((func_descr_t *)jp->entry)->toc);
-#else
- regs->nip = (unsigned long)jp->entry;
#endif
return 1;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9e47d8c..3e9f513 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -675,9 +675,18 @@ static struct notifier_block kprobe_exceptions_nb = {
.priority = 0x7fff /* we need to be notified first */
};
+unsigned long __weak arch_deref_entry_point(void *entry)
+{
+ return (unsigned long)entry;
+}
int __kprobes register_jprobe(struct jprobe *jp)
{
+ unsigned long addr = arch_deref_entry_point(jp->entry);
+
+ if (!kernel_text_address(addr))
+ return -EINVAL;
+
/* Todo: Verify probepoint is a function entry point */
jp->kp.pre_handler = setjmp_pre_handler;
jp->kp.break_handler = longjmp_break_handler;
--
1.5.1.3.g7a33b
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] Make jprobes a little safer for users
I realise jprobes are a razor-blades-included type of interface, but
that doesn't mean we can't try and make them safer to use. This guy I
know once wrote code like this:
struct jprobe jp = { .kp.symbol_name = foo, .entry = jprobe_foo };
And then his kernel exploded. Oops.
This patch adds an arch hook, arch_deref_entry_point() (I don't like it either)
which takes the void * in a struct jprobe, and gives back the text address
that it represents.
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
Signed-off-by: Michael Ellerman [EMAIL PROTECTED]
---
arch/ia64/kernel/kprobes.c|7 ++-
arch/powerpc/kernel/kprobes.c | 11 ---
kernel/kprobes.c |9 +
3 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index 5bc46f1..5dc98b5 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -936,10 +936,15 @@ static void ia64_get_bsp_cfm(struct unw_frame_info *info,
void *arg)
return;
}
+unsigned long arch_deref_entry_point(void *entry)
+{
+ return ((struct fnptr *)entry)-ip;
+}
+
int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
{
struct jprobe *jp = container_of(p, struct jprobe, kp);
- unsigned long addr = ((struct fnptr *)(jp-entry))-ip;
+ unsigned long addr = arch_deref_entry_point(jp-entry);
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
struct param_bsp_cfm pa;
int bytes;
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 0c96611..440f5a8 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -492,6 +492,13 @@ int __kprobes kprobe_exceptions_notify(struct
notifier_block *self,
return ret;
}
+#ifdef CONFIG_PPC64
+unsigned long arch_deref_entry_point(void *entry)
+{
+ return (unsigned long)(((func_descr_t *)entry)-entry);
+}
+#endif
+
int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
{
struct jprobe *jp = container_of(p, struct jprobe, kp);
@@ -500,11 +507,9 @@ int __kprobes setjmp_pre_handler(struct kprobe *p, struct
pt_regs *regs)
memcpy(kcb-jprobe_saved_regs, regs, sizeof(struct pt_regs));
/* setup return addr to the jprobe handler routine */
+ regs-nip = arch_deref_entry_point(jp-entry);
#ifdef CONFIG_PPC64
- regs-nip = (unsigned long)(((func_descr_t *)jp-entry)-entry);
regs-gpr[2] = (unsigned long)(((func_descr_t *)jp-entry)-toc);
-#else
- regs-nip = (unsigned long)jp-entry;
#endif
return 1;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9e47d8c..3e9f513 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -675,9 +675,18 @@ static struct notifier_block kprobe_exceptions_nb = {
.priority = 0x7fff /* we need to be notified first */
};
+unsigned long __weak arch_deref_entry_point(void *entry)
+{
+ return (unsigned long)entry;
+}
int __kprobes register_jprobe(struct jprobe *jp)
{
+ unsigned long addr = arch_deref_entry_point(jp-entry);
+
+ if (!kernel_text_address(addr))
+ return -EINVAL;
+
/* Todo: Verify probepoint is a function entry point */
jp-kp.pre_handler = setjmp_pre_handler;
jp-kp.break_handler = longjmp_break_handler;
--
1.5.1.3.g7a33b
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, 26 Jun 2007 11:48:51 +1000 (EST) Michael Ellerman [EMAIL PROTECTED]
wrote:
I realise jprobes are a razor-blades-included type of interface, but
that doesn't mean we can't try and make them safer to use. This guy I
know once wrote code like this:
struct jprobe jp = { .kp.symbol_name = foo, .entry = jprobe_foo };
And then his kernel exploded. Oops.
This patch adds an arch hook, arch_deref_entry_point() (I don't like it
either)
which takes the void * in a struct jprobe, and gives back the text address
that it represents.
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
Signed-off-by: Michael Ellerman [EMAIL PROTECTED]
---
arch/ia64/kernel/kprobes.c|7 ++-
arch/powerpc/kernel/kprobes.c | 11 ---
kernel/kprobes.c |9 +
We're missing a declaration of arch_deref_entry_point() in some header file?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Make jprobes a little safer for users
On Mon, 2007-06-25 at 19:00 -0700, Andrew Morton wrote:
On Tue, 26 Jun 2007 11:48:51 +1000 (EST) Michael Ellerman [EMAIL PROTECTED]
wrote:
I realise jprobes are a razor-blades-included type of interface, but
that doesn't mean we can't try and make them safer to use. This guy I
know once wrote code like this:
struct jprobe jp = { .kp.symbol_name = foo, .entry = jprobe_foo };
And then his kernel exploded. Oops.
This patch adds an arch hook, arch_deref_entry_point() (I don't like it
either)
which takes the void * in a struct jprobe, and gives back the text address
that it represents.
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
Signed-off-by: Michael Ellerman [EMAIL PROTECTED]
---
arch/ia64/kernel/kprobes.c|7 ++-
arch/powerpc/kernel/kprobes.c | 11 ---
kernel/kprobes.c |9 +
We're missing a declaration of arch_deref_entry_point() in some header file?
Yeah I guess. It's declared weak in kernel/kprobes.c, but there should
be a definition somewhere to make sure the three versions don't get out
of sync. I'll send a patch.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 3/3] Make jprobes a little safer for users
On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
I realise jprobes are a razor-blades-included type of interface, but
that doesn't mean we can't try and make them safer to use. This guy I
know once wrote code like this:
struct jprobe jp = { .kp.symbol_name = foo, .entry = jprobe_foo };
And then his kernel exploded. Oops.
This patch adds an arch hook, arch_deref_entry_point() (I don't like it
either)
which takes the void * in a struct jprobe, and gives back the text address
that it represents.
We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.
Please don't add more weak functions, they're utterly horrible for
anyone trying to understand the code. Otherwise this looks fine to me.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
