Re: [PATCH 2/2] s390/syscall: Keep syscall number in extra ptregs member

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Syscall restarts for system calls invoked via the `EXECUTE` instruction will rewind the instruction pointer by the wrong amount, causing execution corruption.
- [High] The patch breaks the ptrace `NT_S390_SYSTEM_CALL` regset ABI by omitting the instruction length data.
- [High] The syscall number loaded from `%r1` is no longer truncated to 16 bits, allowing arbitrary garbage in the upper bits to cause valid syscalls to fail.
--

--- Patch [2]: [PATCH 2/2] s390/syscall: Keep syscall number in extra ptregs member ---
commit 1c7e9df362bfdaa075e7114b5570a63dc6c4e073
Author: Sven Schnelle <svens@linux.ibm.com>

s390/syscall: Keep syscall number in extra ptregs member
    
Move the syscall number from ptregs::int_code to another union member
called syscall_nr. This simplifies the code a bit.

> diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
> index 4874de5edea0a..c3c69294ff330 100644
> --- a/arch/s390/kernel/signal.c
> +++ b/arch/s390/kernel/signal.c
> @@ -447,12 +447,12 @@ void arch_do_signal_or_restart(struct pt_regs *regs)
>  	 * call information.
>  	 */
>  	current->thread.system_call =
> -		test_pt_regs_flag(regs, PIF_SYSCALL) ? regs->int_code : 0;
> +		test_pt_regs_flag(regs, PIF_SYSCALL) ? regs->syscall_nr : 0;

[Severity: High]
Does this break the ptrace NT_S390_SYSTEM_CALL regset ABI?

Previously, thread.system_call stored both the syscall number and the
instruction length code (ILC) in the upper 16 bits, which userspace tools 
like CRIU depend on to correctly restart syscalls.

By saving only regs->syscall_nr, it appears the instruction length data is
dropped.

>  
>  	if (get_signal(&ksig)) {
>  		/* Whee!  Actually deliver the signal.  */
>  		if (current->thread.system_call) {
> -			regs->int_code = current->thread.system_call;
> +			regs->syscall_nr = current->thread.system_call;
>  			/* Check for system call restarting. */
>  			switch (regs->gprs[2]) {

[ ... ]

> @@ -467,9 +467,7 @@ void arch_do_signal_or_restart(struct pt_regs *regs)
>  				fallthrough;
>  			case -ERESTARTNOINTR:
>  				regs->gprs[2] = regs->orig_gpr2;
> -				regs->psw.addr =
> -					__rewind_psw(regs->psw,
> -						     regs->int_code >> 16);
> +				regs->psw.addr = __rewind_psw(regs->psw, SVC_LEN);

[Severity: High]
Is it safe to rewind the PSW address by a hardcoded SVC_LEN (2 bytes) here?

If a user-space application invokes a restartable syscall using the EXECUTE
instruction, the dynamic instruction length is 4 or 6 bytes. 

Because the hardware ILC is no longer used to determine the rewind length,
could resuming userspace result in execution corruption or a SIGILL by
resuming in the middle of the EXECUTE instruction?

>  				break;
>  			}
>  		}

[ ... ]

> @@ -498,7 +496,7 @@ void arch_do_signal_or_restart(struct pt_regs *regs)
>  		case -ERESTARTSYS:
>  		case -ERESTARTNOINTR:
>  			regs->gprs[2] = regs->orig_gpr2;
> -			regs->psw.addr = __rewind_psw(regs->psw, regs->int_code >> 16);
> +			regs->psw.addr = __rewind_psw(regs->psw, SVC_LEN);

[Severity: High]
This seems to have the same hardcoded SVC_LEN issue as above when restarting
syscalls on the alternative restart path. Could this also cause execution
corruption if the syscall was invoked via EXECUTE?

>  			if (test_thread_flag(TIF_SINGLE_STEP))
>  				clear_thread_flag(TIF_PER_TRAP);
>  			break;

[ ... ]

> diff --git a/arch/s390/kernel/syscall.c b/arch/s390/kernel/syscall.c
> index ce244dceec6dc..62e283ffef373 100644
> --- a/arch/s390/kernel/syscall.c
> +++ b/arch/s390/kernel/syscall.c
> @@ -100,7 +100,10 @@ void noinstr __do_syscall(struct pt_regs *regs, int per_trap)
>  	enter_from_user_mode(regs);
>  	add_random_kstack_offset();
>  	regs->psw = get_lowcore()->svc_old_psw;
> -	regs->int_code = get_lowcore()->svc_int_code;
> +	nr = get_lowcore()->svc_int_code & 0xffff;
> +	if (likely(!nr))
> +		nr = regs->gprs[1];
> +	regs->syscall_nr = nr;

[Severity: High]
Does this assignment need to truncate the value read from regs->gprs[1] to
16 bits?

If a 31-bit compat application, or a 64-bit application leaves garbage in
the upper 32 bits of register %r1 and issues a syscall with SVC number 0, nr
will include that garbage. 

Could this cause the subsequent bounds check on nr in __do_syscall() to
fail, returning -ENOSYS for an otherwise valid syscall?

>  	update_timer_sys();
>  	if (cpu_has_bear())
>  		current->thread.last_break = regs->last_break;

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