| Message ID | 20200903054803.GX3265@brightrain.aerifal.cx (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sh: fix syscall tracing | expand |
Hi Rich! On 9/3/20 7:48 AM, Rich Felker wrote: > Addition of SECCOMP_FILTER exposed a longstanding bug in > do_syscall_trace_enter, whereby r0 (the 5th argument register) was > mistakenly used where r3 (syscall_nr) was intended. By overwriting r0 > rather than r3 with -1 when attempting to block a syscall, the > existing code would instead have caused the syscall to execute with an > argument clobbered. > > Commit 0bb605c2c7f2b4b3 then introduced skipping of the syscall when > do_syscall_trace_enter returns -1, so that the return value set by > seccomp filters would not be clobbered by -ENOSYS. This eliminated the > clobbering of the 5th argument register, but instead caused syscalls > made with a 5th argument of -1 to be misinterpreted as a request by > do_syscall_trace_enter to suppress the syscall. > > Fixes: 0bb605c2c7f2b4b3 ("sh: Add SECCOMP_FILTER") > Fixes: ab99c733ae73cce3 ("sh: Make syscall tracer use tracehook notifiers, add TIF_NOTIFY_RESUME.") > Signed-off-by: Rich Felker <dalias@libc.org> I'm testing this patch now with a rebased kernel and a rebased libseccomp with my patch for SuperH support on top. I'll report back later. Adrian
Hi Rich! On 9/3/20 7:48 AM, Rich Felker wrote: > Addition of SECCOMP_FILTER exposed a longstanding bug in > do_syscall_trace_enter, whereby r0 (the 5th argument register) was > mistakenly used where r3 (syscall_nr) was intended. By overwriting r0 > rather than r3 with -1 when attempting to block a syscall, the > existing code would instead have caused the syscall to execute with an > argument clobbered. > > Commit 0bb605c2c7f2b4b3 then introduced skipping of the syscall when > do_syscall_trace_enter returns -1, so that the return value set by > seccomp filters would not be clobbered by -ENOSYS. This eliminated the > clobbering of the 5th argument register, but instead caused syscalls > made with a 5th argument of -1 to be misinterpreted as a request by > do_syscall_trace_enter to suppress the syscall. > > Fixes: 0bb605c2c7f2b4b3 ("sh: Add SECCOMP_FILTER") > Fixes: ab99c733ae73cce3 ("sh: Make syscall tracer use tracehook notifiers, add TIF_NOTIFY_RESUME.") > Signed-off-by: Rich Felker <dalias@libc.org> > --- > arch/sh/kernel/entry-common.S | 1 - > arch/sh/kernel/ptrace_32.c | 15 +++++---------- > 2 files changed, 5 insertions(+), 11 deletions(-) > > diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S > index ad963104d22d..91ab2607a1ff 100644 > --- a/arch/sh/kernel/entry-common.S > +++ b/arch/sh/kernel/entry-common.S > @@ -370,7 +370,6 @@ syscall_trace_entry: > nop > cmp/eq #-1, r0 > bt syscall_exit > - mov.l r0, @(OFF_R0,r15) ! Save return value > ! Reload R0-R4 from kernel stack, where the > ! parent may have modified them using > ! ptrace(POKEUSR). (Note that R0-R2 are > diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c > index b05bf92f9c32..5281685f6ad1 100644 > --- a/arch/sh/kernel/ptrace_32.c > +++ b/arch/sh/kernel/ptrace_32.c > @@ -455,16 +455,11 @@ long arch_ptrace(struct task_struct *child, long request, > > asmlinkage long do_syscall_trace_enter(struct pt_regs *regs) > { > - long ret = 0; > - > if (test_thread_flag(TIF_SYSCALL_TRACE) && > - tracehook_report_syscall_entry(regs)) > - /* > - * Tracing decided this syscall should not happen. > - * We'll return a bogus call number to get an ENOSYS > - * error, but leave the original number in regs->regs[0]. > - */ > - ret = -1L; > + tracehook_report_syscall_entry(regs)) { > + regs->regs[0] = -ENOSYS; > + return -1; > + } > > if (secure_computing() == -1) > return -1; > @@ -475,7 +470,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs) > audit_syscall_entry(regs->regs[3], regs->regs[4], regs->regs[5], > regs->regs[6], regs->regs[7]); > > - return ret ?: regs->regs[0]; > + return 0; > } > > asmlinkage void do_syscall_trace_leave(struct pt_regs *regs) > I can confirm that this patch fixes both strace for me and does not break libseccomp, I have run the libseccomp testsuite with my patch for SuperH support applied on top of a rebased libseccomp with the 32-bit fixes. Attaching the testsuite log. Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
On Thu, Sep 03, 2020 at 12:14:43PM +0200, John Paul Adrian Glaubitz wrote: > Hi Rich! > > On 9/3/20 7:48 AM, Rich Felker wrote: > > Addition of SECCOMP_FILTER exposed a longstanding bug in > > do_syscall_trace_enter, whereby r0 (the 5th argument register) was > > mistakenly used where r3 (syscall_nr) was intended. By overwriting r0 > > rather than r3 with -1 when attempting to block a syscall, the > > existing code would instead have caused the syscall to execute with an > > argument clobbered. > > > > Commit 0bb605c2c7f2b4b3 then introduced skipping of the syscall when > > do_syscall_trace_enter returns -1, so that the return value set by > > seccomp filters would not be clobbered by -ENOSYS. This eliminated the > > clobbering of the 5th argument register, but instead caused syscalls > > made with a 5th argument of -1 to be misinterpreted as a request by > > do_syscall_trace_enter to suppress the syscall. > > > > Fixes: 0bb605c2c7f2b4b3 ("sh: Add SECCOMP_FILTER") > > Fixes: ab99c733ae73cce3 ("sh: Make syscall tracer use tracehook notifiers, add TIF_NOTIFY_RESUME.") > > Signed-off-by: Rich Felker <dalias@libc.org> > > --- > > arch/sh/kernel/entry-common.S | 1 - > > arch/sh/kernel/ptrace_32.c | 15 +++++---------- > > 2 files changed, 5 insertions(+), 11 deletions(-) > > > > diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S > > index ad963104d22d..91ab2607a1ff 100644 > > --- a/arch/sh/kernel/entry-common.S > > +++ b/arch/sh/kernel/entry-common.S > > @@ -370,7 +370,6 @@ syscall_trace_entry: > > nop > > cmp/eq #-1, r0 > > bt syscall_exit > > - mov.l r0, @(OFF_R0,r15) ! Save return value > > ! Reload R0-R4 from kernel stack, where the > > ! parent may have modified them using > > ! ptrace(POKEUSR). (Note that R0-R2 are > > diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c > > index b05bf92f9c32..5281685f6ad1 100644 > > --- a/arch/sh/kernel/ptrace_32.c > > +++ b/arch/sh/kernel/ptrace_32.c > > @@ -455,16 +455,11 @@ long arch_ptrace(struct task_struct *child, long request, > > > > asmlinkage long do_syscall_trace_enter(struct pt_regs *regs) > > { > > - long ret = 0; > > - > > if (test_thread_flag(TIF_SYSCALL_TRACE) && > > - tracehook_report_syscall_entry(regs)) > > - /* > > - * Tracing decided this syscall should not happen. > > - * We'll return a bogus call number to get an ENOSYS > > - * error, but leave the original number in regs->regs[0]. > > - */ > > - ret = -1L; > > + tracehook_report_syscall_entry(regs)) { > > + regs->regs[0] = -ENOSYS; > > + return -1; > > + } > > > > if (secure_computing() == -1) > > return -1; > > @@ -475,7 +470,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs) > > audit_syscall_entry(regs->regs[3], regs->regs[4], regs->regs[5], > > regs->regs[6], regs->regs[7]); > > > > - return ret ?: regs->regs[0]; > > + return 0; > > } > > > > asmlinkage void do_syscall_trace_leave(struct pt_regs *regs) > > > > I can confirm that this patch fixes both strace for me and does not break libseccomp, > I have run the libseccomp testsuite with my patch for SuperH support applied on top > of a rebased libseccomp with the 32-bit fixes. Attaching the testsuite log. > > Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> Great! Thanks! Rich
Hi Rich! On 9/3/20 6:16 PM, Rich Felker wrote: >> I can confirm that this patch fixes both strace for me and does not break libseccomp, >> I have run the libseccomp testsuite with my patch for SuperH support applied on top >> of a rebased libseccomp with the 32-bit fixes. Attaching the testsuite log. >> >> Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> > > Great! Thanks! Can we still get this merged as a hotfix for 5.9? Adrian
On Mon, Sep 07, 2020 at 11:52:20AM +0200, John Paul Adrian Glaubitz wrote: > Hi Rich! > > On 9/3/20 6:16 PM, Rich Felker wrote: > >> I can confirm that this patch fixes both strace for me and does not break libseccomp, > >> I have run the libseccomp testsuite with my patch for SuperH support applied on top > >> of a rebased libseccomp with the 32-bit fixes. Attaching the testsuite log. > >> > >> Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> > > > > Great! Thanks! > > Can we still get this merged as a hotfix for 5.9? Yes, fixes for regressions in the same release cycle are in-scope (the whole point of having -rc's). I have at least one other fix that needs to go in too and was just giving it a little time to make sure everything's ok now and that there are no more. Rich
Hi Rich! On 9/7/20 7:44 PM, Rich Felker wrote: >> Can we still get this merged as a hotfix for 5.9? > > Yes, fixes for regressions in the same release cycle are in-scope (the > whole point of having -rc's). I have at least one other fix that needs > to go in too and was just giving it a little time to make sure > everything's ok now and that there are no more. Let me know if there is anything else left for testing. Adrian
On 9/10/20 4:55 AM, John Paul Adrian Glaubitz wrote: > Hi Rich! > > On 9/7/20 7:44 PM, Rich Felker wrote: >>> Can we still get this merged as a hotfix for 5.9? >> >> Yes, fixes for regressions in the same release cycle are in-scope (the >> whole point of having -rc's). I have at least one other fix that needs >> to go in too and was just giving it a little time to make sure >> everything's ok now and that there are no more. > > Let me know if there is anything else left for testing. Could you also merge the fix the build break, ala: > The vmlinux image is a current vanilla Linux kernel using an initramfs filesystem: > > make ARCH=sh CROSS_COMPILE=sh2eb-linux-muslfdpic- j2_defconfig vmlinux > > And trying to do that in current git dies with: > > CC init/version.o > In file included from ./include/linux/spinlock.h:318, > from ./arch/sh/include/asm/smp.h:11, > from ./include/linux/smp.h:82, > from ./include/linux/lockdep.h:14, > from ./include/linux/rcupdate.h:29, > from ./include/linux/rculist.h:11, > from ./include/linux/pid.h:5, > from ./include/linux/sched.h:14, > from ./include/linux/utsname.h:6, > from init/version.c:14: > ./include/linux/spinlock_api_smp.h: In function '__raw_spin_trylock': > ./include/linux/spinlock_api_smp.h:90:3: error: implicit declaration of function > 'spin_acquire'; did you mean 'xchg_acquire'? [-Werror=implicit-function-declaration] > 90 | spin_acquire(&lock->dep_map, 0, 1, _RET_IP_); > | ^~~~~~~~~~~~ > | xchg_acquire > ./include/linux/spinlock_api_smp.h:90:21: error: 'raw_spinlock_t' {aka 'struct > raw_spinlock'} has no member named 'dep_map' > 90 | spin_acquire(&lock->dep_map, 0, 1, _RET_IP_); > | ^~ > > And so on and so forth for pages. I bisected it to: > > commit 0cd39f4600ed4de859383018eb10f0f724900e1b > Author: Peter Zijlstra <peterz@infradead.org> > Date: Thu Aug 6 14:35:11 2020 +0200 > > locking/seqlock, headers: Untangle the spaghetti monster Which I reported to Rich on the 2nd and he had me test a one line patch fixing it (adding an extra #include) on the 3rd, but I just did a fresh pull and the j2_defconfig build still broke a week later. Rob
On Thu, Sep 10, 2020 at 06:02:05AM -0500, Rob Landley wrote: > On 9/10/20 4:55 AM, John Paul Adrian Glaubitz wrote: > > Hi Rich! > > > > On 9/7/20 7:44 PM, Rich Felker wrote: > >>> Can we still get this merged as a hotfix for 5.9? > >> > >> Yes, fixes for regressions in the same release cycle are in-scope (the > >> whole point of having -rc's). I have at least one other fix that needs > >> to go in too and was just giving it a little time to make sure > >> everything's ok now and that there are no more. > > > > Let me know if there is anything else left for testing. > > Could you also merge the fix the build break, ala: > > > The vmlinux image is a current vanilla Linux kernel using an initramfs filesystem: > > > > make ARCH=sh CROSS_COMPILE=sh2eb-linux-muslfdpic- j2_defconfig vmlinux > > > > And trying to do that in current git dies with: > > > > CC init/version.o > > In file included from ./include/linux/spinlock.h:318, > > from ./arch/sh/include/asm/smp.h:11, > > from ./include/linux/smp.h:82, > > from ./include/linux/lockdep.h:14, > > from ./include/linux/rcupdate.h:29, > > from ./include/linux/rculist.h:11, > > from ./include/linux/pid.h:5, > > from ./include/linux/sched.h:14, > > from ./include/linux/utsname.h:6, > > from init/version.c:14: > > ./include/linux/spinlock_api_smp.h: In function '__raw_spin_trylock': > > ./include/linux/spinlock_api_smp.h:90:3: error: implicit declaration of function > > 'spin_acquire'; did you mean 'xchg_acquire'? [-Werror=implicit-function-declaration] > > 90 | spin_acquire(&lock->dep_map, 0, 1, _RET_IP_); > > | ^~~~~~~~~~~~ > > | xchg_acquire > > ./include/linux/spinlock_api_smp.h:90:21: error: 'raw_spinlock_t' {aka 'struct > > raw_spinlock'} has no member named 'dep_map' > > 90 | spin_acquire(&lock->dep_map, 0, 1, _RET_IP_); > > | ^~ > > > > And so on and so forth for pages. I bisected it to: > > > > commit 0cd39f4600ed4de859383018eb10f0f724900e1b > > Author: Peter Zijlstra <peterz@infradead.org> > > Date: Thu Aug 6 14:35:11 2020 +0200 > > > > locking/seqlock, headers: Untangle the spaghetti monster > > Which I reported to Rich on the 2nd and he had me test a one line patch fixing > it (adding an extra #include) on the 3rd, but I just did a fresh pull and the > j2_defconfig build still broke a week later. Yes, that's presently the other regression fix I have queued for the second pull request. Rich
Hi Rich! On 9/10/20 3:37 PM, Rich Felker wrote: >> Which I reported to Rich on the 2nd and he had me test a one line patch fixing >> it (adding an extra #include) on the 3rd, but I just did a fresh pull and the >> j2_defconfig build still broke a week later. > > Yes, that's presently the other regression fix I have queued for the > second pull request. Any news on this? Seems like Linus just tagged -rc5 this week. Adrian
On Tue, Sep 15, 2020 at 12:35:28PM +0200, John Paul Adrian Glaubitz wrote: > Hi Rich! > > On 9/10/20 3:37 PM, Rich Felker wrote: > >> Which I reported to Rich on the 2nd and he had me test a one line patch fixing > >> it (adding an extra #include) on the 3rd, but I just did a fresh pull and the > >> j2_defconfig build still broke a week later. > > > > Yes, that's presently the other regression fix I have queued for the > > second pull request. > > Any news on this? Seems like Linus just tagged -rc5 this week. I rebased against -rc5 and pushed for-next a couple days ago. It doesn't look like there are any problems so I'll proceed with the PR. Rich
On Tue, Sep 15, 2020 at 08:28:45PM -0400, Rich Felker wrote: > On Tue, Sep 15, 2020 at 12:35:28PM +0200, John Paul Adrian Glaubitz wrote: > > Hi Rich! > > > > On 9/10/20 3:37 PM, Rich Felker wrote: > > >> Which I reported to Rich on the 2nd and he had me test a one line patch fixing > > >> it (adding an extra #include) on the 3rd, but I just did a fresh pull and the > > >> j2_defconfig build still broke a week later. > > > > > > Yes, that's presently the other regression fix I have queued for the > > > second pull request. > > > > Any news on this? Seems like Linus just tagged -rc5 this week. > > I rebased against -rc5 and pushed for-next a couple days ago. It > doesn't look like there are any problems so I'll proceed with the PR. It's now been merged.
diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S index ad963104d22d..91ab2607a1ff 100644 --- a/arch/sh/kernel/entry-common.S +++ b/arch/sh/kernel/entry-common.S @@ -370,7 +370,6 @@ syscall_trace_entry: nop cmp/eq #-1, r0 bt syscall_exit - mov.l r0, @(OFF_R0,r15) ! Save return value ! Reload R0-R4 from kernel stack, where the ! parent may have modified them using ! ptrace(POKEUSR). (Note that R0-R2 are diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c index b05bf92f9c32..5281685f6ad1 100644 --- a/arch/sh/kernel/ptrace_32.c +++ b/arch/sh/kernel/ptrace_32.c @@ -455,16 +455,11 @@ long arch_ptrace(struct task_struct *child, long request, asmlinkage long do_syscall_trace_enter(struct pt_regs *regs) { - long ret = 0; - if (test_thread_flag(TIF_SYSCALL_TRACE) && - tracehook_report_syscall_entry(regs)) - /* - * Tracing decided this syscall should not happen. - * We'll return a bogus call number to get an ENOSYS - * error, but leave the original number in regs->regs[0]. - */ - ret = -1L; + tracehook_report_syscall_entry(regs)) { + regs->regs[0] = -ENOSYS; + return -1; + } if (secure_computing() == -1) return -1; @@ -475,7 +470,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs) audit_syscall_entry(regs->regs[3], regs->regs[4], regs->regs[5], regs->regs[6], regs->regs[7]); - return ret ?: regs->regs[0]; + return 0; } asmlinkage void do_syscall_trace_leave(struct pt_regs *regs)
Addition of SECCOMP_FILTER exposed a longstanding bug in do_syscall_trace_enter, whereby r0 (the 5th argument register) was mistakenly used where r3 (syscall_nr) was intended. By overwriting r0 rather than r3 with -1 when attempting to block a syscall, the existing code would instead have caused the syscall to execute with an argument clobbered. Commit 0bb605c2c7f2b4b3 then introduced skipping of the syscall when do_syscall_trace_enter returns -1, so that the return value set by seccomp filters would not be clobbered by -ENOSYS. This eliminated the clobbering of the 5th argument register, but instead caused syscalls made with a 5th argument of -1 to be misinterpreted as a request by do_syscall_trace_enter to suppress the syscall. Fixes: 0bb605c2c7f2b4b3 ("sh: Add SECCOMP_FILTER") Fixes: ab99c733ae73cce3 ("sh: Make syscall tracer use tracehook notifiers, add TIF_NOTIFY_RESUME.") Signed-off-by: Rich Felker <dalias@libc.org> --- arch/sh/kernel/entry-common.S | 1 - arch/sh/kernel/ptrace_32.c | 15 +++++---------- 2 files changed, 5 insertions(+), 11 deletions(-)