| Message ID | 20211111220429.797-1-svens@stackframe.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | parisc/entry: fix trace test in syscall exit path | expand |
On 2021-11-11 5:04 p.m., Sven Schnelle wrote: > commit 8779e05ba8aa ("parisc: Fix ptrace check on syscall return") > fixed testing of TI_FLAGS. This uncovered a bug in the test mask. > syscall_restore_rfi is only used when the kernel needs to exit to > usespace with single stepping via recovery counter enabled. The test > however used _TIF_SYSCALL_TRACE_MASK, which includes a lot of bits > that shouldn't be tested here. > > Fix this by using TIF_SINGLESTEP and TIF_BLOCKSTEP directly and > remove those bits from TIF_SYSCALL_TRACE_MASK. > > I encountered this bug by enabling syscall tracepoints. Both in qemu and > on real hardware. As soon as i enabled the tracepoint (sys_exit_read, > but i guess it doesn't really matter which one), i got random page > faults in userspace almost immediately. > > Signed-off-by: Sven Schnelle <svens@stackframe.org> > --- > arch/parisc/include/asm/thread_info.h | 3 +-- > arch/parisc/kernel/entry.S | 2 +- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/parisc/include/asm/thread_info.h b/arch/parisc/include/asm/thread_info.h > index 1a58795f785c..a3ba8c518292 100644 > --- a/arch/parisc/include/asm/thread_info.h > +++ b/arch/parisc/include/asm/thread_info.h > @@ -68,8 +68,7 @@ struct thread_info { > > #define _TIF_USER_WORK_MASK (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME | \ > _TIF_NEED_RESCHED | _TIF_NOTIFY_SIGNAL) > -#define _TIF_SYSCALL_TRACE_MASK (_TIF_SYSCALL_TRACE | _TIF_SINGLESTEP | \ > - _TIF_BLOCKSTEP | _TIF_SYSCALL_AUDIT | \ > +#define _TIF_SYSCALL_TRACE_MASK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ > _TIF_SECCOMP | _TIF_SYSCALL_TRACEPOINT) > > #ifdef CONFIG_64BIT > diff --git a/arch/parisc/kernel/entry.S b/arch/parisc/kernel/entry.S > index 57944d6f9ebb..88c188a965d8 100644 > --- a/arch/parisc/kernel/entry.S > +++ b/arch/parisc/kernel/entry.S > @@ -1805,7 +1805,7 @@ syscall_restore: > > /* Are we being ptraced? */ > LDREG TASK_TI_FLAGS(%r1),%r19 > - ldi _TIF_SYSCALL_TRACE_MASK,%r2 > + ldi _TIF_SINGLESTEP|_TIF_BLOCKSTEP,%r2 This change seems applied to the old code and not 8779e05ba8aa. Dave
On 2021-11-11 6:24 p.m., John David Anglin wrote: >> /* Are we being ptraced? */ >> LDREG TASK_TI_FLAGS(%r1),%r19 >> - ldi _TIF_SYSCALL_TRACE_MASK,%r2 >> + ldi _TIF_SINGLESTEP|_TIF_BLOCKSTEP,%r2 > This change seems applied to the old code and not 8779e05ba8aa. Forget this. TASK_TI_FLAGS was introduced in "parisc: Move thread_info into task struct". Dave
Sven Schnelle <svens@stackframe.org> writes: > commit 8779e05ba8aa ("parisc: Fix ptrace check on syscall return") > fixed testing of TI_FLAGS. This uncovered a bug in the test mask. > syscall_restore_rfi is only used when the kernel needs to exit to > usespace with single stepping via recovery counter enabled. The test > however used _TIF_SYSCALL_TRACE_MASK, which includes a lot of bits > that shouldn't be tested here. > > Fix this by using TIF_SINGLESTEP and TIF_BLOCKSTEP directly and > remove those bits from TIF_SYSCALL_TRACE_MASK. I think we need to have TIF_SINGLESTEP and TIF_BLOCKSTEP in TIF_SYSCALL_TRACE_MASK otherwise do_syscall_trace_exit() isn't called when leaving to userspace. I'll read the code a bit more during the weekend and prepare a v2.
On 2021-11-12 2:18 a.m., Sven Schnelle wrote: > Sven Schnelle <svens@stackframe.org> writes: > >> commit 8779e05ba8aa ("parisc: Fix ptrace check on syscall return") >> fixed testing of TI_FLAGS. This uncovered a bug in the test mask. >> syscall_restore_rfi is only used when the kernel needs to exit to >> usespace with single stepping via recovery counter enabled. The test >> however used _TIF_SYSCALL_TRACE_MASK, which includes a lot of bits >> that shouldn't be tested here. >> >> Fix this by using TIF_SINGLESTEP and TIF_BLOCKSTEP directly and >> remove those bits from TIF_SYSCALL_TRACE_MASK. > I think we need to have TIF_SINGLESTEP and TIF_BLOCKSTEP in > TIF_SYSCALL_TRACE_MASK otherwise do_syscall_trace_exit() isn't > called when leaving to userspace. I'll read the code a bit more > during the weekend and prepare a v2. Signal delivery is broken in 5.16.x. This causes a number of glibc and ada test regressions.
diff --git a/arch/parisc/include/asm/thread_info.h b/arch/parisc/include/asm/thread_info.h index 1a58795f785c..a3ba8c518292 100644 --- a/arch/parisc/include/asm/thread_info.h +++ b/arch/parisc/include/asm/thread_info.h @@ -68,8 +68,7 @@ struct thread_info { #define _TIF_USER_WORK_MASK (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME | \ _TIF_NEED_RESCHED | _TIF_NOTIFY_SIGNAL) -#define _TIF_SYSCALL_TRACE_MASK (_TIF_SYSCALL_TRACE | _TIF_SINGLESTEP | \ - _TIF_BLOCKSTEP | _TIF_SYSCALL_AUDIT | \ +#define _TIF_SYSCALL_TRACE_MASK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ _TIF_SECCOMP | _TIF_SYSCALL_TRACEPOINT) #ifdef CONFIG_64BIT diff --git a/arch/parisc/kernel/entry.S b/arch/parisc/kernel/entry.S index 57944d6f9ebb..88c188a965d8 100644 --- a/arch/parisc/kernel/entry.S +++ b/arch/parisc/kernel/entry.S @@ -1805,7 +1805,7 @@ syscall_restore: /* Are we being ptraced? */ LDREG TASK_TI_FLAGS(%r1),%r19 - ldi _TIF_SYSCALL_TRACE_MASK,%r2 + ldi _TIF_SINGLESTEP|_TIF_BLOCKSTEP,%r2 and,COND(=) %r19,%r2,%r0 b,n syscall_restore_rfi
commit 8779e05ba8aa ("parisc: Fix ptrace check on syscall return") fixed testing of TI_FLAGS. This uncovered a bug in the test mask. syscall_restore_rfi is only used when the kernel needs to exit to usespace with single stepping via recovery counter enabled. The test however used _TIF_SYSCALL_TRACE_MASK, which includes a lot of bits that shouldn't be tested here. Fix this by using TIF_SINGLESTEP and TIF_BLOCKSTEP directly and remove those bits from TIF_SYSCALL_TRACE_MASK. I encountered this bug by enabling syscall tracepoints. Both in qemu and on real hardware. As soon as i enabled the tracepoint (sys_exit_read, but i guess it doesn't really matter which one), i got random page faults in userspace almost immediately. Signed-off-by: Sven Schnelle <svens@stackframe.org> --- arch/parisc/include/asm/thread_info.h | 3 +-- arch/parisc/kernel/entry.S | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)