Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken
On Mon, Feb 1, 2021 at 12:40 PM Andy Lutomirski 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]
Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken
On Sun, Jan 31, 2021 at 3:35 PM Linus Torvalds
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
Re: [REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken
On Sun, Jan 31, 2021 at 3:39 PM Kyle Huey wrote: > > On Sun, Jan 31, 2021 at 3:36 PM Andy Lutomirski 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
On Sun, Jan 31, 2021 at 3:36 PM Andy Lutomirski 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
> On Jan 31, 2021, at 2:57 PM, Linus Torvalds
> wrote:
>
> On Sun, Jan 31, 2021 at 2:20 PM Andy Lutomirski 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
On Sun, Jan 31, 2021 at 3:18 PM Kyle Huey 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
On Sun, Jan 31, 2021 at 2:27 PM Kyle Huey wrote:
>
> On Sun, Jan 31, 2021 at 2:20 PM Andy Lutomirski wrote:
> >
> >
> >
> > > On Jan 31, 2021, at 2:08 PM, Kyle Huey wrote:
> > >
> > > On Sun, Jan 31, 2021 at 2:04 PM Andy Lutomirski
> > > 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
#include
#include
#include
#include
#include
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, ®s) == 0);
previous_rip = regs.rip;
regs.rip = 0xdeadbeef;
assert(ptrace(PTRACE_SETREGS, child, NULL, ®s) == 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, ®s) == 0);
regs.rip = previous_rip;
assert(ptrace(PTRACE_SETREGS, child, NULL, ®s) == 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
On Sun, Jan 31, 2021 at 2:20 PM Andy Lutomirski 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
On Sun, Jan 31, 2021 at 2:20 PM Andy Lutomirski wrote: > > > > > On Jan 31, 2021, at 2:08 PM, Kyle Huey wrote: > > > > On Sun, Jan 31, 2021 at 2:04 PM Andy Lutomirski > > 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
> On Jan 31, 2021, at 2:08 PM, Kyle Huey wrote: > > On Sun, Jan 31, 2021 at 2:04 PM Andy Lutomirski 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
On Sun, Jan 31, 2021 at 2:04 PM Andy Lutomirski 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
> On Jan 31, 2021, at 1:30 PM, Linus Torvalds > 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
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
On Sun, Jan 31, 2021 at 12:20 PM Gabriel Krisman Bertazi
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 writes: > On Sun, Jan 31, 2021 at 10:54 AM Yuxuan Shui 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
On Sun, Jan 31, 2021 at 10:54 AM Yuxuan Shui 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
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
On Sat, Jan 30, 2021 at 5:56 PM Linus Torvalds wrote: > > On Sat, Jan 30, 2021 at 5:32 PM Kyle Huey 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
On Sat, Jan 30, 2021 at 5:32 PM Kyle Huey 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
[REGRESSION] x86/entry: TIF_SINGLESTEP handling is still broken
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
