ptrace: copy_thread() should clear TIF_SINGLESTEP and X86_EFLAGS_TF

This patch fixes x86, other machines need the similar fix. Hopefully maintainers can help.

If the tracee calls fork() after PTRACE_SINGLESTEP, the forked child starts with TIF_SINGLESTEP/X86_EFLAGS_TF bits copied from ptraced parent. This is not right, especially when the new child is not auto-attaced: in this case it is killed by SIGTRAP.

Test-case:

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

	int main(void)
	{
		int pid, status;

		if (!(pid = fork())) {
			assert(ptrace(PTRACE_TRACEME) == 0);
			kill(getpid(), SIGSTOP);

			if (!fork()) {
				/* kernel bug: this child will be killed by SIGTRAP */
				printf("Hello world\n");
				return 43;
			}

			wait(&status);
			return WEXITSTATUS(status);
		}

		for (;;) {
			assert(pid == wait(&status));
			if (WIFEXITED(status))
				break;
			assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);
		}

		assert(WEXITSTATUS(status) == 43);
		return 0;
	}

Tested on x86_64, hopefully the change in process_32.c is right too.

---

 arch/x86/kernel/process_32.c |    3 +++
 arch/x86/kernel/process_64.c |    3 +++
 2 files changed, 6 insertions(+)

@@ -252,6 +252,8 @@ int copy_thread(unsigned long clone_flag
 	childregs->ax = 0;
 	childregs->sp = sp;
 
+	childregs->flags &= ~X86_EFLAGS_TF;
+
 	p->thread.sp = (unsigned long) childregs;
 	p->thread.sp0 = (unsigned long) (childregs+1);
 

@@ -287,6 +289,7 @@ int copy_thread(unsigned long clone_flag
 	clear_tsk_thread_flag(p, TIF_DS_AREA_MSR);
 	p->thread.ds_ctx = NULL;
 
+	clear_tsk_thread_flag(p, TIF_SINGLESTEP);
 	clear_tsk_thread_flag(p, TIF_DEBUGCTLMSR);
 	p->thread.debugctlmsr = 0;
 

@@ -289,6 +289,8 @@ int copy_thread(unsigned long clone_flag
 	if (sp == ~0UL)
 		childregs->sp = (unsigned long)childregs;
 
+	childregs->flags &= ~X86_EFLAGS_TF;
+
 	p->thread.sp = (unsigned long) childregs;
 	p->thread.sp0 = (unsigned long) (childregs+1);
 	p->thread.usersp = me->thread.usersp;

@@ -332,6 +334,7 @@ int copy_thread(unsigned long clone_flag
 	clear_tsk_thread_flag(p, TIF_DS_AREA_MSR);
 	p->thread.ds_ctx = NULL;
 
+	clear_tsk_thread_flag(p, TIF_SINGLESTEP);
 	clear_tsk_thread_flag(p, TIF_DEBUGCTLMSR);
 	p->thread.debugctlmsr = 0;
 

There is also the TIF_FORCED_TF logic to check. Probably it should just call user_disable_single_step() instead. Perhaps copy_process() should just call that and then no per-arch changes would be required.

Thanks,
Roland

I thought this flag has no effect without TIF_SINGLESTEP, and it is always updated by enable_single_step().

OK, thanks, will resend tomorrow.

user_disable_single_step() is very much arch-dependent, I am worried if it is safe to call it from copy_process(), when the new task_struct, thread_info, etc are not properly initialized yet...

Oleg.

Right, but to be anal about the semantics you shouldn't clear TF if it's not set, etc.

Well, arch people can weigh in about if it is problematical. Skimming over the existing arch definitions, it looks OK to me.

Thanks,
Roland

If the tracee calls fork() after PTRACE_SINGLESTEP, the forked child starts with TIF_SINGLESTEP/X86_EFLAGS_TF bits copied from ptraced parent. This is not right, especially when the new child is not auto-attaced: in this case it is killed by SIGTRAP.

Change copy_process() to call user_disable_single_step() if TIF_SINGLESTEP is set. Tested on x86, hopefully this is correct on any architecture.

Test-case:

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

	int main(void)
	{
		int pid, status;

		if (!(pid = fork())) {
			assert(ptrace(PTRACE_TRACEME) == 0);
			kill(getpid(), SIGSTOP);

			if (!fork()) {
				/* kernel bug: this child will be killed by SIGTRAP */
				printf("Hello world\n");
				return 43;
			}

			wait(&status);
			return WEXITSTATUS(status);
		}

		for (;;) {
			assert(pid == wait(&status));
			if (WIFEXITED(status))
				break;
			assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);
		}

		assert(WEXITSTATUS(status) == 43);
		return 0;
	}

---

 kernel/fork.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

@@ -1199,10 +1199,14 @@ static struct task_struct *copy_process(
 		p->sas_ss_sp = p->sas_ss_size = 0;
 
 	/*
-	 * Syscall tracing should be turned off in the child regardless
-	 * of CLONE_PTRACE.
+	 * Syscall tracing and stepping hould be turned off in the
+	 * child regardless of CLONE_PTRACE.
 	 */
 	clear_tsk_thread_flag(p, TIF_SYSCALL_TRACE);
+#ifdef TIF_SINGLESTEP
+	if (unlikely(test_tsk_thread_flag(p, TIF_SINGLESTEP)))
+		user_disable_single_step(p);
+#endif
 #ifdef TIF_SYSCALL_EMU
 	clear_tsk_thread_flag(p, TIF_SYSCALL_EMU);
 #endif

That is inappropriate use of arch details in generic code. It might happen to be harmless fritter in practice on the arch's we have but it is certainly not the correct way to go about things. You should just call user_disable_single_step() unconditionally. Even on an arch with no such machinery at all that should be defined safely as a no-op (see linux/ptrace.h). If there is some reason not to do that, please explain it.

Thanks,
Roland

Yes, we have arch_has_single_step.

I added test_tsk_thread_flag(TIF_SINGLESTEP) check for 2 reasons: to optimize out user_disable_single_step() in the likely case, and because I wasn't sure it is safe to call user_disable_single_step() unconditionally.

OK, will resend, thanks.

Oleg.

If the tracee calls fork() after PTRACE_SINGLESTEP, the forked child starts with TIF_SINGLESTEP/X86_EFLAGS_TF bits copied from ptraced parent. This is not right, especially when the new child is not auto-attaced: in this case it is killed by SIGTRAP.

Change copy_process() to call user_disable_single_step(). Tested on x86.

Test-case:

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

	int main(void)
	{
		int pid, status;

		if (!(pid = fork())) {
			assert(ptrace(PTRACE_TRACEME) == 0);
			kill(getpid(), SIGSTOP);

			if (!fork()) {
				/* kernel bug: this child will be killed by SIGTRAP */
				printf("Hello world\n");
				return 43;
			}

			wait(&status);
			return WEXITSTATUS(status);
		}

		for (;;) {
			assert(pid == wait(&status));
			if (WIFEXITED(status))
				break;
			assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);
		}

		assert(WEXITSTATUS(status) == 43);
		return 0;
	}

---

 kernel/fork.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

@@ -1199,9 +1199,10 @@ static struct task_struct *copy_process(
 		p->sas_ss_sp = p->sas_ss_size = 0;
 
 	/*
-	 * Syscall tracing should be turned off in the child regardless
-	 * of CLONE_PTRACE.
+	 * Syscall tracing and stepping should be turned off in the
+	 * child regardless of CLONE_PTRACE.
 	 */
+	user_disable_single_step(p);
 	clear_tsk_thread_flag(p, TIF_SYSCALL_TRACE);
 #ifdef TIF_SYSCALL_EMU
 	clear_tsk_thread_flag(p, TIF_SYSCALL_EMU);