[REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Kyle Huey]

Yuxuan Shui previous reported a regression in single step reporting,
introduced in 64eb35f701f04b30706e21d1b02636b5d31a37d2, with a patch
to fix it.

However, after that is fixed, there is another regression introduced
later in the same series, in 2991552447707d791d9d81a5dc161f9e9e90b163,
that again breaks the same code.

The patch renames ARCH_SYSCALL_EXIT_WORK to ARCH_SYSCALL_WORK_EXIT,
which orphans the definition of ARCH_SYSCALL_EXIT_WORK in
arch/x86/include/asm/entry-common.h. No work was done to port
TIF_SINGLESTEP to syscall_work. Despite the code in report_single_step
that checks current_thread_info()->flags, because the code is no
longer checking the TIF values at all to decide whether to enter
syscall_exit_work, report_single_step will never be called and we will
again fail to report the single step.

I tested that with 2991552447707d791d9d81a5dc161f9e9e90b163 reverted
and Yuxuan's patch applied to Linus's tip rr works and passes all
tests.

- Kyle

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Linus Torvalds]

On Sat, Jan 30, 2021 at 5:32 PM Kyle Huey <me@kylehuey.com> wrote:
>
> I tested that with 2991552447707d791d9d81a5dc161f9e9e90b163 reverted
> and Yuxuan's patch applied to Linus's tip rr works and passes all
> tests.

There's a patch in the -tip tree that hasn't been merged yet:

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git core/urgent

(there's only that one patch in that branch right now, commit ID
41c1a06d1d1544bed9692ba72a5692454eee1945).

It should be making its way my direction any day now, but in the
meantime if you can verify that it makes things work for you, that
would be great..

            Linus

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Kyle Huey]

On Sat, Jan 30, 2021 at 5:56 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Jan 30, 2021 at 5:32 PM Kyle Huey <me@kylehuey.com> wrote:
> >
> > I tested that with 2991552447707d791d9d81a5dc161f9e9e90b163 reverted
> > and Yuxuan's patch applied to Linus's tip rr works and passes all
> > tests.
>
> There's a patch in the -tip tree that hasn't been merged yet:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git core/urgent
>
> (there's only that one patch in that branch right now, commit ID
> 41c1a06d1d1544bed9692ba72a5692454eee1945).
>
> It should be making its way my direction any day now, but in the
> meantime if you can verify that it makes things work for you, that
> would be great..

Right, I'm saying that that is not sufficient.

41c1a06d1d1544bed9692ba72a5692454eee1945 does indeed fix the bug
introduced in 64eb35f701f04b30706e21d1b02636b5d31a37d2, but
2991552447707d791d9d81a5dc161f9e9e90b163 introduced another bug that
remains unfixed.

- Kyle

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Yuxuan Shui]

I didn't understand Kyle's point at first, so I asked for clarification
and will record my understanding below for posterity.

ARCH_SYSCALL_EXIT_WORK was a flag that was checked by various functions
(via SYSCALL_EXIT_WORK) before calling syscall_exit_work, which is what
reports single steps. This flag was supposed to be overridden by
architecture specific definitions. And indeed, x86 overrides it, to
TIF_SINGLESTEP.

However, commit 2991552447707d791d9d81a5dc161f9e9e90b163 renamed
ARCH_SYSCALL_EXIT_WORK to ARCH_SYSCALL_WORK_EXIT, thus x86's definition
no longer override it.  Looks like there was an oversight the definition
in x86 wasn't updated.

But renaming the definition in x86 is not enough, as TIF_SINGLESTEP is
set in current_thread_info()->flags, and the same commit has removed the
code that checks those flags. We have to also migrate TIF_SINGLESTEP from
thread info flags to syscall work flags, to make the whole thing work again.

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Linus Torvalds]

On Sun, Jan 31, 2021 at 10:54 AM Yuxuan Shui <yshuiv7@gmail.com> wrote:
>
> But renaming the definition in x86 is not enough, as TIF_SINGLESTEP is
> set in current_thread_info()->flags, and the same commit has removed the
> code that checks those flags. We have to also migrate TIF_SINGLESTEP from
> thread info flags to syscall work flags, to make the whole thing work again.

Ok, so I now have the first fix merged, but what's the next step here?

As you say, the x86 ARCH_SYSCALL_EXIT_WORK is now entirely unused.

It's called ARCH_SYSCALL_WORK_EXIT these days, but that's for the
SYSCALL_WORK_SYSCALL_xyz flags, not for the TIF_xyz ones.

Revert? Or does somebody have a fix patch?

            Linus

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Gabriel Krisman Bertazi]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, Jan 31, 2021 at 10:54 AM Yuxuan Shui <yshuiv7@gmail.com> wrote:
>>
>> But renaming the definition in x86 is not enough, as TIF_SINGLESTEP is
>> set in current_thread_info()->flags, and the same commit has removed the
>> code that checks those flags. We have to also migrate TIF_SINGLESTEP from
>> thread info flags to syscall work flags, to make the whole thing work again.
>
> Ok, so I now have the first fix merged, but what's the next step here?
>
> As you say, the x86 ARCH_SYSCALL_EXIT_WORK is now entirely unused.
>
> It's called ARCH_SYSCALL_WORK_EXIT these days, but that's for the
> SYSCALL_WORK_SYSCALL_xyz flags, not for the TIF_xyz ones.
>
> Revert? Or does somebody have a fix patch?

I think we should migrate TIF_SINGLESTEP to a SYSCALL_WORK flag as that
is just a simple refactor. I can get a patch to you and Thomas during
the first part of the week, for the next -rc. I will also review the x86
version of ARCH_SYSCALL_EXIT WORK to make sure i didn't miss anything
else.

Reverting would be slightly be annoying as it requires reverting syscall
user dispatch as well.

-- 
Gabriel Krisman Bertazi

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Linus Torvalds]

On Sun, Jan 31, 2021 at 12:20 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
>
> I think we should migrate TIF_SINGLESTEP to a SYSCALL_WORK flag as that
> is just a simple refactor.

That makes no sense at all to me.

A single-step has absolutely nothing to do with system calls, and it's
also not what any other architecture is doing.

So honestly, something is wrong if TIF_SINGLESTEP should be moved to
the SYSCALL_WORK_SYSCALL_xyz flags. That's not what single-stepping is
at all about.

It looks like commit 299155244770 ("entry: Drop usage of TIF flags in
the generic syscall code") is simply wrong. The whole premise for it
was wrong because it didn't notice that TIF_SINGLESTEP isn't a syscall
flag - and shouldn't be one.

The problem seems to be that TIF_SINGLESTEP has two different
meanings: the usual "enable single stepping", *and* then a special
magic hack to also do it at system call exit. You only seem to have
looked at the magic hack, not the actual REAL meaning of
TIF_SINGLESTEP, which causes TF to be set and the debug fault.

This whole code is very confusing. What is that
"arch_syscall_exit_tracehook()" thing in the first place? I see an
arch wrapper, but I don't see any architecture that actually defines
it, so it seems to always become just tracehook_report_syscall_exit().

It then does (on x86) the generic version, which does

        if (step)
                user_single_step_report(regs);
        else
                ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_EXIT);

where that user_single_step_report() is a nonsensical special case
that just does what a debug fault does for all the *usual* single step
events in exc_debug_user() (which does not use that pointless
"user_single_step_report()" function, but just does

                send_sigtrap(regs, 0, get_si_code(dr6));

so please, let's take a step back here, and look at the basics:

 - the *main* user of TIF_SINGLESTEP is regular debugging that causes
a #DB after each user space instruction, and causes that
send_sigtrap() in exc_debug_user()

 - there is one very odd and weird special case that is about having
system call tracing enabled, which seems to have unclear semantics and
this is the case that got broken.

What's the fix here? Because no, single-stepping isn't about system
calls, and we should not try to change the code to be about system
calls, because no other architecture does that EITHER.

Now, it's possible that eventually

 (a) the normal TF flag setting doesn't actually need TIF_SINGESTEP at all

 (b) the TIF_SINGLESTEP bit really only needs to be about the odd
special case for system calls.

but as things are now, (a) is *NOT* true. We have very magical
behavior wrt TIF_SINGLESTEP, see enable_single_step(), and the comment
about

        /*
         * If TF was already set, check whether it was us who set it.
         * If not, we should never attempt a block step.
         */

which actually ends up depending very subtly on whether TIF_SINGLESTEP
was already set *before* we did that enable_single_step(), and/or
whether TF was already set on the stack (because users can set it
themselves without ptrace).

Again, the special system call case is the special case, NOT the
normal case. Don't try to make TIF_SINGLESTEP be about the special
case.

              Linus

             Linus

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Linus Torvalds]

Btw Kyle, do you have a good simple test-case for this? Clearly this
is some weird special case use, and regular single-stepping works
fine.

             Linus

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Andy Lutomirski]

> On Jan 31, 2021, at 1:30 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> Btw Kyle, do you have a good simple test-case for this? Clearly this
> is some weird special case use, and regular single-stepping works
> fine.
> 
> 

Indeed, and I have tests for this.

TBH, the TIF_SINGLESTEP code makes no sense, and I think it has at least three overloaded meanings. I can try to look in a bit.  I’ve mostly avoided breaking it in the past, but that doesn’t mean I understand the original intent.

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Kyle Huey]

On Sun, Jan 31, 2021 at 2:04 PM Andy Lutomirski <luto@amacapital.net> wrote:
> Indeed, and I have tests for this.

Do you mean you already have a test case or that you would like a
minimized test case?

- Kyle

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Andy Lutomirski]

> On Jan 31, 2021, at 2:08 PM, Kyle Huey <me@kylehuey.com> wrote:
> 
> On Sun, Jan 31, 2021 at 2:04 PM Andy Lutomirski <luto@amacapital.net> wrote:
>> Indeed, and I have tests for this.
> 
> Do you mean you already have a test case or that you would like a
> minimized test case?

A smallish test that we could stick in selftests would be great if that’s straightforward.

> 
> - Kyle

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Kyle Huey]

On Sun, Jan 31, 2021 at 2:20 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>
> > On Jan 31, 2021, at 2:08 PM, Kyle Huey <me@kylehuey.com> wrote:
> >
> > On Sun, Jan 31, 2021 at 2:04 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >> Indeed, and I have tests for this.
> >
> > Do you mean you already have a test case or that you would like a
> > minimized test case?
>
> A smallish test that we could stick in selftests would be great if that’s straightforward.

I'll look into it.

- Kyle

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Kyle Huey]

On Sun, Jan 31, 2021 at 2:27 PM Kyle Huey <me@kylehuey.com> wrote:
>
> On Sun, Jan 31, 2021 at 2:20 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >
> >
> > > On Jan 31, 2021, at 2:08 PM, Kyle Huey <me@kylehuey.com> wrote:
> > >
> > > On Sun, Jan 31, 2021 at 2:04 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > >> Indeed, and I have tests for this.
> > >
> > > Do you mean you already have a test case or that you would like a
> > > minimized test case?
> >
> > A smallish test that we could stick in selftests would be great if that’s straightforward.
>
> I'll look into it.
>
> - Kyle

A minimal test case follows.

The key to triggering this bug is to enter a ptrace syscall stop and
then use PTRACE_SINGLESTEP to exit it. On a good kernel this will not
result in any userspace code execution in the tracee because on the
way out of the kernel's syscall handling path the singlestep trap will
be raised immediately. On a bad kernel that stop will not be raised,
and in the example below, the program will crash.

- Kyle

---

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

void do_child() {
  /* Synchronize with the parent */
  kill(getpid(), SIGSTOP);
  /* Do a syscall */
  printf("child is alive\n");
  /* Return and exit */
}

int main() {
  pid_t child = -1;
  int status = 0;
  unsigned long long previous_rip = 0;
  struct user_regs_struct regs;

  if ((child = fork()) == 0) {
      do_child();
      return 0;
  }

  /* Adds 0x80 to syscall stops so we can see them easily */
  intptr_t options = PTRACE_O_TRACESYSGOOD;
  /* Take control of the child (which should be waiting */
  assert(ptrace(PTRACE_SEIZE, child, NULL, options) == 0);
  assert(waitpid(child, &status, 0) == child);
  assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGSTOP);

  /* Advance to the syscall stop for the write underlying
   * the child's printf.
   */
  assert(ptrace(PTRACE_SYSCALL, child, NULL, 0) == 0);
  assert(waitpid(child, &status, 0) == child);
  /* Should be a syscall stop */
  assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP | 0x80);

  /* Mess with the child's registers, so it will crash if
   * it executes any code
   */
  assert(ptrace(PTRACE_GETREGS, child, NULL, &regs) == 0);
  previous_rip = regs.rip;
  regs.rip = 0xdeadbeef;
  assert(ptrace(PTRACE_SETREGS, child, NULL, &regs) == 0);
  /* Singlestep. This should trap without executing any code */
  assert(ptrace(PTRACE_SINGLESTEP, child, NULL, 0) == 0);
  assert(waitpid(child, &status, 0) == child);
  /* Should be at a singlestep SIGTRAP. In a buggy kernel,
   * the SIGTRAP is skipped, execution resumes, and we
   * get a SIGSEGV at the invalid address.
   */
  assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);

  /* Restore registers */
  assert(ptrace(PTRACE_GETREGS, child, NULL, &regs) == 0);
  regs.rip = previous_rip;
  assert(ptrace(PTRACE_SETREGS, child, NULL, &regs) == 0);

  /* Continue to the end of the program */
  assert(ptrace(PTRACE_CONT, child, NULL, 0) == 0);
  assert(waitpid(child, &status, 0) == child);
  /* Verify the child exited cleanly */
  assert(WIFEXITED(status) && WEXITSTATUS(status) == 0);

  printf("SUCCESS\n");

  return 0;
}

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Linus Torvalds]

On Sun, Jan 31, 2021 at 3:18 PM Kyle Huey <me@kylehuey.com> wrote:
>
> The key to triggering this bug is to enter a ptrace syscall stop and
> then use PTRACE_SINGLESTEP to exit it. On a good kernel this will not
> result in any userspace code execution in the tracee because on the
> way out of the kernel's syscall handling path the singlestep trap will
> be raised immediately. On a bad kernel that stop will not be raised,
> and in the example below, the program will crash.

Thanks, great explanation, and I can certainly see the behavior you mention.

I wonder if the simple solution is to just

 (a) always set one of the SYSCALL_WORK_EXIT bits on the child in
ptrace (exactly to catch the child on system call exit)

 (b) basically revert 299155244770 ("entry: Drop usage of TIF flags in
the generic syscall code") and have the syscall exit code check the
TIF_SINGLESTEP flag

Hmm?

        Linus

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Linus Torvalds]

On Sun, Jan 31, 2021 at 3:35 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I wonder if the simple solution is to just
>
>  (a) always set one of the SYSCALL_WORK_EXIT bits on the child in
> ptrace (exactly to catch the child on system call exit)
>
>  (b) basically revert 299155244770 ("entry: Drop usage of TIF flags in
> the generic syscall code") and have the syscall exit code check the
> TIF_SINGLESTEP flag

Actually, (b) looks unnecessary - as long as we get to
syscall_exit_work(), the current code will work fine.

So maybe just add a dummy SYSCALL_WORK_SYSCALL_EXIT_TRAP, and set that
flag whenever a singestep is requested for a process that is currently
in a system call?

IOW, make it a very explicit "do TF for system calls", rather than the
old code that was doing so implicitly and not very obviously. Hmm?

                    Linus

[PATCH] entry: Fix missed trap after single-step on system call return

[Gabriel Krisman Bertazi]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, Jan 31, 2021 at 3:35 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I wonder if the simple solution is to just
>>
>>  (a) always set one of the SYSCALL_WORK_EXIT bits on the child in
>> ptrace (exactly to catch the child on system call exit)
>>
>>  (b) basically revert 299155244770 ("entry: Drop usage of TIF flags in
>> the generic syscall code") and have the syscall exit code check the
>> TIF_SINGLESTEP flag
>
> Actually, (b) looks unnecessary - as long as we get to
> syscall_exit_work(), the current code will work fine.
>
> So maybe just add a dummy SYSCALL_WORK_SYSCALL_EXIT_TRAP, and set that
> flag whenever a singestep is requested for a process that is currently
> in a system call?
>
> IOW, make it a very explicit "do TF for system calls", rather than the
> old code that was doing so implicitly and not very obviously. Hmm?

Linus,

Does the patch below follows your suggestion?  I'm setting the
SYSCALL_WORK shadowing TIF_SINGLESTEP every time, instead of only when
the child is inside a system call.  Is this acceptable?

This seems to pass Kyle's test case.  Kyle, can you verify it works with
rr?

I can also turn Kyle's test case into a selftest, if it is ok with him.

Thanks,

-- >8 --
Subject: [PATCH] entry: Fix missed trap after single-step on a system call return

Commit 299155244770 ("entry: Drop usage of TIF flags in the generic
syscall code") introduces a bug on architectures using the generic
syscall entry code, in which processes stopped by PTRACE_SYSCALL do not
trap on syscall return after receiving a TIF_SINGLESTEP. The reason is
the meaning of TIF_SINGLESTEP flag is overloaded to cause the trap after
a system call is executed, but since the above commit, the syscall call
handler only checks for the SYSCALL_WORK flags on the exit work.

This patch splits the meaning of TIF_SINGLESTEP such that it only means
single-step mode, and creates a new type of SYSCALL_WORK to request a
trap immediately after a syscall in single-step mode.  In the current
implementation, the SYSCALL_WORK flag shadows the TIF_SINGLESTEP flag
for simplicity.

Since x86 is the only code already using the generic syscall handling,
this also updates that architecture to flip this bit when a tracer
enables single step.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: 299155244770 ("entry: Drop usage of TIF flags in the generic syscall code")
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 arch/x86/include/asm/entry-common.h |  2 --
 arch/x86/kernel/step.c              | 10 ++++++++--
 include/linux/entry-common.h        |  1 +
 include/linux/thread_info.h         |  2 ++
 kernel/entry/common.c               | 12 ++----------
 5 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/entry-common.h b/arch/x86/include/asm/entry-common.h
index 6fe54b2813c1..2b87b191b3b8 100644
--- a/arch/x86/include/asm/entry-common.h
+++ b/arch/x86/include/asm/entry-common.h
@@ -43,8 +43,6 @@ static __always_inline void arch_check_user_regs(struct pt_regs *regs)
 }
 #define arch_check_user_regs arch_check_user_regs
 
-#define ARCH_SYSCALL_EXIT_WORK		(_TIF_SINGLESTEP)
-
 static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
 						  unsigned long ti_work)
 {
diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index 60d2c3798ba2..de975b62f10a 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -127,12 +127,17 @@ static int enable_single_step(struct task_struct *child)
 		regs->flags |= X86_EFLAGS_TF;
 
 	/*
-	 * Always set TIF_SINGLESTEP - this guarantees that
-	 * we single-step system calls etc..  This will also
+	 * Always set TIF_SINGLESTEP.  This will also
 	 * cause us to set TF when returning to user mode.
 	 */
 	set_tsk_thread_flag(child, TIF_SINGLESTEP);
 
+	/*
+	 * Trigger a trap is triggered once stepping out of a system
+	 * call prior to executing any user instruction.
+	 */
+	set_task_syscall_work(child, SYSCALL_EXIT_TRAP);
+
 	oflags = regs->flags;
 
 	/* Set TF on the kernel stack.. */
@@ -230,6 +235,7 @@ void user_disable_single_step(struct task_struct *child)
 
 	/* Always clear TIF_SINGLESTEP... */
 	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
+	clear_task_syscall_work(child, SYSCALL_EXIT_TRAP);
 
 	/* But touch TF only if it was set by us.. */
 	if (test_and_clear_tsk_thread_flag(child, TIF_FORCED_TF))
diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
index ca86a00abe86..a104b298019a 100644
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -46,6 +46,7 @@
 				 SYSCALL_WORK_SYSCALL_TRACE |		\
 				 SYSCALL_WORK_SYSCALL_AUDIT |		\
 				 SYSCALL_WORK_SYSCALL_USER_DISPATCH |	\
+				 SYSCALL_WORK_SYSCALL_EXIT_TRAP	|	\
 				 ARCH_SYSCALL_WORK_EXIT)
 
 /*
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index c8a974cead73..9b2158c69275 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -43,6 +43,7 @@ enum syscall_work_bit {
 	SYSCALL_WORK_BIT_SYSCALL_EMU,
 	SYSCALL_WORK_BIT_SYSCALL_AUDIT,
 	SYSCALL_WORK_BIT_SYSCALL_USER_DISPATCH,
+	SYSCALL_WORK_BIT_SYSCALL_EXIT_TRAP,
 };
 
 #define SYSCALL_WORK_SECCOMP		BIT(SYSCALL_WORK_BIT_SECCOMP)
@@ -51,6 +52,7 @@ enum syscall_work_bit {
 #define SYSCALL_WORK_SYSCALL_EMU	BIT(SYSCALL_WORK_BIT_SYSCALL_EMU)
 #define SYSCALL_WORK_SYSCALL_AUDIT	BIT(SYSCALL_WORK_BIT_SYSCALL_AUDIT)
 #define SYSCALL_WORK_SYSCALL_USER_DISPATCH BIT(SYSCALL_WORK_BIT_SYSCALL_USER_DISPATCH)
+#define SYSCALL_WORK_SYSCALL_EXIT_TRAP	BIT(SYSCALL_WORK_BIT_SYSCALL_EXIT_TRAP)
 #endif
 
 #include <asm/thread_info.h>
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 6dd82be60df8..f9d491b17b78 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -209,15 +209,9 @@ static void exit_to_user_mode_prepare(struct pt_regs *regs)
 	lockdep_sys_exit();
 }
 
-#ifndef _TIF_SINGLESTEP
-static inline bool report_single_step(unsigned long work)
-{
-	return false;
-}
-#else
 /*
  * If SYSCALL_EMU is set, then the only reason to report is when
- * TIF_SINGLESTEP is set (i.e. PTRACE_SYSEMU_SINGLESTEP).  This syscall
+ * SINGLESTEP is set (i.e. PTRACE_SYSEMU_SINGLESTEP).  This syscall
  * instruction has been already reported in syscall_enter_from_user_mode().
  */
 static inline bool report_single_step(unsigned long work)
@@ -225,10 +219,8 @@ static inline bool report_single_step(unsigned long work)
 	if (work & SYSCALL_WORK_SYSCALL_EMU)
 		return false;
 
-	return !!(current_thread_info()->flags & _TIF_SINGLESTEP);
+	return work & SYSCALL_WORK_SYSCALL_EXIT_TRAP;
 }
-#endif
-
 
 static void syscall_exit_work(struct pt_regs *regs, unsigned long work)
 {
-- 
2.30.0

Re: [PATCH] entry: Fix missed trap after single-step on system call return

[Linus Torvalds]

On Wed, Feb 3, 2021 at 10:00 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Does the patch below follows your suggestion?  I'm setting the
> SYSCALL_WORK shadowing TIF_SINGLESTEP every time, instead of only when
> the child is inside a system call.  Is this acceptable?

Looks sane to me.

My main worry would be about "what about the next system call"? It's
not what Kyle's case cares about, but let me just give an example:

 - task A traces task B, and starts single-stepping. Task B was *not*
in a system call at this point.

 - task B happily executes one instruction at a time, takes a TF
fault, everything is good

 - task B now does a system call. That will disable single-stepping
while in the kernel

 - task B returns from the system call. TF will be set in eflags, but
the first instruction *after* the system call will execute unless we
go through the system call exit path

So I think the tracer basically misses one instruction when single-stepping.

I think your patch works for this case (because the SYSCALL_EXIT_TRAP
flag stays set until single-stepping is done), so I think it's all
good. But can you verify, just to allay my worry?

         Linus

Re: [PATCH] entry: Fix missed trap after single-step on system call return

[Andy Lutomirski]

On Wed, Feb 3, 2021 at 10:10 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Feb 3, 2021 at 10:00 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
> >
> > Does the patch below follows your suggestion?  I'm setting the
> > SYSCALL_WORK shadowing TIF_SINGLESTEP every time, instead of only when
> > the child is inside a system call.  Is this acceptable?
>
> Looks sane to me.
>
> My main worry would be about "what about the next system call"? It's
> not what Kyle's case cares about, but let me just give an example:
>
>  - task A traces task B, and starts single-stepping. Task B was *not*
> in a system call at this point.
>
>  - task B happily executes one instruction at a time, takes a TF
> fault, everything is good
>
>  - task B now does a system call. That will disable single-stepping
> while in the kernel
>
>  - task B returns from the system call. TF will be set in eflags, but
> the first instruction *after* the system call will execute unless we
> go through the system call exit path
>
> So I think the tracer basically misses one instruction when single-stepping.

I was hoping you wouldn't ask this :)

The x86 architecture is fundamentally a bit busted here.  If we return
from a system call with SYSRET and TF is set in R11, then SYSRET
traps, which means that #DB is delivered before executing a user
instruction.  I have been asking Intel for quite a while to document
this, and they said they did, but I still can't find it.  IRET is the
opposite: if we return from a system call with IRET and TF is set on
the stack, we execute one user instruction and then trap.

So if we want to reliably single-step a system call and trap after the
system call, we just need to synthesize a trap on the way out.  Doing
this and getting all the nasty corners (e.g. sigreturn setting TF,
sigreturn *clearing* TF, signal delivery as part of the syscall,
ptrace mucking with TF) etc right might be nontrivial.

I suspect the behavior back in the bad old asm-entry-path days was at
best inconsistent.

--Andy

Re: [PATCH] entry: Fix missed trap after single-step on system call return

[Linus Torvalds]

On Wed, Feb 3, 2021 at 10:18 AM Andy Lutomirski <luto@kernel.org> wrote:
>
> So if we want to reliably single-step a system call and trap after the
> system call, we just need to synthesize a trap on the way out.

Well, I think Gabriel's patch does exactly that, due to how
SYSCALL_EXIT_TRAP is set. It looks like subsequent system calls will
work exactly the way the concurrent system call case (that Kyle's test
did) does.

So it all _looks_ sane to me, but this is one of those "it needs testing".

               Linus

Re: [PATCH] entry: Fix missed trap after single-step on system call return

[Kyle Huey]

On Wed, Feb 3, 2021 at 10:00 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Sun, Jan 31, 2021 at 3:35 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >>
> >> I wonder if the simple solution is to just
> >>
> >>  (a) always set one of the SYSCALL_WORK_EXIT bits on the child in
> >> ptrace (exactly to catch the child on system call exit)
> >>
> >>  (b) basically revert 299155244770 ("entry: Drop usage of TIF flags in
> >> the generic syscall code") and have the syscall exit code check the
> >> TIF_SINGLESTEP flag
> >
> > Actually, (b) looks unnecessary - as long as we get to
> > syscall_exit_work(), the current code will work fine.
> >
> > So maybe just add a dummy SYSCALL_WORK_SYSCALL_EXIT_TRAP, and set that
> > flag whenever a singestep is requested for a process that is currently
> > in a system call?
> >
> > IOW, make it a very explicit "do TF for system calls", rather than the
> > old code that was doing so implicitly and not very obviously. Hmm?
>
> Linus,
>
> Does the patch below follows your suggestion?  I'm setting the
> SYSCALL_WORK shadowing TIF_SINGLESTEP every time, instead of only when
> the child is inside a system call.  Is this acceptable?
>
> This seems to pass Kyle's test case.  Kyle, can you verify it works with
> rr?

I will test it later today.

> I can also turn Kyle's test case into a selftest, if it is ok with him.

Sure. Consider whatever license/copyright/etc you need granted.

- Kyle

> Thanks,
>
> -- >8 --
> Subject: [PATCH] entry: Fix missed trap after single-step on a system call return
>
> Commit 299155244770 ("entry: Drop usage of TIF flags in the generic
> syscall code") introduces a bug on architectures using the generic
> syscall entry code, in which processes stopped by PTRACE_SYSCALL do not
> trap on syscall return after receiving a TIF_SINGLESTEP. The reason is
> the meaning of TIF_SINGLESTEP flag is overloaded to cause the trap after
> a system call is executed, but since the above commit, the syscall call
> handler only checks for the SYSCALL_WORK flags on the exit work.
>
> This patch splits the meaning of TIF_SINGLESTEP such that it only means
> single-step mode, and creates a new type of SYSCALL_WORK to request a
> trap immediately after a syscall in single-step mode.  In the current
> implementation, the SYSCALL_WORK flag shadows the TIF_SINGLESTEP flag
> for simplicity.
>
> Since x86 is the only code already using the generic syscall handling,
> this also updates that architecture to flip this bit when a tracer
> enables single step.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Fixes: 299155244770 ("entry: Drop usage of TIF flags in the generic syscall code")
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  arch/x86/include/asm/entry-common.h |  2 --
>  arch/x86/kernel/step.c              | 10 ++++++++--
>  include/linux/entry-common.h        |  1 +
>  include/linux/thread_info.h         |  2 ++
>  kernel/entry/common.c               | 12 ++----------
>  5 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/include/asm/entry-common.h b/arch/x86/include/asm/entry-common.h
> index 6fe54b2813c1..2b87b191b3b8 100644
> --- a/arch/x86/include/asm/entry-common.h
> +++ b/arch/x86/include/asm/entry-common.h
> @@ -43,8 +43,6 @@ static __always_inline void arch_check_user_regs(struct pt_regs *regs)
>  }
>  #define arch_check_user_regs arch_check_user_regs
>
> -#define ARCH_SYSCALL_EXIT_WORK         (_TIF_SINGLESTEP)
> -
>  static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
>                                                   unsigned long ti_work)
>  {
> diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
> index 60d2c3798ba2..de975b62f10a 100644
> --- a/arch/x86/kernel/step.c
> +++ b/arch/x86/kernel/step.c
> @@ -127,12 +127,17 @@ static int enable_single_step(struct task_struct *child)
>                 regs->flags |= X86_EFLAGS_TF;
>
>         /*
> -        * Always set TIF_SINGLESTEP - this guarantees that
> -        * we single-step system calls etc..  This will also
> +        * Always set TIF_SINGLESTEP.  This will also
>          * cause us to set TF when returning to user mode.
>          */
>         set_tsk_thread_flag(child, TIF_SINGLESTEP);
>
> +       /*
> +        * Trigger a trap is triggered once stepping out of a system
> +        * call prior to executing any user instruction.
> +        */
> +       set_task_syscall_work(child, SYSCALL_EXIT_TRAP);
> +
>         oflags = regs->flags;
>
>         /* Set TF on the kernel stack.. */
> @@ -230,6 +235,7 @@ void user_disable_single_step(struct task_struct *child)
>
>         /* Always clear TIF_SINGLESTEP... */
>         clear_tsk_thread_flag(child, TIF_SINGLESTEP);
> +       clear_task_syscall_work(child, SYSCALL_EXIT_TRAP);
>
>         /* But touch TF only if it was set by us.. */
>         if (test_and_clear_tsk_thread_flag(child, TIF_FORCED_TF))
> diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
> index ca86a00abe86..a104b298019a 100644
> --- a/include/linux/entry-common.h
> +++ b/include/linux/entry-common.h
> @@ -46,6 +46,7 @@
>                                  SYSCALL_WORK_SYSCALL_TRACE |           \
>                                  SYSCALL_WORK_SYSCALL_AUDIT |           \
>                                  SYSCALL_WORK_SYSCALL_USER_DISPATCH |   \
> +                                SYSCALL_WORK_SYSCALL_EXIT_TRAP |       \
>                                  ARCH_SYSCALL_WORK_EXIT)
>
>  /*
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index c8a974cead73..9b2158c69275 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -43,6 +43,7 @@ enum syscall_work_bit {
>         SYSCALL_WORK_BIT_SYSCALL_EMU,
>         SYSCALL_WORK_BIT_SYSCALL_AUDIT,
>         SYSCALL_WORK_BIT_SYSCALL_USER_DISPATCH,
> +       SYSCALL_WORK_BIT_SYSCALL_EXIT_TRAP,
>  };
>
>  #define SYSCALL_WORK_SECCOMP           BIT(SYSCALL_WORK_BIT_SECCOMP)
> @@ -51,6 +52,7 @@ enum syscall_work_bit {
>  #define SYSCALL_WORK_SYSCALL_EMU       BIT(SYSCALL_WORK_BIT_SYSCALL_EMU)
>  #define SYSCALL_WORK_SYSCALL_AUDIT     BIT(SYSCALL_WORK_BIT_SYSCALL_AUDIT)
>  #define SYSCALL_WORK_SYSCALL_USER_DISPATCH BIT(SYSCALL_WORK_BIT_SYSCALL_USER_DISPATCH)
> +#define SYSCALL_WORK_SYSCALL_EXIT_TRAP BIT(SYSCALL_WORK_BIT_SYSCALL_EXIT_TRAP)
>  #endif
>
>  #include <asm/thread_info.h>
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index 6dd82be60df8..f9d491b17b78 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -209,15 +209,9 @@ static void exit_to_user_mode_prepare(struct pt_regs *regs)
>         lockdep_sys_exit();
>  }
>
> -#ifndef _TIF_SINGLESTEP
> -static inline bool report_single_step(unsigned long work)
> -{
> -       return false;
> -}
> -#else
>  /*
>   * If SYSCALL_EMU is set, then the only reason to report is when
> - * TIF_SINGLESTEP is set (i.e. PTRACE_SYSEMU_SINGLESTEP).  This syscall
> + * SINGLESTEP is set (i.e. PTRACE_SYSEMU_SINGLESTEP).  This syscall
>   * instruction has been already reported in syscall_enter_from_user_mode().
>   */
>  static inline bool report_single_step(unsigned long work)
> @@ -225,10 +219,8 @@ static inline bool report_single_step(unsigned long work)
>         if (work & SYSCALL_WORK_SYSCALL_EMU)
>                 return false;
>
> -       return !!(current_thread_info()->flags & _TIF_SINGLESTEP);
> +       return work & SYSCALL_WORK_SYSCALL_EXIT_TRAP;
>  }
> -#endif
> -
>
>  static void syscall_exit_work(struct pt_regs *regs, unsigned long work)
>  {
> --
> 2.30.0
>

Re: [PATCH] entry: Fix missed trap after single-step on system call return

[Kyle Huey]

On Wed, Feb 3, 2021 at 10:11 AM Kyle Huey <me@kylehuey.com> wrote:
>
> On Wed, Feb 3, 2021 at 10:00 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
> > This seems to pass Kyle's test case.  Kyle, can you verify it works with
> > rr?
>
> I will test it later today.

I have verified that a) the test case I sent earlier passes now and b)
all rr tests pass now.

Tested-by: Kyle Huey <me@kylehuey.com>

- Kyle

Re: [PATCH] entry: Fix missed trap after single-step on system call return

[Linus Torvalds]

On Wed, Feb 3, 2021 at 3:55 PM Kyle Huey <me@kylehuey.com> wrote:
>
> I have verified that a) the test case I sent earlier passes now and b)
> all rr tests pass now.

Thanks for keeping on top of this.

Thomas/Andy - the patch looks straightforward and obvious enough, and
I don't see any issues with it, so I assume I'll get it through the
normal channels and will archive this whole discussion.

No huge hurry, as long as it hits 5.11 final so that we don't end up
with a regression.

Thanks,
               Linus

[tip: core/urgent] entry: Ensure trap after single-step on system call return

[tip-bot2 for Gabriel Krisman Bertazi]

The following commit has been merged into the core/urgent branch of tip:

Commit-ID:     6342adcaa683c2b705c24ed201dc11b35854c88d
Gitweb:        https://git.kernel.org/tip/6342adcaa683c2b705c24ed201dc11b35854c88d
Author:        Gabriel Krisman Bertazi <krisman@collabora.com>
AuthorDate:    Wed, 03 Feb 2021 13:00:48 -05:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 06 Feb 2021 00:21:42 +01:00

entry: Ensure trap after single-step on system call return

Commit 299155244770 ("entry: Drop usage of TIF flags in the generic syscall
code") introduced a bug on architectures using the generic syscall entry
code, in which processes stopped by PTRACE_SYSCALL do not trap on syscall
return after receiving a TIF_SINGLESTEP.

The reason is that the meaning of TIF_SINGLESTEP flag is overloaded to
cause the trap after a system call is executed, but since the above commit,
the syscall call handler only checks for the SYSCALL_WORK flags on the exit
work.

Split the meaning of TIF_SINGLESTEP such that it only means single-step
mode, and create a new type of SYSCALL_WORK to request a trap immediately
after a syscall in single-step mode.  In the current implementation, the
SYSCALL_WORK flag shadows the TIF_SINGLESTEP flag for simplicity.

Update x86 to flip this bit when a tracer enables single stepping.

Fixes: 299155244770 ("entry: Drop usage of TIF flags in the generic syscall code")
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Kyle Huey <me@kylehuey.com>
Link: https://lore.kernel.org/r/87h7mtc9pr.fsf_-_@collabora.com

---
 arch/x86/include/asm/entry-common.h |  2 --
 arch/x86/kernel/step.c              | 10 ++++++++--
 include/linux/entry-common.h        |  1 +
 include/linux/thread_info.h         |  2 ++
 kernel/entry/common.c               | 12 ++----------
 5 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/entry-common.h b/arch/x86/include/asm/entry-common.h
index 6fe54b2..2b87b19 100644
--- a/arch/x86/include/asm/entry-common.h
+++ b/arch/x86/include/asm/entry-common.h
@@ -43,8 +43,6 @@ static __always_inline void arch_check_user_regs(struct pt_regs *regs)
 }
 #define arch_check_user_regs arch_check_user_regs
 
-#define ARCH_SYSCALL_EXIT_WORK		(_TIF_SINGLESTEP)
-
 static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
 						  unsigned long ti_work)
 {
diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index 60d2c37..0f3c307 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -127,12 +127,17 @@ static int enable_single_step(struct task_struct *child)
 		regs->flags |= X86_EFLAGS_TF;
 
 	/*
-	 * Always set TIF_SINGLESTEP - this guarantees that
-	 * we single-step system calls etc..  This will also
+	 * Always set TIF_SINGLESTEP.  This will also
 	 * cause us to set TF when returning to user mode.
 	 */
 	set_tsk_thread_flag(child, TIF_SINGLESTEP);
 
+	/*
+	 * Ensure that a trap is triggered once stepping out of a system
+	 * call prior to executing any user instruction.
+	 */
+	set_task_syscall_work(child, SYSCALL_EXIT_TRAP);
+
 	oflags = regs->flags;
 
 	/* Set TF on the kernel stack.. */
@@ -230,6 +235,7 @@ void user_disable_single_step(struct task_struct *child)
 
 	/* Always clear TIF_SINGLESTEP... */
 	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
+	clear_task_syscall_work(child, SYSCALL_EXIT_TRAP);
 
 	/* But touch TF only if it was set by us.. */
 	if (test_and_clear_tsk_thread_flag(child, TIF_FORCED_TF))
diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
index ca86a00..a104b29 100644
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -46,6 +46,7 @@
 				 SYSCALL_WORK_SYSCALL_TRACE |		\
 				 SYSCALL_WORK_SYSCALL_AUDIT |		\
 				 SYSCALL_WORK_SYSCALL_USER_DISPATCH |	\
+				 SYSCALL_WORK_SYSCALL_EXIT_TRAP	|	\
 				 ARCH_SYSCALL_WORK_EXIT)
 
 /*
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index c8a974c..9b2158c 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -43,6 +43,7 @@ enum syscall_work_bit {
 	SYSCALL_WORK_BIT_SYSCALL_EMU,
 	SYSCALL_WORK_BIT_SYSCALL_AUDIT,
 	SYSCALL_WORK_BIT_SYSCALL_USER_DISPATCH,
+	SYSCALL_WORK_BIT_SYSCALL_EXIT_TRAP,
 };
 
 #define SYSCALL_WORK_SECCOMP		BIT(SYSCALL_WORK_BIT_SECCOMP)
@@ -51,6 +52,7 @@ enum syscall_work_bit {
 #define SYSCALL_WORK_SYSCALL_EMU	BIT(SYSCALL_WORK_BIT_SYSCALL_EMU)
 #define SYSCALL_WORK_SYSCALL_AUDIT	BIT(SYSCALL_WORK_BIT_SYSCALL_AUDIT)
 #define SYSCALL_WORK_SYSCALL_USER_DISPATCH BIT(SYSCALL_WORK_BIT_SYSCALL_USER_DISPATCH)
+#define SYSCALL_WORK_SYSCALL_EXIT_TRAP	BIT(SYSCALL_WORK_BIT_SYSCALL_EXIT_TRAP)
 #endif
 
 #include <asm/thread_info.h>
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 6dd82be..f9d491b 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -209,15 +209,9 @@ static void exit_to_user_mode_prepare(struct pt_regs *regs)
 	lockdep_sys_exit();
 }
 
-#ifndef _TIF_SINGLESTEP
-static inline bool report_single_step(unsigned long work)
-{
-	return false;
-}
-#else
 /*
  * If SYSCALL_EMU is set, then the only reason to report is when
- * TIF_SINGLESTEP is set (i.e. PTRACE_SYSEMU_SINGLESTEP).  This syscall
+ * SINGLESTEP is set (i.e. PTRACE_SYSEMU_SINGLESTEP).  This syscall
  * instruction has been already reported in syscall_enter_from_user_mode().
  */
 static inline bool report_single_step(unsigned long work)
@@ -225,10 +219,8 @@ static inline bool report_single_step(unsigned long work)
 	if (work & SYSCALL_WORK_SYSCALL_EMU)
 		return false;
 
-	return !!(current_thread_info()->flags & _TIF_SINGLESTEP);
+	return work & SYSCALL_WORK_SYSCALL_EXIT_TRAP;
 }
-#endif
-
 
 static void syscall_exit_work(struct pt_regs *regs, unsigned long work)
 {

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Linus Torvalds]

On Sun, Jan 31, 2021 at 2:20 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> A smallish test that we could stick in selftests would be great if that’s straightforward.

Side note: it would be good to have a test-case for the interaction
with the "block step" code too.

I hate our name for it ("block step"?), but it modifies TF, to only
trap on taken branches, not after each instruction.

So there's all these things that interact:

 - you can set TF yourself in user space with 'popf' and get a debug
signal (not after the popf, but after the instruction _after_ popf,
iirc)

 - you can set TF as a debugger and that's basically what
TIF_SINGLESTEP fundamentally means

 - we have TIF_FORCED_TF, which says whether TF was set by ptrace, or
whether it was already set independently by the application before
ptrace, so that we can know whether to clear it or keep it set *after*
the single-step.

 - you can then *modify* the behavior of TF to trap only on control
flow changes (and we use TIF_BLOCKSTEP to specify that behavior)

 - and there's also obviously some very subtle and unclear rules about
when system call instructions cause debug traps

The basic TF behavior is fairly simple: it caused #DB, and we send a signal.

The "app set TF _itself_, and we want to debug across that event" is a
lot more interesting, but it's unclear whether anybody does it. It's
really just a "we want to be able to debug that case too", and
TIF_FORCED_TF means that it should be possible.

I didn't test that it works, though. Sounds worth a test-case?

The TIF_BLOCKSTEP thing changes no other logic, but basically sets the
bit in the MSR that modifies just when TF traps. I may hate the name,
but I think it works.

The odd system call tracing part I have no idea who depends on it
(apparently "rr", which I assume is some replay thing), and I suspect
our semantics for it has been basically random historical one, and
it's apparently what changed.

That's the one that we _really_ should have a test-case for, along
with some documentation and code comment what the actual semantics
need to be so that we don't break it again.

             Linus

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Andy Lutomirski]

> On Jan 31, 2021, at 2:57 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Sun, Jan 31, 2021 at 2:20 PM Andy Lutomirski <luto@amacapital.net> wrote:
>> 
>> A smallish test that we could stick in selftests would be great if that’s straightforward.
> 
> Side note: it would be good to have a test-case for the interaction
> with the "block step" code too.
> 
> I hate our name for it ("block step"?), but it modifies TF, to only
> trap on taken branches, not after each instruction.
> 
> So there's all these things that interact:
> 
> - you can set TF yourself in user space with 'popf' and get a debug
> signal (not after the popf, but after the instruction _after_ popf,
> iirc)
> 
> - you can set TF as a debugger and that's basically what
> TIF_SINGLESTEP fundamentally means
> 
> - we have TIF_FORCED_TF, which says whether TF was set by ptrace, or
> whether it was already set independently by the application before
> ptrace, so that we can know whether to clear it or keep it set *after*
> the single-step.
> 
> - you can then *modify* the behavior of TF to trap only on control
> flow changes (and we use TIF_BLOCKSTEP to specify that behavior)
> 
> - and there's also obviously some very subtle and unclear rules about
> when system call instructions cause debug traps
> 
> The basic TF behavior is fairly simple: it caused #DB, and we send a signal.

This is all fundamentally impossible to do fully correctly because a program can use PUSHF to read TF, and there is only one TF bit, and the app and the debugger can fight over it. The insn breakpoint mechanism is much better, but even AMD CPUs can’t (I think) be programmed to breakpoint the entire user address space. So we do our best to fudge it.

> 
> The "app set TF _itself_, and we want to debug across that event" is a
> lot more interesting, but it's unclear whether anybody does it. It's
> really just a "we want to be able to debug that case too", and
> TIF_FORCED_TF means that it should be possible.
> 
> I didn't test that it works, though. Sounds worth a test-case?
> 

I can look. We do have tests for apps setting TF with no debugger attached.

> The TIF_BLOCKSTEP thing changes no other logic, but basically sets the
> bit in the MSR that modifies just when TF traps. I may hate the name,
> but I think it works.
> 

It has certainly been busted in the past in corner cases. I don’t think we have tests.

> The odd system call tracing part I have no idea who depends on it
> (apparently "rr", which I assume is some replay thing), and I suspect
> our semantics for it has been basically random historical one, and
> it's apparently what changed.
> 
> That's the one that we _really_ should have a test-case for, along
> with some documentation and code comment what the actual semantics
> need to be so that we don't break it again.

This rr thing may be tangled up with the nonsense semantics of SYSRET.  I’ll muck around with Kyle’s test and try to figure out what broke.

I’m guessing the issue is that we are correctly setting TF in the EFLAGS image, but IRET helpfully only traps after the first user insn executes, which isn’t what the tracer is expects. 

> 
>             Linus

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Kyle Huey]

On Sun, Jan 31, 2021 at 3:36 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > The odd system call tracing part I have no idea who depends on it
> > (apparently "rr", which I assume is some replay thing), and I suspect
> > our semantics for it has been basically random historical one, and
> > it's apparently what changed.
> >
> > That's the one that we _really_ should have a test-case for, along
> > with some documentation and code comment what the actual semantics
> > need to be so that we don't break it again.
>
> This rr thing may be tangled up with the nonsense semantics of SYSRET.  I’ll muck around with Kyle’s test and try to figure out what broke.
>
> I’m guessing the issue is that we are correctly setting TF in the EFLAGS image, but IRET helpfully only traps after the first user insn executes, which isn’t what the tracer is expects.

The state of TF shouldn't really matter here. There should be no user
space code execution in the example I gave. This behavior all happens
in the kernel and not on the silicon.

- Kyle

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Andy Lutomirski]

On Sun, Jan 31, 2021 at 3:39 PM Kyle Huey <me@kylehuey.com> wrote:
>
> On Sun, Jan 31, 2021 at 3:36 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > > The odd system call tracing part I have no idea who depends on it
> > > (apparently "rr", which I assume is some replay thing), and I suspect
> > > our semantics for it has been basically random historical one, and
> > > it's apparently what changed.
> > >
> > > That's the one that we _really_ should have a test-case for, along
> > > with some documentation and code comment what the actual semantics
> > > need to be so that we don't break it again.
> >
> > This rr thing may be tangled up with the nonsense semantics of SYSRET.  I’ll muck around with Kyle’s test and try to figure out what broke.
> >
> > I’m guessing the issue is that we are correctly setting TF in the EFLAGS image, but IRET helpfully only traps after the first user insn executes, which isn’t what the tracer is expects.
>
> The state of TF shouldn't really matter here. There should be no user
> space code execution in the example I gave. This behavior all happens
> in the kernel and not on the silicon.
>

I admit that PTRACE_SINGLESTEP seems like an odd way to spell "advance
to the end of the syscall", but you're right, it should work.

Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken

[Robert O'Callahan]

On Mon, Feb 1, 2021 at 12:40 PM Andy Lutomirski <luto@kernel.org> wrote:
> I admit that PTRACE_SINGLESTEP seems like an odd way to spell "advance
> to the end of the syscall", but you're right, it should work.

We don't know of any better way to advance to the end of the syscall
without executing any userspace instructions. We could set a
breakpoint at the syscall return address but weird edge cases
complicate that.

Rob
-- 
"He was pierced for our transgressions, he was crushed for our
iniquities; the punishment that brought us peace was upon him, and by
his wounds we are healed. We all, like sheep, have gone astray, each
of us has turned to his own way; and the LORD has laid on him the
iniquity of us all." [Isaiah 53:5-6]