All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping
@ 2016-06-11 14:06 Masami Hiramatsu
  2016-06-13  4:30 ` Ananth N Mavinakayanahalli
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2016-06-11 14:06 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Masami Hiramatsu, linux-kernel, Peter Zijlstra,
	Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
	Andy Lutomirski, systemtap, Steven Rostedt

Fix kprobe_fault_handler to clear TF (trap flag) bit of
flags register in the case of fault fixup on single-stepping.

If we put a kprobe on the instruction which can cause a
page fault (e.g. actual mov instructions in copy_user_*),
that fault happens on a single-stepping buffer. In this
case, kprobes resets running instance so that the CPU can
retry execution on the original ip address.
However, current code forgets reset TF bit. Since this
fault happens with TF bit set for enabling single-stepping,
when it retries, it causes a debug exception and kprobes
can not handle it because it already reset itself.

On the most of x86-64 platform, it can be easily reproduced
by using kprobe tracer. E.g.

  # cd /sys/kernel/debug/tracing
  # echo p copy_user_enhanced_fast_string+5 > kprobe_events
  # echo 1 > events/kprobes/enable

And you'll see a kernel panic on do_debug(), since the debug
trap is not handled by kprobes.

To fix this problem, we just need to clear the TF bit when
resetting running kprobe.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 38cf7a7..856df81 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -961,6 +961,13 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 		 * normal page fault.
 		 */
 		regs->ip = (unsigned long)cur->addr;
+		/*
+		 * Trap flag has been set here because this fault happened
+		 * where the single stepping will be done. So clear it with
+		 * resetting current kprobe.
+		 */
+		regs->flags &= ~X86_EFLAGS_TF;
+		/* If the TF was set before the kprobe hit, don't touch it */
 		regs->flags |= kcb->kprobe_old_flags;
 		if (kcb->kprobe_status == KPROBE_REENTER)
 			restore_previous_kprobe(kcb);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping
  2016-06-11 14:06 [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping Masami Hiramatsu
@ 2016-06-13  4:30 ` Ananth N Mavinakayanahalli
  2016-06-13 23:13 ` Steven Rostedt
  2016-06-14 11:31 ` [tip:perf/urgent] kprobes/x86: Clear TF bit in fault on single-stepping tip-bot for Masami Hiramatsu
  2 siblings, 0 replies; 7+ messages in thread
From: Ananth N Mavinakayanahalli @ 2016-06-13  4:30 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Ingo Molnar, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	H . Peter Anvin, Andy Lutomirski, systemtap, Steven Rostedt

On Sat, Jun 11, 2016 at 11:06:53PM +0900, Masami Hiramatsu wrote:
> Fix kprobe_fault_handler to clear TF (trap flag) bit of
> flags register in the case of fault fixup on single-stepping.
> 
> If we put a kprobe on the instruction which can cause a
> page fault (e.g. actual mov instructions in copy_user_*),
> that fault happens on a single-stepping buffer. In this
> case, kprobes resets running instance so that the CPU can
> retry execution on the original ip address.
> However, current code forgets reset TF bit. Since this
> fault happens with TF bit set for enabling single-stepping,
> when it retries, it causes a debug exception and kprobes
> can not handle it because it already reset itself.
> 
> On the most of x86-64 platform, it can be easily reproduced
> by using kprobe tracer. E.g.
> 
>   # cd /sys/kernel/debug/tracing
>   # echo p copy_user_enhanced_fast_string+5 > kprobe_events
>   # echo 1 > events/kprobes/enable
> 
> And you'll see a kernel panic on do_debug(), since the debug
> trap is not handled by kprobes.
> 
> To fix this problem, we just need to clear the TF bit when
> resetting running kprobe.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Good catch!

Reviewed-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping
  2016-06-11 14:06 [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping Masami Hiramatsu
  2016-06-13  4:30 ` Ananth N Mavinakayanahalli
@ 2016-06-13 23:13 ` Steven Rostedt
  2016-06-13 23:20   ` Steven Rostedt
  2016-06-14 11:31 ` [tip:perf/urgent] kprobes/x86: Clear TF bit in fault on single-stepping tip-bot for Masami Hiramatsu
  2 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2016-06-13 23:13 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Ingo Molnar, linux-kernel, Peter Zijlstra,
	Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
	Andy Lutomirski, systemtap, Linus Torvalds, fenghua.yu

On Sat, 11 Jun 2016 23:06:53 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Fix kprobe_fault_handler to clear TF (trap flag) bit of
> flags register in the case of fault fixup on single-stepping.
> 
> If we put a kprobe on the instruction which can cause a
> page fault (e.g. actual mov instructions in copy_user_*),
> that fault happens on a single-stepping buffer. In this
> case, kprobes resets running instance so that the CPU can
> retry execution on the original ip address.
> However, current code forgets reset TF bit. Since this
> fault happens with TF bit set for enabling single-stepping,
> when it retries, it causes a debug exception and kprobes
> can not handle it because it already reset itself.
> 
> On the most of x86-64 platform, it can be easily reproduced
> by using kprobe tracer. E.g.
> 
>   # cd /sys/kernel/debug/tracing
>   # echo p copy_user_enhanced_fast_string+5 > kprobe_events
>   # echo 1 > events/kprobes/enable
> 
> And you'll see a kernel panic on do_debug(), since the debug
> trap is not handled by kprobes.
> 
> To fix this problem, we just need to clear the TF bit when
> resetting running kprobe.
> 

This should definitely be marked for stable, and I bisected it all the
way down to this commit: f4cb1cc18f364d "x86-64, copy_user: Remove zero
byte check before copy user buffer."

I reverted that commit and sure enough, this bug goes away. I'm not
saying the revert should be done. I'm just doing an FYI, and showing how
changes that appear to be a nice clean up can have subtle effects. I'm
not even sure how that change caused this to be a problem with kprobes.

The proper fix is this patch.

Acked-by: Steven Rostedt <rostedt@goodmis.org>

Please add:

Cc: stable@vger.kernel.org # v3.14+

-- Steve


> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/x86/kernel/kprobes/core.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 38cf7a7..856df81 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -961,6 +961,13 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
>  		 * normal page fault.
>  		 */
>  		regs->ip = (unsigned long)cur->addr;
> +		/*
> +		 * Trap flag has been set here because this fault happened
> +		 * where the single stepping will be done. So clear it with
> +		 * resetting current kprobe.
> +		 */
> +		regs->flags &= ~X86_EFLAGS_TF;
> +		/* If the TF was set before the kprobe hit, don't touch it */
>  		regs->flags |= kcb->kprobe_old_flags;
>  		if (kcb->kprobe_status == KPROBE_REENTER)
>  			restore_previous_kprobe(kcb);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping
  2016-06-13 23:13 ` Steven Rostedt
@ 2016-06-13 23:20   ` Steven Rostedt
  2016-06-14  1:19     ` Masami Hiramatsu
  2016-06-14  9:59     ` Ingo Molnar
  0 siblings, 2 replies; 7+ messages in thread
From: Steven Rostedt @ 2016-06-13 23:20 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Ingo Molnar, linux-kernel, Peter Zijlstra,
	Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
	Andy Lutomirski, systemtap, Linus Torvalds, fenghua.yu

On Mon, 13 Jun 2016 19:13:45 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:


> >   # cd /sys/kernel/debug/tracing
> >   # echo p copy_user_enhanced_fast_string+5 > kprobe_events
> >   # echo 1 > events/kprobes/enable
> > 
> > And you'll see a kernel panic on do_debug(), since the debug
> > trap is not handled by kprobes.
> > 
> > To fix this problem, we just need to clear the TF bit when
> > resetting running kprobe.
> >   
> 
> This should definitely be marked for stable, and I bisected it all the
> way down to this commit: f4cb1cc18f364d "x86-64, copy_user: Remove zero
> byte check before copy user buffer."
> 
> I reverted that commit and sure enough, this bug goes away. I'm not
> saying the revert should be done. I'm just doing an FYI, and showing how
> changes that appear to be a nice clean up can have subtle effects. I'm
> not even sure how that change caused this to be a problem with kprobes.
> 

Nevermind, reverting that commit only moved the location of the
"rep movsb" that you were placing the kprobe on. When I do:

  echo "p copy_user_enhanced_fast_string+9" > kprobe_events

I get the same result.

That means we need to make that stable tag even earlier.

-- Steve

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping
  2016-06-13 23:20   ` Steven Rostedt
@ 2016-06-14  1:19     ` Masami Hiramatsu
  2016-06-14  9:59     ` Ingo Molnar
  1 sibling, 0 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2016-06-14  1:19 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ingo Molnar, linux-kernel, Peter Zijlstra,
	Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
	Andy Lutomirski, systemtap, Linus Torvalds, fenghua.yu

On Mon, 13 Jun 2016 19:20:19 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 13 Jun 2016 19:13:45 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> 
> > >   # cd /sys/kernel/debug/tracing
> > >   # echo p copy_user_enhanced_fast_string+5 > kprobe_events
> > >   # echo 1 > events/kprobes/enable
> > > 
> > > And you'll see a kernel panic on do_debug(), since the debug
> > > trap is not handled by kprobes.
> > > 
> > > To fix this problem, we just need to clear the TF bit when
> > > resetting running kprobe.
> > >   
> > 
> > This should definitely be marked for stable, and I bisected it all the
> > way down to this commit: f4cb1cc18f364d "x86-64, copy_user: Remove zero
> > byte check before copy user buffer."

I agree this is for stable.

> > I reverted that commit and sure enough, this bug goes away. I'm not
> > saying the revert should be done. I'm just doing an FYI, and showing how
> > changes that appear to be a nice clean up can have subtle effects. I'm
> > not even sure how that change caused this to be a problem with kprobes.
> > 
> 
> Nevermind, reverting that commit only moved the location of the
> "rep movsb" that you were placing the kprobe on. When I do:
> 
>   echo "p copy_user_enhanced_fast_string+9" > kprobe_events
> 
> I get the same result.
> 
> That means we need to make that stable tag even earlier.

Yeah, I think it may never be tested from beginning,
so more thant 10 years we have this issue on the kernel.
I recommend this for all the stable/long term support kernels.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping
  2016-06-13 23:20   ` Steven Rostedt
  2016-06-14  1:19     ` Masami Hiramatsu
@ 2016-06-14  9:59     ` Ingo Molnar
  1 sibling, 0 replies; 7+ messages in thread
From: Ingo Molnar @ 2016-06-14  9:59 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Ingo Molnar, linux-kernel, Peter Zijlstra,
	Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
	Andy Lutomirski, systemtap, Linus Torvalds, fenghua.yu


* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 13 Jun 2016 19:13:45 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> 
> > >   # cd /sys/kernel/debug/tracing
> > >   # echo p copy_user_enhanced_fast_string+5 > kprobe_events
> > >   # echo 1 > events/kprobes/enable
> > > 
> > > And you'll see a kernel panic on do_debug(), since the debug
> > > trap is not handled by kprobes.
> > > 
> > > To fix this problem, we just need to clear the TF bit when
> > > resetting running kprobe.
> > >   
> > 
> > This should definitely be marked for stable, and I bisected it all the
> > way down to this commit: f4cb1cc18f364d "x86-64, copy_user: Remove zero
> > byte check before copy user buffer."
> > 
> > I reverted that commit and sure enough, this bug goes away. I'm not
> > saying the revert should be done. I'm just doing an FYI, and showing how
> > changes that appear to be a nice clean up can have subtle effects. I'm
> > not even sure how that change caused this to be a problem with kprobes.
> > 
> 
> Nevermind, reverting that commit only moved the location of the
> "rep movsb" that you were placing the kprobe on. When I do:
> 
>   echo "p copy_user_enhanced_fast_string+9" > kprobe_events
> 
> I get the same result.
> 
> That means we need to make that stable tag even earlier.

I added:

  Cc: stable@vger.kernel.org # All the way back to ancient kernels

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [tip:perf/urgent] kprobes/x86: Clear TF bit in fault on single-stepping
  2016-06-11 14:06 [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping Masami Hiramatsu
  2016-06-13  4:30 ` Ananth N Mavinakayanahalli
  2016-06-13 23:13 ` Steven Rostedt
@ 2016-06-14 11:31 ` tip-bot for Masami Hiramatsu
  2 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Masami Hiramatsu @ 2016-06-14 11:31 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: luto, ananth, linux-kernel, brgerst, acme, mingo, bp, peterz,
	eranian, alexander.shishkin, dvlasenk, tglx, vincent.weaver, hpa,
	jolsa, torvalds, mhiramat, rostedt

Commit-ID:  dcfc47248d3f7d28df6f531e6426b933de94370d
Gitweb:     http://git.kernel.org/tip/dcfc47248d3f7d28df6f531e6426b933de94370d
Author:     Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate: Sat, 11 Jun 2016 23:06:53 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 14 Jun 2016 12:00:54 +0200

kprobes/x86: Clear TF bit in fault on single-stepping

Fix kprobe_fault_handler() to clear the TF (trap flag) bit of
the flags register in the case of a fault fixup on single-stepping.

If we put a kprobe on the instruction which caused a
page fault (e.g. actual mov instructions in copy_user_*),
that fault happens on the single-stepping buffer. In this
case, kprobes resets running instance so that the CPU can
retry execution on the original ip address.

However, current code forgets to reset the TF bit. Since this
fault happens with TF bit set for enabling single-stepping,
when it retries, it causes a debug exception and kprobes
can not handle it because it already reset itself.

On the most of x86-64 platform, it can be easily reproduced
by using kprobe tracer. E.g.

  # cd /sys/kernel/debug/tracing
  # echo p copy_user_enhanced_fast_string+5 > kprobe_events
  # echo 1 > events/kprobes/enable

And you'll see a kernel panic on do_debug(), since the debug
trap is not handled by kprobes.

To fix this problem, we just need to clear the TF bit when
resetting running kprobe.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: systemtap@sourceware.org
Cc: stable@vger.kernel.org # All the way back to ancient kernels
Link: http://lkml.kernel.org/r/20160611140648.25885.37482.stgit@devbox
[ Updated the comments. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/kprobes/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 38cf7a7..7847e5c 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -961,7 +961,19 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 		 * normal page fault.
 		 */
 		regs->ip = (unsigned long)cur->addr;
+		/*
+		 * Trap flag (TF) has been set here because this fault
+		 * happened where the single stepping will be done.
+		 * So clear it by resetting the current kprobe:
+		 */
+		regs->flags &= ~X86_EFLAGS_TF;
+
+		/*
+		 * If the TF flag was set before the kprobe hit,
+		 * don't touch it:
+		 */
 		regs->flags |= kcb->kprobe_old_flags;
+
 		if (kcb->kprobe_status == KPROBE_REENTER)
 			restore_previous_kprobe(kcb);
 		else

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-06-14 11:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-11 14:06 [PATCH tip/master] [BUGFIX] kprobes/x86: Fix to clear TF bit in fault-on-single-stepping Masami Hiramatsu
2016-06-13  4:30 ` Ananth N Mavinakayanahalli
2016-06-13 23:13 ` Steven Rostedt
2016-06-13 23:20   ` Steven Rostedt
2016-06-14  1:19     ` Masami Hiramatsu
2016-06-14  9:59     ` Ingo Molnar
2016-06-14 11:31 ` [tip:perf/urgent] kprobes/x86: Clear TF bit in fault on single-stepping tip-bot for Masami Hiramatsu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.