Re: [RFC PATCH] s390/syscall: Sign extend system call number retrieved via syscall_get_nr()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] s390/syscall: syscall_get_nr() sign extension skips -ENOSYS in __do_syscall()
- [Medium] s390/syscall: inconsistent nr sign extension and return value in __do_syscall()
--

commit 6009c1b6249f84606771eedb6feddc0743454bc7
Author: Heiko Carstens <hca@linux.ibm.com>

s390/syscall: Sign extend system call number retrieved via syscall_get_nr()

This commit sign-extends the 16-bit system call number in syscall_get_nr() on
s390 so that negative system call numbers such as -1 are returned as -1
instead of 65535. It also removes the s390-specific 16-bit mask workaround in
the ptrace set_syscall_info selftest.

> diff --git a/arch/s390/include/asm/syscall.h b/arch/s390/include/asm/syscall.h
> index 5f310caad1fce..c67eeca3f33a1 100644
> --- a/arch/s390/include/asm/syscall.h
> +++ b/arch/s390/include/asm/syscall.h
> @@ -20,7 +20,7 @@ static inline long syscall_get_nr(struct task_struct *task,
>  				  struct pt_regs *regs)
>  {
>  	return test_pt_regs_flag(regs, PIF_SYSCALL) ?
> -		(regs->int_code & 0xffff) : -1;
> +		(s16)(regs->int_code & 0xffff) : -1;
>  }

[Severity: High]
When a system call is skipped under SECCOMP_RET_TRACE on s390, can this cause
__do_syscall() to return the wrong value to userspace instead of -ENOSYS?

Previously, syscall_get_nr() returned an unsigned 16-bit value (65535 when nr
was set to -1), so after PTRACE_EVENT_SECCOMP in
kernel/seccomp.c:__seccomp_filter():

	/* Check if the tracer forced the syscall to be skipped. */
	this_syscall = syscall_get_nr(current, current_pt_regs());
	if (this_syscall < 0)
		goto skip;

this check was not taken on s390. syscall_enter_from_user_mode_work()
returned permit = true with nr = 65535, and arch/s390/kernel/syscall.c:
__do_syscall() set regs->gprs[2] = -ENOSYS before skipping the
sys_call_table dispatch.

With syscall_get_nr() now returning -1, __seccomp_filter() jumps to skip and
returns false without calling syscall_set_return_value(), so
syscall_enter_from_user_mode_work() returns permit = false. In
arch/s390/kernel/syscall.c:__do_syscall():

	regs->gprs[2] = nr;
	...
	permit = syscall_enter_from_user_mode_work(regs, &nr);
	...
	if (unlikely(test_and_clear_pt_regs_flag(regs, PIF_SYSCALL_RET_SET) || !permit))
		goto out;
	regs->gprs[2] = -ENOSYS;

when permit is false, execution jumps to out before setting
regs->gprs[2] = -ENOSYS:

- If a tracer skips the syscall at PTRACE_EVENT_SECCOMP via
  PTRACE_SET_SYSCALL_INFO with nr = -1, syscall_set_nr() only updates
  regs->int_code and leaves regs->gprs[2] holding the original positive
  syscall number, returning that positive number to userspace.
- If the tracer skips the syscall via PTRACE_POKEUSR or PTRACE_SETREGSET by
  writing -1 to gprs[2], __poke_user() writes -1 into regs->gprs[2] without
  setting PIF_SYSCALL_RET_SET, returning -1 (-EPERM) instead of -ENOSYS.
- If userspace invokes syscall(-1) directly under a SECCOMP_RET_TRACE filter
  without the tracer modifying registers, regs->gprs[2] remains 65535 and is
  returned to userspace instead of -ENOSYS.

[Severity: Medium]
Should arch/s390/kernel/syscall.c:__do_syscall() and
arch/s390/include/asm/syscall.h:syscall_set_nr() also be updated to keep the
syscall number representation consistent with syscall_get_nr()?

In arch/s390/kernel/syscall.c:__do_syscall():

	nr = regs->int_code & 0xffff;
	if (likely(!nr)) {
		nr = regs->gprs[1] & 0xffff;
		regs->int_code &= ~0xffffUL;
		regs->int_code |= nr;
	}
	regs->gprs[2] = nr;

nr and regs->gprs[2] are still extracted as zero-extended 16-bit values. When
userspace invokes a negative syscall number such as syscall(-1), regs->gprs[2]
(the ptrace ABI register for the syscall number at entry) and the initial nr
passed into syscall_enter_from_user_mode_work() are 65535 while
syscall_get_nr() and PTRACE_GET_SYSCALL_INFO report -1.

Similarly, in arch/s390/include/asm/syscall.h:syscall_set_nr():

	regs->int_code = (regs->int_code & ~0xffff) | (nr & 0xffff);

only regs->int_code is updated while regs->gprs[2] is left unchanged. If a
tracer modifies the syscall number via PTRACE_SET_SYSCALL_INFO, subsequent
reads via PTRACE_PEEKUSR or PTRACE_GETREGSET(NT_PRSTATUS) during syscall
entry still see the old syscall number in regs->gprs[2], and writing back
NT_PRSTATUS via PTRACE_SETREGSET overwrites regs->int_code with that stale
regs->gprs[2] value in __poke_user().

>  
>  static inline void syscall_set_nr(struct task_struct *task,

[ ... ]

>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021125911.1550331-1-hca@linux.ibm.com?part=1