All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] riscv: return -ENOSYS for syscall -1
@ 2020-12-21 22:52 ` Andreas Schwab
  0 siblings, 0 replies; 12+ messages in thread
From: Andreas Schwab @ 2020-12-21 22:52 UTC (permalink / raw)
  To: linux-riscv
  Cc: linux-kernel, Palmer Dabbelt, Paul Walmsley, Tycho Andersen,
	David Abdurachmanov

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
 	/* Call syscall */
 	la s0, sys_call_table
 	slli t0, a7, RISCV_LGPTR
-- 
2.29.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] riscv: return -ENOSYS for syscall -1
@ 2020-12-21 22:52 ` Andreas Schwab
  0 siblings, 0 replies; 12+ messages in thread
From: Andreas Schwab @ 2020-12-21 22:52 UTC (permalink / raw)
  To: linux-riscv
  Cc: David Abdurachmanov, Tycho Andersen, Palmer Dabbelt,
	linux-kernel, Paul Walmsley

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
 	/* Call syscall */
 	la s0, sys_call_table
 	slli t0, a7, RISCV_LGPTR
-- 
2.29.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
  2020-12-21 22:52 ` Andreas Schwab
@ 2020-12-22 16:22   ` Tycho Andersen
  -1 siblings, 0 replies; 12+ messages in thread
From: Tycho Andersen @ 2020-12-22 16:22 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: linux-riscv, linux-kernel, Palmer Dabbelt, Paul Walmsley,
	David Abdurachmanov

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
@ 2020-12-22 16:22   ` Tycho Andersen
  0 siblings, 0 replies; 12+ messages in thread
From: Tycho Andersen @ 2020-12-22 16:22 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: David Abdurachmanov, linux-riscv, Palmer Dabbelt, linux-kernel,
	Paul Walmsley

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>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
  2020-12-22 16:22   ` Tycho Andersen
@ 2020-12-23  4:04     ` Palmer Dabbelt
  -1 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2020-12-23  4:04 UTC (permalink / raw)
  To: tycho
  Cc: schwab, linux-riscv, linux-kernel, Paul Walmsley, david.abdurachmanov

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
@ 2020-12-23  4:04     ` Palmer Dabbelt
  0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2020-12-23  4:04 UTC (permalink / raw)
  To: tycho
  Cc: david.abdurachmanov, schwab, linux-riscv, linux-kernel, Paul Walmsley

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
  2020-12-22 16:22   ` Tycho Andersen
@ 2020-12-23  8:24     ` Christoph Hellwig
  -1 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2020-12-23  8:24 UTC (permalink / raw)
  To: Tycho Andersen
  Cc: Andreas Schwab, David Abdurachmanov, linux-riscv, Palmer Dabbelt,
	linux-kernel, Paul Walmsley

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!).

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
@ 2020-12-23  8:24     ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2020-12-23  8:24 UTC (permalink / raw)
  To: Tycho Andersen
  Cc: David Abdurachmanov, Andreas Schwab, linux-kernel,
	Palmer Dabbelt, Paul Walmsley, linux-riscv

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!).

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
  2020-12-23  8:24     ` Christoph Hellwig
@ 2020-12-24  2:54       ` Palmer Dabbelt
  -1 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2020-12-24  2:54 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: tycho, schwab, david.abdurachmanov, linux-riscv, linux-kernel,
	Paul Walmsley

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
@ 2020-12-24  2:54       ` Palmer Dabbelt
  0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2020-12-24  2:54 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: david.abdurachmanov, schwab, linux-kernel, Paul Walmsley,
	linux-riscv, tycho

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
  2020-12-24  2:54       ` Palmer Dabbelt
@ 2020-12-24  4:38         ` Tycho Andersen
  -1 siblings, 0 replies; 12+ messages in thread
From: Tycho Andersen @ 2020-12-24  4:38 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Christoph Hellwig, schwab, david.abdurachmanov, linux-riscv,
	linux-kernel, Paul Walmsley

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: return -ENOSYS for syscall -1
@ 2020-12-24  4:38         ` Tycho Andersen
  0 siblings, 0 replies; 12+ messages in thread
From: Tycho Andersen @ 2020-12-24  4:38 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: david.abdurachmanov, schwab, linux-kernel, Christoph Hellwig,
	Paul Walmsley, linux-riscv

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-12-24  4:44 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-21 22:52 [PATCH] riscv: return -ENOSYS for syscall -1 Andreas Schwab
2020-12-21 22:52 ` Andreas Schwab
2020-12-22 16:22 ` Tycho Andersen
2020-12-22 16:22   ` Tycho Andersen
2020-12-23  4:04   ` Palmer Dabbelt
2020-12-23  4:04     ` Palmer Dabbelt
2020-12-23  8:24   ` Christoph Hellwig
2020-12-23  8:24     ` Christoph Hellwig
2020-12-24  2:54     ` Palmer Dabbelt
2020-12-24  2:54       ` Palmer Dabbelt
2020-12-24  4:38       ` Tycho Andersen
2020-12-24  4:38         ` Tycho Andersen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.