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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Tracer modifications to syscall return values during exit stops are completely ignored.
- [Low] The patch leaves behind an unused and broken macro `_PIF_SYSCALL_RET_SET`.

Pre-existing issues:
- [High] `syscall_rollback()` register restoration is unconditionally clobbered by `-ENOSYS` assignment.
--

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

s390/syscall: Keep syscall return in extra ptregs member

This patch introduces a new member syscall_ret in struct pt_regs to keep
the syscall return value separate from gprs[2]. It copies this value back
to gprs[2] before exit to userspace, resolving issues where setting both a
return value and skipping the syscall was impossible with seccomp or ptrace.

Link: https://lore.kernel.org/all/20260703105718.GO751831@noisy.programming.kicks-ass.net/

> diff --git a/arch/s390/include/asm/ptrace.h b/arch/s390/include/asm/ptrace.h
> index 495e310c3d6d7..f39b349da9a7d 100644
> --- a/arch/s390/include/asm/ptrace.h
> +++ b/arch/s390/include/asm/ptrace.h
> @@ -15,9 +15,8 @@
>  
>  #define PIF_SYSCALL			0	/* inside a system call */
>  #define PIF_PSW_ADDR_ADJUSTED		1	/* psw address has been adjusted */
> -#define PIF_SYSCALL_RET_SET		2	/* return value was set via ptrace */
> -#define PIF_GUEST_FAULT			3	/* indicates program check in sie64a */
> -#define PIF_FTRACE_FULL_REGS		4	/* all register contents valid (ftrace) */
> +#define PIF_GUEST_FAULT			2	/* indicates program check in sie64a */
> +#define PIF_FTRACE_FULL_REGS		3	/* all register contents valid (ftrace) */

[Severity: Low]
Could this leave behind a broken macro? Since PIF_SYSCALL_RET_SET is deleted
here, it looks like _PIF_SYSCALL_RET_SET (which is defined right below this
using BIT(PIF_SYSCALL_RET_SET)) will cause a compilation failure if it is ever
used.

[ ... ]

> diff --git a/arch/s390/kernel/syscall.c b/arch/s390/kernel/syscall.c
> index 75d5a3cab14e6..ce244dceec6dc 100644
> --- a/arch/s390/kernel/syscall.c
> +++ b/arch/s390/kernel/syscall.c
> @@ -117,25 +117,16 @@ void noinstr __do_syscall(struct pt_regs *regs, int per_trap)

[ ... ]

>  	if (likely(nr < NR_syscalls)) {
>  		nr = array_index_nospec(nr, NR_syscalls);
> -		regs->gprs[2] = sys_call_table[nr](regs);
> +		regs->syscall_ret = sys_call_table[nr](regs);
>  	}
> -out:
> +	regs->gprs[2] = regs->syscall_ret;

[Severity: High]
Are tracer modifications to the syscall return value being ignored here?

The assignment regs->gprs[2] = regs->syscall_ret happens before we call
syscall_exit_to_user_mode().

If a tracer stops the process during exit hooks inside
syscall_exit_to_user_mode() and modifies the return value, it updates
regs->syscall_ret via syscall_set_return_value(), but this change is never
propagated to the actual CPU register gprs[2].

Will the process resume using the unmodified original return value, completely
breaking exit-time return value injection?

[Severity: High]
This is a pre-existing issue, but isn't the register restoration from
syscall_rollback() clobbered by this assignment?

If a seccomp filter uses SECCOMP_RET_TRAP to abort the syscall, it triggers a
rollback which restores regs->orig_gpr2 into regs->gprs[2]:

arch/s390/include/asm/syscall.h:syscall_rollback() {
    ...
    regs->gprs[2] = regs->orig_gpr2;
    ...
}

However, this restored value is then immediately overwritten here by the
assignment regs->gprs[2] = regs->syscall_ret (which holds -ENOSYS for aborted
syscalls).

Does this break userspace seccomp signal handlers that expect the restored
state in gprs[2]?

>  	syscall_exit_to_user_mode(regs);
>  }

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