Re: [PATCH v16 01/18] seccomp: Convert __secure_computing() to return boolean

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Incomplete conversion of secure_computing() return semantics in x86 vsyscall emulation causes allowed vsyscalls to be incorrectly skipped and blocked vsyscalls to be executed.
- [Critical] Inverted seccomp evaluation logic for tracer-modified syscalls in SECCOMP_RET_TRACE allows ptracers to bypass seccomp and execute blocked system calls.
--

commit f5bc8c5696220b369255d486da26d273d661d2fa
Author: Jinjie Ruan <ruanjinjie@huawei.com>

seccomp: Convert __secure_computing() to return boolean

The return value of __secure_computing() currently uses 0 to indicate
that a system call should be allowed, and -1 to indicate that it should
be blocked/killed. This 0/-1 pattern is non-intuitive for a security
check function and makes the control flow at the call sites less readable.

> diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
> index ea36de9fa8641..6aed3987b9f96 100644
> --- a/arch/x86/entry/vsyscall/vsyscall_64.c
> +++ b/arch/x86/entry/vsyscall/vsyscall_64.c
> @@ -198,7 +198,7 @@ static bool __emulate_vsyscall(struct pt_regs *regs, unsigned long address)
>  	regs->orig_ax = syscall_nr;
>  	regs->ax = -ENOSYS;
>  	tmp = secure_computing();
> -	if ((!tmp && regs->orig_ax != syscall_nr) || regs->ip != address) {
> +	if ((tmp && regs->orig_ax != syscall_nr) || regs->ip != address) {
>  		warn_bad_vsyscall(KERN_DEBUG, regs,
>  				  "seccomp tried to change syscall nr or ip");
>  		force_exit_sig(SIGSYS);
>  		return true;
>  	}
>  	regs->orig_ax = -1;
>  	if (tmp)
>  		goto do_ret;  /* skip requested */

[Severity: Critical]
Since secure_computing() now returns true when allowing a system call, doesn't
this check skip the emulation for allowed vsyscalls and execute the blocked
ones instead?

[ ... ]

> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 066909393c38f..1fec6efedab62 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c

[ ... ]

> @@ -1330,19 +1330,19 @@ static int __seccomp_filter(int this_syscall, const bool recheck_after_trace)
>  		 * a skip would have already been reported.
>  		 */
>  		if (__seccomp_filter(this_syscall, true))
> -			return -1;
> +			return false;
>  
> -		return 0;
> +		return true;

[Severity: Critical]
Is this logic inverted for tracer-modified syscalls in SECCOMP_RET_TRACE?

__seccomp_filter() now returns true on allow and false on deny.
If the nested __seccomp_filter() call evaluates the modified syscall and
returns true (allow), the if condition evaluates to true and it returns
false (deny).

Conversely, if it returns false (deny), it bypasses the if condition and
returns true (allow).

Could a ptracer bypass the seccomp sandbox entirely by changing the syscall
to a prohibited one, which would then be incorrectly allowed here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629130616.642022-1-ruanjinjie@huawei.com?part=1