Re: [PATCH v11 2/3] ptrace: add PTRACE_SET_SYSCALL_INFO syscall skipping support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] ptrace: ptrace_set_syscall_info_seccomp_skip() breaks syscall
  skip on powerpc
--

commit 18172201986e4a2acbcbbd3f1822583db8925569
Author: Renzo Davoli <renzo@cs.unibo.it>

ptrace: add PTRACE_SET_SYSCALL_INFO syscall skipping support

This commit extends PTRACE_SET_SYSCALL_INFO to allow a tracer to skip a
system call at a PTRACE_SYSCALL_INFO_SECCOMP stop by changing the operation
to PTRACE_SYSCALL_INFO_EXIT and specifying the return value.

> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 3f2fe68ccb140..ab6f5dab02238 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c

[ ... ]

> @@ -1119,12 +1128,35 @@ ptrace_set_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
>  	return 0;
>  }
>  
> +/*
> + * Skip the system call at a PTRACE_SYSCALL_INFO_SECCOMP stop and set its
> + * return value as specified by info->exit.
> + *
> + * The skip request is passed to seccomp out of band instead of by setting
> + * the system call number to -1: on some architectures the syscall number,
> + * or the register seccomp reads it from, shares a register with the return
> + * value, and on others -1 cannot be represented as a syscall number.
> + */
> +static int
> +ptrace_set_syscall_info_seccomp_skip(struct task_struct *child,
> +				     struct pt_regs *regs,
> +				     struct ptrace_syscall_info *info)
> +{
> +	int ret = ptrace_set_syscall_info_exit(child, regs, info);
> +
> +	if (!ret)
> +		seccomp_ptrace_skip_syscall(child);
> +
> +	return ret;
> +}

[Severity: High]
Does calling ptrace_set_syscall_info_exit() from the tracer while the tracee
is stopped at PTRACE_EVENT_SECCOMP break both the tracer and the tracee on
powerpc?

ptrace_set_syscall_info_exit() calls syscall_set_return_value(child, regs,
...), which on powerpc sets TIF_SYSCALL_RET on current rather than on task:

arch/powerpc/include/asm/syscall.h:syscall_set_return_value() {
    ...
	/*
	 * We are returning from the syscall, so anything that
	 * depends on the regs being in the entry state is now invalid.
	 */
	set_thread_flag(TIF_SYSCALL_RET);
}

Because ptrace_set_syscall_info_seccomp_skip() runs in the tracer's task
context, TIF_SYSCALL_RET is set on the tracer.  On the tracer's next system
call (such as PTRACE_GET_SYSCALL_INFO or PTRACE_CONT),
system_call_exception() sees TIF_SYSCALL_RET set, clears it, and returns
syscall_get_error(current, regs) (0) without executing the system call:

arch/powerpc/kernel/syscall.c:system_call_exception() {
    ...
	if (unlikely(!syscall_enter_from_user_mode_randomize_stack(regs, &r0) ||
			test_thread_flag(TIF_SYSCALL_RET))) {
		clear_thread_flag(TIF_SYSCALL_RET);
		return syscall_get_error(current, regs);
	}
    ...
}

In addition, when the tracee resumes from the skipped seccomp stop,
syscall_enter_from_user_mode_randomize_stack() returns false and
system_call_exception() returns syscall_get_error(current, regs), which
syscall_exit_prepare() writes into regs->gpr[3].  When info->exit.is_error is
0, syscall_get_error() returns 0, overwriting any non-error return value set
by the tracer in regs->gpr[3].

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114181923.370930-1-renzo@cs.unibo.it?part=2