diff mbox series

sh: fix syscall tracing

Message ID 20200903054803.GX3265@brightrain.aerifal.cx
State New
Headers show
Series sh: fix syscall tracing | expand

Commit Message

Rich Felker Sept. 3, 2020, 5:48 a.m. UTC
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(-)

Comments

John Paul Adrian Glaubitz Sept. 3, 2020, 8:13 a.m. UTC | #1
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
John Paul Adrian Glaubitz Sept. 3, 2020, 10:14 a.m. UTC | #2
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>
Rich Felker Sept. 3, 2020, 4:16 p.m. UTC | #3
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
John Paul Adrian Glaubitz Sept. 7, 2020, 9:52 a.m. UTC | #4
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
Rich Felker Sept. 7, 2020, 5:44 p.m. UTC | #5
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
John Paul Adrian Glaubitz Sept. 10, 2020, 9:55 a.m. UTC | #6
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
Rob Landley Sept. 10, 2020, 11:02 a.m. UTC | #7
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
Rich Felker Sept. 10, 2020, 1:37 p.m. UTC | #8
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
John Paul Adrian Glaubitz Sept. 15, 2020, 10:35 a.m. UTC | #9
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
Rich Felker Sept. 16, 2020, 12:28 a.m. UTC | #10
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
Rich Felker Sept. 18, 2020, 7:29 p.m. UTC | #11
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 mbox series

Patch

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)