[PATCH 0/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[PATCH 0/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Oleg Nesterov]

Hello,

This is the very old bug initially reported by Evan in 2010, see
https://bugzilla.kernel.org/show_bug.cgi?id=16061
Somehow we forgot to fix it and now Pedro reports it again.

>From the changelog:

    Note: in the longer term we should probably change setup_sigcontext()
    to use get_flags() and then just remove this user_disable_single_step().

Yes, but this needs more changes. Lets start with more simple and
backportable fix. Also because I think that enable_single_step() and
the whole TIF_SINGLESTEP/TIF_FORCED_TF logic need some cleanups, but
I am not sure what we can do.

Oleg.

 arch/x86/kernel/signal.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

[PATCH 1/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Oleg Nesterov]

When the TIF_SINGLESTEP tracee dequeues a signal, handle_signal()
clears TIF_FORCED_TF and X86_EFLAGS_TF but leaves TIF_SINGLESTEP set.

If the tracer does PTRACE_SINGLESTEP again, enable_single_step() sets
X86_EFLAGS_TF but not TIF_FORCED_TF. This means that the subsequent
PTRACE_CONT doesn't not clear X86_EFLAGS_TF, and the tracee gets the
wrong SIGTRAP.

Test-case (needs -O2 to avoid prologue insns in signal handler):

	#include <unistd.h>
	#include <stdio.h>
	#include <sys/ptrace.h>
	#include <sys/wait.h>
	#include <sys/user.h>
	#include <assert.h>
	#include <stddef.h>

	void handler(int n)
	{
		asm("nop");
	}

	int child(void)
	{
		assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
		signal(SIGALRM, handler);
		kill(getpid(), SIGALRM);
		return 0x23;
	}

	void *getip(int pid)
	{
		return (void*)ptrace(PTRACE_PEEKUSER, pid,
					offsetof(struct user, regs.rip), 0);
	}

	int main(void)
	{
		int pid, status;

		pid = fork();
		if (!pid)
			return child();

		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGALRM);

		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
		assert((getip(pid) - (void*)handler) == 0);

		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
		assert((getip(pid) - (void*)handler) == 1);

		assert(ptrace(PTRACE_CONT, pid, 0,0) == 0);
		assert(wait(&status) == pid);
		assert(WIFEXITED(status) && WEXITSTATUS(status) == 0x23);

		return 0;
	}

The last assert() fails because PTRACE_CONT wrongly triggers another
single-step and X86_EFLAGS_TF can't be cleared by debugger until the
tracee does sys_rt_sigreturn().

Change handle_signal() to do user_disable_single_step() if stepping,
we do not need to preserve TIF_SINGLESTEP because we are going to do
ptrace_notify(), and it is simply wrong to leak this bit.

While at it, change the comment to explain why we also need to clear
TF unconditionally after setup_rt_frame().

Note: in the longer term we should probably change setup_sigcontext()
to use get_flags() and then just remove this user_disable_single_step().
And, the state of TIF_FORCED_TF can be wrong after restore_sigcontext()
which can set/clear TF, this needs another fix.

Reported-by: Evan Teran <eteran@alum.rit.edu>
Reported-by: Pedro Alves <palves@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/signal.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ed37a76..9d3a15b 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -629,7 +629,8 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
 static void
 handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 {
-	bool failed;
+	bool stepping, failed;
+
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* If so, check system call restarting.. */
@@ -653,12 +654,13 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	}
 
 	/*
-	 * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF
-	 * flag so that register information in the sigcontext is correct.
+	 * If TF is set due to a debugger (TIF_FORCED_TF), clear TF now
+	 * so that register information in the sigcontext is correct and
+	 * then notify the tracer before entering the signal handler.
 	 */
-	if (unlikely(regs->flags & X86_EFLAGS_TF) &&
-	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
-		regs->flags &= ~X86_EFLAGS_TF;
+	stepping = test_thread_flag(TIF_SINGLESTEP);
+	if (stepping)
+		user_disable_single_step(current);
 
 	failed = (setup_rt_frame(ksig, regs) < 0);
 	if (!failed) {
@@ -669,10 +671,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		 * it might disable possible debug exception from the
 		 * signal handler.
 		 *
-		 * Clear TF when entering the signal handler, but
-		 * notify any tracer that was single-stepping it.
-		 * The tracer may want to single-step inside the
-		 * handler too.
+		 * Clear TF for the case when it wasn't set by debugger to
+		 * avoid the recursive send_sigtrap() in SIGTRAP handler.
 		 */
 		regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF);
 		/*
@@ -681,7 +681,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		if (used_math())
 			drop_init_fpu(current);
 	}
-	signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP));
+	signal_setup_done(failed, ksig, stepping);
 }
 
 #ifdef CONFIG_X86_32
-- 
1.5.5.1

Re: [PATCH 1/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Pedro Alves]

Thanks a lot Oleg.

Question - shouldn't ptrace tests be put in
tools/testing/selftests/ptrace/ in the kernel tree nowadays?

Thanks,
Pedro Alves

On 11/03/2014 08:13 PM, Oleg Nesterov wrote:
> When the TIF_SINGLESTEP tracee dequeues a signal, handle_signal()
> clears TIF_FORCED_TF and X86_EFLAGS_TF but leaves TIF_SINGLESTEP set.
> 
> If the tracer does PTRACE_SINGLESTEP again, enable_single_step() sets
> X86_EFLAGS_TF but not TIF_FORCED_TF. This means that the subsequent
> PTRACE_CONT doesn't not clear X86_EFLAGS_TF, and the tracee gets the
> wrong SIGTRAP.
> 
> Test-case (needs -O2 to avoid prologue insns in signal handler):
> 
> 	#include <unistd.h>
> 	#include <stdio.h>
> 	#include <sys/ptrace.h>
> 	#include <sys/wait.h>
> 	#include <sys/user.h>
> 	#include <assert.h>
> 	#include <stddef.h>
> 
> 	void handler(int n)
> 	{
> 		asm("nop");
> 	}
> 
> 	int child(void)
> 	{
> 		assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
> 		signal(SIGALRM, handler);
> 		kill(getpid(), SIGALRM);
> 		return 0x23;
> 	}
> 
> 	void *getip(int pid)
> 	{
> 		return (void*)ptrace(PTRACE_PEEKUSER, pid,
> 					offsetof(struct user, regs.rip), 0);
> 	}
> 
> 	int main(void)
> 	{
> 		int pid, status;
> 
> 		pid = fork();
> 		if (!pid)
> 			return child();
> 
> 		assert(wait(&status) == pid);
> 		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGALRM);
> 
> 		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
> 		assert(wait(&status) == pid);
> 		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
> 		assert((getip(pid) - (void*)handler) == 0);
> 
> 		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
> 		assert(wait(&status) == pid);
> 		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
> 		assert((getip(pid) - (void*)handler) == 1);
> 
> 		assert(ptrace(PTRACE_CONT, pid, 0,0) == 0);
> 		assert(wait(&status) == pid);
> 		assert(WIFEXITED(status) && WEXITSTATUS(status) == 0x23);
> 
> 		return 0;
> 	}
> 
> The last assert() fails because PTRACE_CONT wrongly triggers another
> single-step and X86_EFLAGS_TF can't be cleared by debugger until the
> tracee does sys_rt_sigreturn().
> 
> Change handle_signal() to do user_disable_single_step() if stepping,
> we do not need to preserve TIF_SINGLESTEP because we are going to do
> ptrace_notify(), and it is simply wrong to leak this bit.
> 
> While at it, change the comment to explain why we also need to clear
> TF unconditionally after setup_rt_frame().
> 
> Note: in the longer term we should probably change setup_sigcontext()
> to use get_flags() and then just remove this user_disable_single_step().
> And, the state of TIF_FORCED_TF can be wrong after restore_sigcontext()
> which can set/clear TF, this needs another fix.
> 
> Reported-by: Evan Teran <eteran@alum.rit.edu>
> Reported-by: Pedro Alves <palves@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/kernel/signal.c |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index ed37a76..9d3a15b 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -629,7 +629,8 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
>  static void
>  handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  {
> -	bool failed;
> +	bool stepping, failed;
> +
>  	/* Are we from a system call? */
>  	if (syscall_get_nr(current, regs) >= 0) {
>  		/* If so, check system call restarting.. */
> @@ -653,12 +654,13 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  	}
>  
>  	/*
> -	 * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF
> -	 * flag so that register information in the sigcontext is correct.
> +	 * If TF is set due to a debugger (TIF_FORCED_TF), clear TF now
> +	 * so that register information in the sigcontext is correct and
> +	 * then notify the tracer before entering the signal handler.
>  	 */
> -	if (unlikely(regs->flags & X86_EFLAGS_TF) &&
> -	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
> -		regs->flags &= ~X86_EFLAGS_TF;
> +	stepping = test_thread_flag(TIF_SINGLESTEP);
> +	if (stepping)
> +		user_disable_single_step(current);
>  
>  	failed = (setup_rt_frame(ksig, regs) < 0);
>  	if (!failed) {
> @@ -669,10 +671,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  		 * it might disable possible debug exception from the
>  		 * signal handler.
>  		 *
> -		 * Clear TF when entering the signal handler, but
> -		 * notify any tracer that was single-stepping it.
> -		 * The tracer may want to single-step inside the
> -		 * handler too.
> +		 * Clear TF for the case when it wasn't set by debugger to
> +		 * avoid the recursive send_sigtrap() in SIGTRAP handler.
>  		 */
>  		regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF);
>  		/*
> @@ -681,7 +681,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  		if (used_math())
>  			drop_init_fpu(current);
>  	}
> -	signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP));
> +	signal_setup_done(failed, ksig, stepping);
>  }
>  
>  #ifdef CONFIG_X86_32
> 

Re: [PATCH 1/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Oleg Nesterov]

On 11/03, Pedro Alves wrote:
>
> Thanks a lot Oleg.

thanks for the detailed report ;)

> Question - shouldn't ptrace tests be put in
> tools/testing/selftests/ptrace/ in the kernel tree nowadays?

Oh, I do not know. Personally I am not sure that selftests/ptrace/
makes sense and I did not even know (or forgot) we already have it.
To me it would be better to move the single peeksiginfo.c to ptrace
testsuite and remove this dir.

That said, I certainly won't argue if you or Jan will maintain
selftests/ptrace and send the patches with the new test-cases ;)

The only problem is that every new test-case should be justified
somehow. For example, should we add the test-case from this changelog
into selftests/ptrace/ ? Honestly, I do not know. This bug is minor,
and probably we do not want a test-case for every bug we fix. So I'd
leave this to you, you know how ptrace is actually _used_ and what is
important for gdb.

The same for other tests in ptrace testsuite. Some of them are really
"random", in any case (I think) we should not blindly put them all into
selftests/ptrace. Not to mention the coding style which should be fixed.

And I know that gdb has a lot of internal tests, and gdb developers
run them (and ptrace tests) to check the new kernels... Who else do
you think will use selftests/ptrace?

But again, if you/Jan want to add something into selftests - please
send a patch. I will even try to review it but only in a sense that
I will try to convince myself that I understand _what_ this test does,
not _why_ we need this test-case. You certainly understand "why" much
better.

Add Denys, perhaps he also has some tests for strace.

Oleg.

Re: [PATCH 1/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Pedro Alves]

On 11/04/2014 11:55 PM, Oleg Nesterov wrote:
> On 11/03, Pedro Alves wrote:
>>
>> Thanks a lot Oleg.
> 
> thanks for the detailed report ;)
> 
>> Question - shouldn't ptrace tests be put in
>> tools/testing/selftests/ptrace/ in the kernel tree nowadays?
> 
> Oh, I do not know. Personally I am not sure that selftests/ptrace/
> makes sense and I did not even know (or forgot) we already have it.

I didn't know about it either until recently.

> To me it would be better to move the single peeksiginfo.c to ptrace
> testsuite and remove this dir.
> 
> That said, I certainly won't argue if you or Jan will maintain
> selftests/ptrace and send the patches with the new test-cases ;)

IMO, having the tests in the kernel tree might make it simpler
to require fixes to come along with tests, so we're better covered
against regressions, but whatever works best to whoever is
maintaining ptrace is good for me.  ;-)

> 
> The only problem is that every new test-case should be justified
> somehow. For example, should we add the test-case from this changelog
> into selftests/ptrace/ ? 

IMO, we should have a ptrace test for this somewhere.  I just don't
know about the intention of selftests/ptrace/, but it looked like
it was meant as a replacement for the out of tree testsuite.
Having the ptrace testsuite in the tree makes sense to me,
for making it easier to require ptrace fixes and extensions to
come along with tests, and to make it easier to catch regressions
closer to the source, and to serve as self-documentation on
expected behavior, but I don't have time to actively pursue
that myself, unfortunately.

> Honestly, I do not know. This bug is minor,
> and probably we do not want a test-case for every bug we fix. 

My view is that there was a bug, and a test case would help
prevent re-introducing it.

> So I'd
> leave this to you, you know how ptrace is actually _used_ and what is
> important for gdb.

Right.  FYI, I've added a testcase to gdb as well, to cover the use
case from a higher level functionality view:

 https://sourceware.org/ml/gdb-patches/2014-10/msg00780.html

(Between a bug/regression being introduced in the kernel and running
the gdb testsuite against that, a lot of time may pass.  Myself,
I usually only test and develop against whatever kernel the
distro ships, which is a released kernel.  And gdb doesn't use
every ptrace feature.)

> 
> The same for other tests in ptrace testsuite. Some of them are really
> "random", in any case (I think) we should not blindly put them all into
> selftests/ptrace. Not to mention the coding style which should be fixed.

Agreed.

> 
> And I know that gdb has a lot of internal tests, and gdb developers
> run them (and ptrace tests) to check the new kernels... Who else do
> you think will use selftests/ptrace?

IMO, whoever touches ptrace code in the kernel should be
required to run the ptrace tests, but not the testsuites of
random other tools that use ptrace (like gdb, strace and others).
Having the ptrace tests in the tree sounded like might make
such a requirement simpler, and I naively thought that the existence
of the directory might mean that that's the direction kernel folks
had agreed on, that's all.

> 
> But again, if you/Jan want to add something into selftests - please
> send a patch. I will even try to review it but only in a sense that
> I will try to convince myself that I understand _what_ this test does,
> not _why_ we need this test-case. You certainly understand "why" much
> better.
> 
> Add Denys, perhaps he also has some tests for strace.

Thanks,
Pedro Alves

Re: [PATCH 1/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Oleg Nesterov]

ping ;)

Should I resend? This fixes the real (although not that serious) bug
and nobody objected.

On 11/03, Oleg Nesterov wrote:
>
> When the TIF_SINGLESTEP tracee dequeues a signal, handle_signal()
> clears TIF_FORCED_TF and X86_EFLAGS_TF but leaves TIF_SINGLESTEP set.
> 
> If the tracer does PTRACE_SINGLESTEP again, enable_single_step() sets
> X86_EFLAGS_TF but not TIF_FORCED_TF. This means that the subsequent
> PTRACE_CONT doesn't not clear X86_EFLAGS_TF, and the tracee gets the
> wrong SIGTRAP.
> 
> Test-case (needs -O2 to avoid prologue insns in signal handler):
> 
> 	#include <unistd.h>
> 	#include <stdio.h>
> 	#include <sys/ptrace.h>
> 	#include <sys/wait.h>
> 	#include <sys/user.h>
> 	#include <assert.h>
> 	#include <stddef.h>
> 
> 	void handler(int n)
> 	{
> 		asm("nop");
> 	}
> 
> 	int child(void)
> 	{
> 		assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
> 		signal(SIGALRM, handler);
> 		kill(getpid(), SIGALRM);
> 		return 0x23;
> 	}
> 
> 	void *getip(int pid)
> 	{
> 		return (void*)ptrace(PTRACE_PEEKUSER, pid,
> 					offsetof(struct user, regs.rip), 0);
> 	}
> 
> 	int main(void)
> 	{
> 		int pid, status;
> 
> 		pid = fork();
> 		if (!pid)
> 			return child();
> 
> 		assert(wait(&status) == pid);
> 		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGALRM);
> 
> 		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
> 		assert(wait(&status) == pid);
> 		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
> 		assert((getip(pid) - (void*)handler) == 0);
> 
> 		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
> 		assert(wait(&status) == pid);
> 		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
> 		assert((getip(pid) - (void*)handler) == 1);
> 
> 		assert(ptrace(PTRACE_CONT, pid, 0,0) == 0);
> 		assert(wait(&status) == pid);
> 		assert(WIFEXITED(status) && WEXITSTATUS(status) == 0x23);
> 
> 		return 0;
> 	}
> 
> The last assert() fails because PTRACE_CONT wrongly triggers another
> single-step and X86_EFLAGS_TF can't be cleared by debugger until the
> tracee does sys_rt_sigreturn().
> 
> Change handle_signal() to do user_disable_single_step() if stepping,
> we do not need to preserve TIF_SINGLESTEP because we are going to do
> ptrace_notify(), and it is simply wrong to leak this bit.
> 
> While at it, change the comment to explain why we also need to clear
> TF unconditionally after setup_rt_frame().
> 
> Note: in the longer term we should probably change setup_sigcontext()
> to use get_flags() and then just remove this user_disable_single_step().
> And, the state of TIF_FORCED_TF can be wrong after restore_sigcontext()
> which can set/clear TF, this needs another fix.
> 
> Reported-by: Evan Teran <eteran@alum.rit.edu>
> Reported-by: Pedro Alves <palves@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/kernel/signal.c |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index ed37a76..9d3a15b 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -629,7 +629,8 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
>  static void
>  handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  {
> -	bool failed;
> +	bool stepping, failed;
> +
>  	/* Are we from a system call? */
>  	if (syscall_get_nr(current, regs) >= 0) {
>  		/* If so, check system call restarting.. */
> @@ -653,12 +654,13 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  	}
>  
>  	/*
> -	 * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF
> -	 * flag so that register information in the sigcontext is correct.
> +	 * If TF is set due to a debugger (TIF_FORCED_TF), clear TF now
> +	 * so that register information in the sigcontext is correct and
> +	 * then notify the tracer before entering the signal handler.
>  	 */
> -	if (unlikely(regs->flags & X86_EFLAGS_TF) &&
> -	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
> -		regs->flags &= ~X86_EFLAGS_TF;
> +	stepping = test_thread_flag(TIF_SINGLESTEP);
> +	if (stepping)
> +		user_disable_single_step(current);
>  
>  	failed = (setup_rt_frame(ksig, regs) < 0);
>  	if (!failed) {
> @@ -669,10 +671,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  		 * it might disable possible debug exception from the
>  		 * signal handler.
>  		 *
> -		 * Clear TF when entering the signal handler, but
> -		 * notify any tracer that was single-stepping it.
> -		 * The tracer may want to single-step inside the
> -		 * handler too.
> +		 * Clear TF for the case when it wasn't set by debugger to
> +		 * avoid the recursive send_sigtrap() in SIGTRAP handler.
>  		 */
>  		regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF);
>  		/*
> @@ -681,7 +681,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  		if (used_math())
>  			drop_init_fpu(current);
>  	}
> -	signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP));
> +	signal_setup_done(failed, ksig, stepping);
>  }
>  
>  #ifdef CONFIG_X86_32
> -- 
> 1.5.5.1
> 

Re: [PATCH 1/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Andres Freund]

Hi,

Could this patch please be picked up? I very regularly hit the problems
caused due to this in gdb (just single step out of a system call that
returns due to EINTR to reproduce and then single step some more...).

I've first spent an embarassing amount of time trying to figure out
what's wrong, to then find a closed bugzilla entry from 2010, to then
find this thread...

I've verified that applying the fix ontop 1fa185ebcbc fixes the issue
for me. So feel free to add a Tested-By. I, by far, don't understand the
code well enough for an actual review though, sorry.

Anything else I can do?

Greetings,

Andres Freund

Re: [PATCH 1/1] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Oleg Nesterov]

On 02/17, Andres Freund wrote:
>
> Could this patch please be picked up? I very regularly hit the problems
> caused due to this in gdb (just single step out of a system call that
> returns due to EINTR to reproduce and then single step some more...).
>
> I've first spent an embarassing amount of time trying to figure out
> what's wrong, to then find a closed bugzilla entry from 2010, to then
> find this thread...
>
> I've verified that applying the fix ontop 1fa185ebcbc fixes the issue
> for me. So feel free to add a Tested-By. I, by far, don't understand the
> code well enough for an actual review though, sorry.
>
> Anything else I can do?

Lets hope your tested-by will help ;) I'll resend this fix once again.

Oleg.

[PATCH RESEND] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Oleg Nesterov]

When the TIF_SINGLESTEP tracee dequeues a signal, handle_signal()
clears TIF_FORCED_TF and X86_EFLAGS_TF but leaves TIF_SINGLESTEP set.

If the tracer does PTRACE_SINGLESTEP again, enable_single_step() sets
X86_EFLAGS_TF but not TIF_FORCED_TF. This means that the subsequent
PTRACE_CONT doesn't not clear X86_EFLAGS_TF, and the tracee gets the
wrong SIGTRAP.

Test-case (needs -O2 to avoid prologue insns in signal handler):

	#include <unistd.h>
	#include <stdio.h>
	#include <sys/ptrace.h>
	#include <sys/wait.h>
	#include <sys/user.h>
	#include <assert.h>
	#include <stddef.h>

	void handler(int n)
	{
		asm("nop");
	}

	int child(void)
	{
		assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
		signal(SIGALRM, handler);
		kill(getpid(), SIGALRM);
		return 0x23;
	}

	void *getip(int pid)
	{
		return (void*)ptrace(PTRACE_PEEKUSER, pid,
					offsetof(struct user, regs.rip), 0);
	}

	int main(void)
	{
		int pid, status;

		pid = fork();
		if (!pid)
			return child();

		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGALRM);

		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
		assert((getip(pid) - (void*)handler) == 0);

		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
		assert((getip(pid) - (void*)handler) == 1);

		assert(ptrace(PTRACE_CONT, pid, 0,0) == 0);
		assert(wait(&status) == pid);
		assert(WIFEXITED(status) && WEXITSTATUS(status) == 0x23);

		return 0;
	}

The last assert() fails because PTRACE_CONT wrongly triggers another
single-step and X86_EFLAGS_TF can't be cleared by debugger until the
tracee does sys_rt_sigreturn().

Change handle_signal() to do user_disable_single_step() if stepping,
we do not need to preserve TIF_SINGLESTEP because we are going to do
ptrace_notify(), and it is simply wrong to leak this bit.

While at it, change the comment to explain why we also need to clear
TF unconditionally after setup_rt_frame().

Note: in the longer term we should probably change setup_sigcontext()
to use get_flags() and then just remove this user_disable_single_step().
And, the state of TIF_FORCED_TF can be wrong after restore_sigcontext()
which can set/clear TF, this needs another fix.

Reported-by: Evan Teran <eteran@alum.rit.edu>
Reported-by: Pedro Alves <palves@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/signal.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ed37a76..9d3a15b 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -629,7 +629,8 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
 static void
 handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 {
-	bool failed;
+	bool stepping, failed;
+
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* If so, check system call restarting.. */
@@ -653,12 +654,13 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	}
 
 	/*
-	 * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF
-	 * flag so that register information in the sigcontext is correct.
+	 * If TF is set due to a debugger (TIF_FORCED_TF), clear TF now
+	 * so that register information in the sigcontext is correct and
+	 * then notify the tracer before entering the signal handler.
 	 */
-	if (unlikely(regs->flags & X86_EFLAGS_TF) &&
-	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
-		regs->flags &= ~X86_EFLAGS_TF;
+	stepping = test_thread_flag(TIF_SINGLESTEP);
+	if (stepping)
+		user_disable_single_step(current);
 
 	failed = (setup_rt_frame(ksig, regs) < 0);
 	if (!failed) {
@@ -669,10 +671,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		 * it might disable possible debug exception from the
 		 * signal handler.
 		 *
-		 * Clear TF when entering the signal handler, but
-		 * notify any tracer that was single-stepping it.
-		 * The tracer may want to single-step inside the
-		 * handler too.
+		 * Clear TF for the case when it wasn't set by debugger to
+		 * avoid the recursive send_sigtrap() in SIGTRAP handler.
 		 */
 		regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF);
 		/*
@@ -681,7 +681,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		if (used_math())
 			drop_init_fpu(current);
 	}
-	signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP));
+	signal_setup_done(failed, ksig, stepping);
 }
 
 #ifdef CONFIG_X86_32
-- 
1.5.5.1

[PATCH 2nd RESEND] ptrace/x86: fix the TIF_FORCED_TF logic in handle_signal()

[Oleg Nesterov]

When the TIF_SINGLESTEP tracee dequeues a signal, handle_signal()
clears TIF_FORCED_TF and X86_EFLAGS_TF but leaves TIF_SINGLESTEP set.

If the tracer does PTRACE_SINGLESTEP again, enable_single_step() sets
X86_EFLAGS_TF but not TIF_FORCED_TF. This means that the subsequent
PTRACE_CONT doesn't not clear X86_EFLAGS_TF, and the tracee gets the
wrong SIGTRAP.

Test-case (needs -O2 to avoid prologue insns in signal handler):

	#include <unistd.h>
	#include <stdio.h>
	#include <sys/ptrace.h>
	#include <sys/wait.h>
	#include <sys/user.h>
	#include <assert.h>
	#include <stddef.h>

	void handler(int n)
	{
		asm("nop");
	}

	int child(void)
	{
		assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
		signal(SIGALRM, handler);
		kill(getpid(), SIGALRM);
		return 0x23;
	}

	void *getip(int pid)
	{
		return (void*)ptrace(PTRACE_PEEKUSER, pid,
					offsetof(struct user, regs.rip), 0);
	}

	int main(void)
	{
		int pid, status;

		pid = fork();
		if (!pid)
			return child();

		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGALRM);

		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
		assert((getip(pid) - (void*)handler) == 0);

		assert(ptrace(PTRACE_SINGLESTEP, pid, 0, SIGALRM) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
		assert((getip(pid) - (void*)handler) == 1);

		assert(ptrace(PTRACE_CONT, pid, 0,0) == 0);
		assert(wait(&status) == pid);
		assert(WIFEXITED(status) && WEXITSTATUS(status) == 0x23);

		return 0;
	}

The last assert() fails because PTRACE_CONT wrongly triggers another
single-step and X86_EFLAGS_TF can't be cleared by debugger until the
tracee does sys_rt_sigreturn().

Change handle_signal() to do user_disable_single_step() if stepping,
we do not need to preserve TIF_SINGLESTEP because we are going to do
ptrace_notify(), and it is simply wrong to leak this bit.

While at it, change the comment to explain why we also need to clear
TF unconditionally after setup_rt_frame().

Note: in the longer term we should probably change setup_sigcontext()
to use get_flags() and then just remove this user_disable_single_step().
And, the state of TIF_FORCED_TF can be wrong after restore_sigcontext()
which can set/clear TF, this needs another fix.

Reported-by: Evan Teran <eteran@alum.rit.edu>
Reported-by: Pedro Alves <palves@redhat.com>
Tested-By: Andres Freund <andres@anarazel.de>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/signal.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ed37a76..9d3a15b 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -629,7 +629,8 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
 static void
 handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 {
-	bool failed;
+	bool stepping, failed;
+
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* If so, check system call restarting.. */
@@ -653,12 +654,13 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	}
 
 	/*
-	 * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF
-	 * flag so that register information in the sigcontext is correct.
+	 * If TF is set due to a debugger (TIF_FORCED_TF), clear TF now
+	 * so that register information in the sigcontext is correct and
+	 * then notify the tracer before entering the signal handler.
 	 */
-	if (unlikely(regs->flags & X86_EFLAGS_TF) &&
-	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
-		regs->flags &= ~X86_EFLAGS_TF;
+	stepping = test_thread_flag(TIF_SINGLESTEP);
+	if (stepping)
+		user_disable_single_step(current);
 
 	failed = (setup_rt_frame(ksig, regs) < 0);
 	if (!failed) {
@@ -669,10 +671,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		 * it might disable possible debug exception from the
 		 * signal handler.
 		 *
-		 * Clear TF when entering the signal handler, but
-		 * notify any tracer that was single-stepping it.
-		 * The tracer may want to single-step inside the
-		 * handler too.
+		 * Clear TF for the case when it wasn't set by debugger to
+		 * avoid the recursive send_sigtrap() in SIGTRAP handler.
 		 */
 		regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF);
 		/*
@@ -681,7 +681,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		if (used_math())
 			drop_init_fpu(current);
 	}
-	signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP));
+	signal_setup_done(failed, ksig, stepping);
 }
 
 #ifdef CONFIG_X86_32
-- 
1.5.5.1