| Message ID | mvma6u6vkkv.fsf@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: return -ENOSYS for syscall -1 | expand |
On Mon, Dec 21, 2020 at 11:52:00PM +0100, Andreas Schwab wrote: > Properly return -ENOSYS for syscall -1 instead of leaving the return value > uninitialized. This fixes the strace teststuite. > > Fixes: 5340627e3fe0 ("riscv: add support for SECCOMP and SECCOMP_FILTER") > Signed-off-by: Andreas Schwab <schwab@suse.de> > --- > arch/riscv/kernel/entry.S | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > index 524d918f3601..d07763001eb0 100644 > --- a/arch/riscv/kernel/entry.S > +++ b/arch/riscv/kernel/entry.S > @@ -186,14 +186,7 @@ check_syscall_nr: > * Syscall number held in a7. > * If syscall number is above allowed value, redirect to ni_syscall. > */ > - bge a7, t0, 1f > - /* > - * Check if syscall is rejected by tracer, i.e., a7 == -1. > - * If yes, we pretend it was executed. > - */ > - li t1, -1 > - beq a7, t1, ret_from_syscall_rejected > - blt a7, t1, 1f > + bgeu a7, t0, 1f IIUC, this is all dead code anyway for the path where seccomp actually rejects the syscall, since it should do the rejection directly in handle_syscall_trace_enter(), which is called above this hunk. So it seems good to me. Reviewed-by: Tycho Andersen <tycho@tycho.pizza>
On Tue, 22 Dec 2020 08:22:19 PST (-0800), tycho@tycho.pizza wrote: > On Mon, Dec 21, 2020 at 11:52:00PM +0100, Andreas Schwab wrote: >> Properly return -ENOSYS for syscall -1 instead of leaving the return value >> uninitialized. This fixes the strace teststuite. >> >> Fixes: 5340627e3fe0 ("riscv: add support for SECCOMP and SECCOMP_FILTER") >> Signed-off-by: Andreas Schwab <schwab@suse.de> >> --- >> arch/riscv/kernel/entry.S | 9 +-------- >> 1 file changed, 1 insertion(+), 8 deletions(-) >> >> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S >> index 524d918f3601..d07763001eb0 100644 >> --- a/arch/riscv/kernel/entry.S >> +++ b/arch/riscv/kernel/entry.S >> @@ -186,14 +186,7 @@ check_syscall_nr: >> * Syscall number held in a7. >> * If syscall number is above allowed value, redirect to ni_syscall. >> */ >> - bge a7, t0, 1f >> - /* >> - * Check if syscall is rejected by tracer, i.e., a7 == -1. >> - * If yes, we pretend it was executed. >> - */ >> - li t1, -1 >> - beq a7, t1, ret_from_syscall_rejected >> - blt a7, t1, 1f >> + bgeu a7, t0, 1f > > IIUC, this is all dead code anyway for the path where seccomp actually > rejects the syscall, since it should do the rejection directly in > handle_syscall_trace_enter(), which is called above this hunk. So it > seems good to me. > > Reviewed-by: Tycho Andersen <tycho@tycho.pizza> Thanks, this is on fixes.
On Tue, Dec 22, 2020 at 09:22:19AM -0700, Tycho Andersen wrote: > On Mon, Dec 21, 2020 at 11:52:00PM +0100, Andreas Schwab wrote: > > Properly return -ENOSYS for syscall -1 instead of leaving the return value > > uninitialized. This fixes the strace teststuite. > > > > Fixes: 5340627e3fe0 ("riscv: add support for SECCOMP and SECCOMP_FILTER") > > Signed-off-by: Andreas Schwab <schwab@suse.de> > > --- > > arch/riscv/kernel/entry.S | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > > index 524d918f3601..d07763001eb0 100644 > > --- a/arch/riscv/kernel/entry.S > > +++ b/arch/riscv/kernel/entry.S > > @@ -186,14 +186,7 @@ check_syscall_nr: > > * Syscall number held in a7. > > * If syscall number is above allowed value, redirect to ni_syscall. > > */ > > - bge a7, t0, 1f > > - /* > > - * Check if syscall is rejected by tracer, i.e., a7 == -1. > > - * If yes, we pretend it was executed. > > - */ > > - li t1, -1 > > - beq a7, t1, ret_from_syscall_rejected > > - blt a7, t1, 1f > > + bgeu a7, t0, 1f > > IIUC, this is all dead code anyway for the path where seccomp actually > rejects the syscall, since it should do the rejection directly in > handle_syscall_trace_enter(), which is called above this hunk. So it > seems good to me. That change really needs to be documented in the commit log, or even better split into a separate patch (still documented of course!).
On Wed, 23 Dec 2020 00:24:04 PST (-0800), Christoph Hellwig wrote: > On Tue, Dec 22, 2020 at 09:22:19AM -0700, Tycho Andersen wrote: >> On Mon, Dec 21, 2020 at 11:52:00PM +0100, Andreas Schwab wrote: >> > Properly return -ENOSYS for syscall -1 instead of leaving the return value >> > uninitialized. This fixes the strace teststuite. >> > >> > Fixes: 5340627e3fe0 ("riscv: add support for SECCOMP and SECCOMP_FILTER") >> > Signed-off-by: Andreas Schwab <schwab@suse.de> >> > --- >> > arch/riscv/kernel/entry.S | 9 +-------- >> > 1 file changed, 1 insertion(+), 8 deletions(-) >> > >> > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S >> > index 524d918f3601..d07763001eb0 100644 >> > --- a/arch/riscv/kernel/entry.S >> > +++ b/arch/riscv/kernel/entry.S >> > @@ -186,14 +186,7 @@ check_syscall_nr: >> > * Syscall number held in a7. >> > * If syscall number is above allowed value, redirect to ni_syscall. >> > */ >> > - bge a7, t0, 1f >> > - /* >> > - * Check if syscall is rejected by tracer, i.e., a7 == -1. >> > - * If yes, we pretend it was executed. >> > - */ >> > - li t1, -1 >> > - beq a7, t1, ret_from_syscall_rejected >> > - blt a7, t1, 1f >> > + bgeu a7, t0, 1f >> >> IIUC, this is all dead code anyway for the path where seccomp actually >> rejects the syscall, since it should do the rejection directly in >> handle_syscall_trace_enter(), which is called above this hunk. So it >> seems good to me. > > That change really needs to be documented in the commit log, or even > better split into a separate patch (still documented of course!). Unless I'm missing something, this is already how it works already? handle_syscall_trace_enter is checking the result of do_syscall_trace_enter(), which checks secure_computing(). When secure_computing() rejects the syscall we already ended up rejecting the syscall, so this code wasn't doing anything for the case it was supposed to handle. It was, however, intercepting syscall number -1 when we weren't rejecting the syscall and directly exiting rather than calling sys_ni_syscall. That would, at a bare minimum, result in an uninitialized return value. It also breaks the pairing of trace_sys_enter() and trace_sys_exit(), which doesn't smell like a good idea.
On Wed, Dec 23, 2020 at 06:54:43PM -0800, Palmer Dabbelt wrote: > On Wed, 23 Dec 2020 00:24:04 PST (-0800), Christoph Hellwig wrote: > > On Tue, Dec 22, 2020 at 09:22:19AM -0700, Tycho Andersen wrote: > > > On Mon, Dec 21, 2020 at 11:52:00PM +0100, Andreas Schwab wrote: > > > > Properly return -ENOSYS for syscall -1 instead of leaving the return value > > > > uninitialized. This fixes the strace teststuite. > > > > > > > > Fixes: 5340627e3fe0 ("riscv: add support for SECCOMP and SECCOMP_FILTER") > > > > Signed-off-by: Andreas Schwab <schwab@suse.de> > > > > --- > > > > arch/riscv/kernel/entry.S | 9 +-------- > > > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > > > > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > > > > index 524d918f3601..d07763001eb0 100644 > > > > --- a/arch/riscv/kernel/entry.S > > > > +++ b/arch/riscv/kernel/entry.S > > > > @@ -186,14 +186,7 @@ check_syscall_nr: > > > > * Syscall number held in a7. > > > > * If syscall number is above allowed value, redirect to ni_syscall. > > > > */ > > > > - bge a7, t0, 1f > > > > - /* > > > > - * Check if syscall is rejected by tracer, i.e., a7 == -1. > > > > - * If yes, we pretend it was executed. > > > > - */ > > > > - li t1, -1 > > > > - beq a7, t1, ret_from_syscall_rejected > > > > - blt a7, t1, 1f > > > > + bgeu a7, t0, 1f > > > > > > IIUC, this is all dead code anyway for the path where seccomp actually > > > rejects the syscall, since it should do the rejection directly in > > > handle_syscall_trace_enter(), which is called above this hunk. So it > > > seems good to me. > > > > That change really needs to be documented in the commit log, or even > > better split into a separate patch (still documented of course!). > > Unless I'm missing something, this is already how it works already? Yes, agreed. My musing was mostly just that this is dead code that probably should have been in removed in af33d2433b03 ("riscv: fix seccomp reject syscall code path"), but was overlooked. Maybe this could use a Fixes: tag for that too. Tycho
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S index 524d918f3601..d07763001eb0 100644 --- a/arch/riscv/kernel/entry.S +++ b/arch/riscv/kernel/entry.S @@ -186,14 +186,7 @@ check_syscall_nr: * Syscall number held in a7. * If syscall number is above allowed value, redirect to ni_syscall. */ - bge a7, t0, 1f - /* - * Check if syscall is rejected by tracer, i.e., a7 == -1. - * If yes, we pretend it was executed. - */ - li t1, -1 - beq a7, t1, ret_from_syscall_rejected - blt a7, t1, 1f + bgeu a7, t0, 1f /* Call syscall */ la s0, sys_call_table slli t0, a7, RISCV_LGPTR
Properly return -ENOSYS for syscall -1 instead of leaving the return value uninitialized. This fixes the strace teststuite. Fixes: 5340627e3fe0 ("riscv: add support for SECCOMP and SECCOMP_FILTER") Signed-off-by: Andreas Schwab <schwab@suse.de> --- arch/riscv/kernel/entry.S | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)