Re: [PATCH] seccomp: Fix syscall skip logic on ptrace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Removing the explicit check for tracer-injected negative syscall numbers breaks ptrace syscall emulation by erroneously re-evaluating the seccomp BPF filter against a `-1` syscall number.
--

--- Patch [1]: [PATCH] seccomp: Fix syscall skip logic on ptrace ---
commit e1c90f3fd79c2a1c1deae6adb363b4de34006c48
Author: Michal Suchanek <msuchanek@suse.de>

seccomp: Fix syscall skip logic on ptrace

This commit removes the explicit check for negative syscall numbers after a
ptrace stop in the seccomp filter. This allows architectures where the return
value overlaps with the syscall number or arguments to exit without flagging
the syscall as skipped.

> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 25f62867a16d9..4d3ad6113b216 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1318,11 +1318,8 @@ static int __seccomp_filter(int this_syscall, const bool recheck_after_trace)
>  		 */
>  		if (fatal_signal_pending(current))
>  			goto skip;
> -		/* 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_syscall = syscall_get_nr(current, current_pt_regs());

[Severity: Critical]
By removing this check, does this break ptrace syscall emulation?

The standard mechanism for syscall emulation using ptrace and seccomp (used
by gVisor, User-Mode Linux, proot, and strace) involves changing the syscall
number to -1 to skip the real kernel syscall and setting a custom return
value.

Without this check, when the tracee resumes and re-enters __seccomp_filter(),
the -1 syscall number will fall through to the BPF evaluation below. Since
most seccomp profiles use strict allowlists, won't this cause the -1 syscall
to hit the default action (like SECCOMP_RET_KILL_PROCESS or
SECCOMP_RET_ERRNO) instead of skipping it?

Could this lead to tracees being unexpectedly killed or having their preset
return values overwritten?

>  		/*
>  		 * Recheck the syscall, since it may have changed. This
>  		 * intentionally uses a NULL struct seccomp_data to force

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/al9NlZi20xHMvla2@kunlun.suse.cz?part=1