[PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Yakov Lerner]

I was trying to get the address of instruction to be executed 
next after the kprobed instruction.  But regs->eip in post_handler()
contains value which is useless to the user. It's pre-corrected value.
This value is difficult to use without access to resume_execution(), which
is not exported anyway.
I moved the invocation of post_handler() to *after* resume_execution().
Now regs->eip contains meaningful value in post_handler().

I do not think this change breaks any backward-compatibility.
To make meaning of the old value, post_handler() would need access to
resume_execution() which is not exported.  I have difficulty to believe
that previous, uncorrected, regs->eip can be meaningfully used in
post_handler().

Signed-off-by: Yakov Lerner <iler.ml@gmail.com>
---
 arch/x86/kernel/kprobes.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index 34a5912..60392e2 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -858,15 +858,15 @@ static int __kprobes post_kprobe_handler(struct pt_regs *regs)
 	if (!cur)
 		return 0;
 
+	resume_execution(cur, regs, kcb);
+	regs->flags |= kcb->kprobe_saved_flags;
+	trace_hardirqs_fixup_flags(regs->flags);
+
 	if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
 		kcb->kprobe_status = KPROBE_HIT_SSDONE;
 		cur->post_handler(cur, regs, 0);
 	}
 
-	resume_execution(cur, regs, kcb);
-	regs->flags |= kcb->kprobe_saved_flags;
-	trace_hardirqs_fixup_flags(regs->flags);
-
 	/* Restore back the original saved kprobes variables and continue. */
 	if (kcb->kprobe_status == KPROBE_REENTER) {
 		restore_previous_kprobe(kcb);
-- 
1.5.4.4

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Ananth N Mavinakayanahalli]

On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
> 
> I was trying to get the address of instruction to be executed 
> next after the kprobed instruction.  But regs->eip in post_handler()
> contains value which is useless to the user. It's pre-corrected value.
> This value is difficult to use without access to resume_execution(), which
> is not exported anyway.
> I moved the invocation of post_handler() to *after* resume_execution().
> Now regs->eip contains meaningful value in post_handler().
> 
> I do not think this change breaks any backward-compatibility.
> To make meaning of the old value, post_handler() would need access to
> resume_execution() which is not exported.  I have difficulty to believe
> that previous, uncorrected, regs->eip can be meaningfully used in
> post_handler().

resume_execution() exists not just for the program counter fixups after
out-of-line singlestepping, but is also as an insurance to put the
program counter back to the correct address in case the user's
post_handler() mucks around with it. That isn't possible with this
change :-(

Ananth

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Yakov Lerner]

On Mon, Mar 17, 2008 at 7:19 AM, Ananth N Mavinakayanahalli
<ananth@in.ibm.com> wrote:
> On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
>  >
>  > I was trying to get the address of instruction to be executed
>  > next after the kprobed instruction.  But regs->eip in post_handler()
>  > contains value which is useless to the user. It's pre-corrected value.
>  > This value is difficult to use without access to resume_execution(), which
>  > is not exported anyway.
>  > I moved the invocation of post_handler() to *after* resume_execution().
>  > Now regs->eip contains meaningful value in post_handler().
>  >
>  > I do not think this change breaks any backward-compatibility.
>  > To make meaning of the old value, post_handler() would need access to
>  > resume_execution() which is not exported.  I have difficulty to believe
>  > that previous, uncorrected, regs->eip can be meaningfully used in
>  > post_handler().
>
>  resume_execution() exists not just for the program counter fixups after
>  out-of-line singlestepping, but is also as an insurance to put the
>  program counter back to the correct address in case the user's
>  post_handler() mucks around with it. That isn't possible with this
>  change :-(

I see your point. This can be prevented by saving and restoring regs->ip
around the post_handler() call, no ? Current code is beautiful. Saving and
restoring regs->ip would make this place look ugly.

Otoh, if the post_handler() wants to crash the kernel, it can do it
in thousand ways, not just by trashing regs->ip, no ?

Yakov

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Ananth N Mavinakayanahalli]

On Mon, Mar 17, 2008 at 12:59:05PM +0200, Yakov Lerner wrote:
> On Mon, Mar 17, 2008 at 7:19 AM, Ananth N Mavinakayanahalli
> <ananth@in.ibm.com> wrote:
> > On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
> >  >
> >  > I was trying to get the address of instruction to be executed
> >  > next after the kprobed instruction.  But regs->eip in post_handler()
> >  > contains value which is useless to the user. It's pre-corrected value.
> >  > This value is difficult to use without access to resume_execution(), which
> >  > is not exported anyway.
> >  > I moved the invocation of post_handler() to *after* resume_execution().
> >  > Now regs->eip contains meaningful value in post_handler().
> >  >
> >  > I do not think this change breaks any backward-compatibility.
> >  > To make meaning of the old value, post_handler() would need access to
> >  > resume_execution() which is not exported.  I have difficulty to believe
> >  > that previous, uncorrected, regs->eip can be meaningfully used in
> >  > post_handler().
> >
> >  resume_execution() exists not just for the program counter fixups after
> >  out-of-line singlestepping, but is also as an insurance to put the
> >  program counter back to the correct address in case the user's
> >  post_handler() mucks around with it. That isn't possible with this
> >  change :-(
> 
> I see your point. This can be prevented by saving and restoring regs->ip
> around the post_handler() call, no ? Current code is beautiful. Saving and
> restoring regs->ip would make this place look ugly.
> 
> Otoh, if the post_handler() wants to crash the kernel, it can do it
> in thousand ways, not just by trashing regs->ip, no ?

Of course, there still are other ways to shoot yourself in the foot with
the post_handler(), but, atleast for cases we can control, we need to do
the right thing.

Ananth

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Masami Hiramatsu]

Ananth N Mavinakayanahalli wrote:
> On Mon, Mar 17, 2008 at 12:59:05PM +0200, Yakov Lerner wrote:
>> On Mon, Mar 17, 2008 at 7:19 AM, Ananth N Mavinakayanahalli
>> <ananth@in.ibm.com> wrote:
>>> On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
>>>  >
>>>  > I was trying to get the address of instruction to be executed
>>>  > next after the kprobed instruction.  But regs->eip in post_handler()
>>>  > contains value which is useless to the user. It's pre-corrected value.
>>>  > This value is difficult to use without access to resume_execution(), which
>>>  > is not exported anyway.
>>>  > I moved the invocation of post_handler() to *after* resume_execution().
>>>  > Now regs->eip contains meaningful value in post_handler().
>>>  >
>>>  > I do not think this change breaks any backward-compatibility.
>>>  > To make meaning of the old value, post_handler() would need access to
>>>  > resume_execution() which is not exported.  I have difficulty to believe
>>>  > that previous, uncorrected, regs->eip can be meaningfully used in
>>>  > post_handler().
>>>
>>>  resume_execution() exists not just for the program counter fixups after
>>>  out-of-line singlestepping, but is also as an insurance to put the
>>>  program counter back to the correct address in case the user's
>>>  post_handler() mucks around with it. That isn't possible with this
>>>  change :-(
>> I see your point. This can be prevented by saving and restoring regs->ip
>> around the post_handler() call, no ? Current code is beautiful. Saving and
>> restoring regs->ip would make this place look ugly.
>>
>> Otoh, if the post_handler() wants to crash the kernel, it can do it
>> in thousand ways, not just by trashing regs->ip, no ?
> 
> Of course, there still are other ways to shoot yourself in the foot with
> the post_handler(), but, atleast for cases we can control, we need to do
> the right thing.

Ananth, I think we can not prevent it even if resume_execution() is called
after post_handler, because resume_execution() refers reg->ip...:-(

And Yakov, I think you might need to make a patchset against all arch which
support kprobes, because this patch modifies expected behavior of kprobes
only on x86.

IMHO, Yakov's suggestion will be also good for resume_execution(), because
it only has to clean up after expectable-single-stepping. (user code is
unexpectable... we can not control all of that)


Thanks,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Ananth N Mavinakayanahalli]

On Mon, Mar 17, 2008 at 06:17:21PM -0400, Masami Hiramatsu wrote:
> Ananth N Mavinakayanahalli wrote:
> > On Mon, Mar 17, 2008 at 12:59:05PM +0200, Yakov Lerner wrote:
> >> On Mon, Mar 17, 2008 at 7:19 AM, Ananth N Mavinakayanahalli
> >> <ananth@in.ibm.com> wrote:
> >>> On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
> >>>  >
> >>>  > I was trying to get the address of instruction to be executed
> >>>  > next after the kprobed instruction.  But regs->eip in post_handler()
> >>>  > contains value which is useless to the user. It's pre-corrected value.
> >>>  > This value is difficult to use without access to resume_execution(), which
> >>>  > is not exported anyway.
> >>>  > I moved the invocation of post_handler() to *after* resume_execution().
> >>>  > Now regs->eip contains meaningful value in post_handler().
> >>>  >
> >>>  > I do not think this change breaks any backward-compatibility.
> >>>  > To make meaning of the old value, post_handler() would need access to
> >>>  > resume_execution() which is not exported.  I have difficulty to believe
> >>>  > that previous, uncorrected, regs->eip can be meaningfully used in
> >>>  > post_handler().
> >>>
> >>>  resume_execution() exists not just for the program counter fixups after
> >>>  out-of-line singlestepping, but is also as an insurance to put the
> >>>  program counter back to the correct address in case the user's
> >>>  post_handler() mucks around with it. That isn't possible with this
> >>>  change :-(
> >> I see your point. This can be prevented by saving and restoring regs->ip
> >> around the post_handler() call, no ? Current code is beautiful. Saving and
> >> restoring regs->ip would make this place look ugly.
> >>
> >> Otoh, if the post_handler() wants to crash the kernel, it can do it
> >> in thousand ways, not just by trashing regs->ip, no ?
> > 
> > Of course, there still are other ways to shoot yourself in the foot with
> > the post_handler(), but, atleast for cases we can control, we need to do
> > the right thing.
> 
> Ananth, I think we can not prevent it even if resume_execution() is called
> after post_handler, because resume_execution() refers reg->ip...:-(

OK, I see what you are referring to... though we use kp.addr and
kp.ainsn.insn to start with, we'll need the knowledge of regs->ip at the
end.

> And Yakov, I think you might need to make a patchset against all arch which
> support kprobes, because this patch modifies expected behavior of kprobes
> only on x86.

Agreed.

> IMHO, Yakov's suggestion will be also good for resume_execution(), because
> it only has to clean up after expectable-single-stepping. (user code is
> unexpectable... we can not control all of that)

Ananth

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Ingo Molnar]

* Yakov Lerner <iler.ml@gmail.com> wrote:

> I was trying to get the address of instruction to be executed next 
> after the kprobed instruction.  But regs->eip in post_handler() 
> contains value which is useless to the user. It's pre-corrected value. 
> This value is difficult to use without access to resume_execution(), 
> which is not exported anyway. I moved the invocation of post_handler() 
> to *after* resume_execution(). Now regs->eip contains meaningful value 
> in post_handler().
> 
> I do not think this change breaks any backward-compatibility. To make 
> meaning of the old value, post_handler() would need access to 
> resume_execution() which is not exported.  I have difficulty to 
> believe that previous, uncorrected, regs->eip can be meaningfully used 
> in post_handler().

thanks, i've added your patch to the .26 bucket of x86.git, but it would 
be nice to get an Ack/Nack from a kprobes person as well.

	Ingo

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Ananth N Mavinakayanahalli]

On Fri, Mar 21, 2008 at 12:08:04PM +0100, Ingo Molnar wrote:
> 
> * Yakov Lerner <iler.ml@gmail.com> wrote:
> 
> > I was trying to get the address of instruction to be executed next 
> > after the kprobed instruction.  But regs->eip in post_handler() 
> > contains value which is useless to the user. It's pre-corrected value. 
> > This value is difficult to use without access to resume_execution(), 
> > which is not exported anyway. I moved the invocation of post_handler() 
> > to *after* resume_execution(). Now regs->eip contains meaningful value 
> > in post_handler().
> > 
> > I do not think this change breaks any backward-compatibility. To make 
> > meaning of the old value, post_handler() would need access to 
> > resume_execution() which is not exported.  I have difficulty to 
> > believe that previous, uncorrected, regs->eip can be meaningfully used 
> > in post_handler().
> 
> thanks, i've added your patch to the .26 bucket of x86.git, but it would 
> be nice to get an Ack/Nack from a kprobes person as well.
 
Ingo,

I've tested Yakov's more comprehensive patch on powerpc too. This has my
ack.

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Ingo Molnar]

* Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:

> > thanks, i've added your patch to the .26 bucket of x86.git, but it 
> > would be nice to get an Ack/Nack from a kprobes person as well.
>  
> Ingo,
> 
> I've tested Yakov's more comprehensive patch on powerpc too. This has 
> my ack.
> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

thanks, i've queued up the x86-only patch below for .26 merging. (that 
is all that is needed for x86, and no .25 urgency, right?)

	Ingo

------------------>
Subject: x86, kprobes: correct post-eip value in post_hander()
From: "Yakov Lerner" <iler.ml@gmail.com>
Date: Sun, 16 Mar 2008 03:21:21 -0500

I was trying to get the address of instruction to be executed
next after the kprobed instruction.  But regs->eip in post_handler()
contains value which is useless to the user. It's pre-corrected value.
This value is difficult to use without access to resume_execution(), which
is not exported anyway.
I moved the invocation of post_handler() to *after* resume_execution().
Now regs->eip contains meaningful value in post_handler().

I do not think this change breaks any backward-compatibility.
To make meaning of the old value, post_handler() would need access to
resume_execution() which is not exported.  I have difficulty to believe
that previous, uncorrected, regs->eip can be meaningfully used in
post_handler().

Signed-off-by: Yakov Lerner <iler.ml@gmail.com>
Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/kprobes.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-x86.q/arch/x86/kernel/kprobes.c
===================================================================
--- linux-x86.q.orig/arch/x86/kernel/kprobes.c
+++ linux-x86.q/arch/x86/kernel/kprobes.c
@@ -858,15 +858,15 @@ static int __kprobes post_kprobe_handler
 	if (!cur)
 		return 0;
 
+	resume_execution(cur, regs, kcb);
+	regs->flags |= kcb->kprobe_saved_flags;
+	trace_hardirqs_fixup_flags(regs->flags);
+
 	if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
 		kcb->kprobe_status = KPROBE_HIT_SSDONE;
 		cur->post_handler(cur, regs, 0);
 	}
 
-	resume_execution(cur, regs, kcb);
-	regs->flags |= kcb->kprobe_saved_flags;
-	trace_hardirqs_fixup_flags(regs->flags);
-
 	/* Restore back the original saved kprobes variables and continue. */
 	if (kcb->kprobe_status == KPROBE_REENTER) {
 		restore_previous_kprobe(kcb);

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Ananth N Mavinakayanahalli]

On Fri, Mar 21, 2008 at 03:32:29PM +0100, Ingo Molnar wrote:
> 
> * Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:
> 
> > > thanks, i've added your patch to the .26 bucket of x86.git, but it 
> > > would be nice to get an Ack/Nack from a kprobes person as well.
> >  
> > Ingo,
> > 
> > I've tested Yakov's more comprehensive patch on powerpc too. This has 
> > my ack.
> > 
> > Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> 
> thanks, i've queued up the x86-only patch below for .26 merging. (that 
> is all that is needed for x86, and no .25 urgency, right?)

Yup. This can wait for .26

Ananth

Re: [PATCH] Subject: kprobes-x86: correct post-eip value in post_hander()

[Masami Hiramatsu]

Ingo Molnar wrote:
> * Yakov Lerner <iler.ml@gmail.com> wrote:
> 
>> I was trying to get the address of instruction to be executed next 
>> after the kprobed instruction.  But regs->eip in post_handler() 
>> contains value which is useless to the user. It's pre-corrected value. 
>> This value is difficult to use without access to resume_execution(), 
>> which is not exported anyway. I moved the invocation of post_handler() 
>> to *after* resume_execution(). Now regs->eip contains meaningful value 
>> in post_handler().
>>
>> I do not think this change breaks any backward-compatibility. To make 
>> meaning of the old value, post_handler() would need access to 
>> resume_execution() which is not exported.  I have difficulty to 
>> believe that previous, uncorrected, regs->eip can be meaningfully used 
>> in post_handler().
> 
> thanks, i've added your patch to the .26 bucket of x86.git, but it would 
> be nice to get an Ack/Nack from a kprobes person as well.
> 
> 	Ingo

Ingo, I also tested this on x86-64/x86/ia64.

Acked-by: Masami Hiramatsu <mhiramat@redhat.com>

Thanks,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com