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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ptrace: ptrace_set_syscall_info_seccomp_skip() breaks syscall
  skip on powerpc
- [Medium] ptrace: stale SECCOMP op in ptrace_get_syscall_info_op()
  after syscall skip
--

commit 8c9888f26aefc1ee67703974a37b40daf729c189
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 at a
PTRACE_SYSCALL_INFO_SECCOMP stop to skip the system call and set its return
value by changing info.op to PTRACE_SYSCALL_INFO_EXIT.

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

[ ... ]

> @@ -1119,12 +1120,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() -> syscall_set_return_value() from
a seccomp stop work on powerpc?

arch/powerpc/include/asm/syscall.h:syscall_set_return_value() {
    ...
	set_thread_flag(TIF_SYSCALL_RET);
}

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);
	}
    ...
}

Because syscall_set_return_value() calls set_thread_flag(TIF_SYSCALL_RET)
rather than set_tsk_thread_flag(task, TIF_SYSCALL_RET), calling it from the
tracer sets TIF_SYSCALL_RET on current (the tracer), causing
system_call_exception() to skip the tracer's next system call (such as
PTRACE_CONT) and return 0.

In addition, when the tracee resumes from the skipped seccomp stop with a
non-error return value (info->exit.is_error == 0), system_call_exception()
returns syscall_get_error(current, regs), which is 0 when no error is set, and
syscall_exit_prepare() overwrites regs->gpr[3] with that 0, clobbering the
return value set by the tracer.

>  
>  static int
>  ptrace_set_syscall_info(struct task_struct *child, unsigned long user_size,
>  			const void __user *datavp)
>  {
>  	struct pt_regs *regs = task_pt_regs(child);
>  	struct ptrace_syscall_info info;
> +	int op;
>  
>  	if (user_size < sizeof(info))
>  		return -EINVAL;
> @@ -1141,8 +1165,25 @@ ptrace_set_syscall_info(struct task_struct *child, unsigned long user_size,
>  	if (info.flags || info.reserved)
>  		return -EINVAL;
>  
> -	/* Changing the type of the system call stop is not supported yet. */
> -	if (ptrace_get_syscall_info_op(child) != info.op)
> +	op = ptrace_get_syscall_info_op(child);
> +
> +	/*
> +	 * Changing the type of the system call stop is not supported, except
> +	 * that a PTRACE_SYSCALL_INFO_SECCOMP stop can be turned into a
> +	 * PTRACE_SYSCALL_INFO_EXIT one to skip the system call and set its
> +	 * return value.  Once that is done, the system call number and
> +	 * arguments cannot be changed anymore during the same stop, as the
> +	 * return value might share registers with them.
> +	 */
> +	if (op == PTRACE_SYSCALL_INFO_SECCOMP) {
> +		if (info.op == PTRACE_SYSCALL_INFO_EXIT)
> +			return ptrace_set_syscall_info_seccomp_skip(child, regs,
> +								    &info);
> +		if (seccomp_ptrace_syscall_skipped(child))
> +			return -EINVAL;
> +	}

[Severity: Medium]
Should ptrace_get_syscall_info_op() also return PTRACE_SYSCALL_INFO_EXIT once
seccomp_ptrace_syscall_skipped(child) is true?

Right now, ptrace_get_syscall_info_op() continues to return
PTRACE_SYSCALL_INFO_SECCOMP for SIGTRAP | (PTRACE_EVENT_SECCOMP << 8) even
after the syscall has been skipped.  If the tracer calls
PTRACE_GET_SYSCALL_INFO later during the same stop, ptrace_get_syscall_info()
still reports op == PTRACE_SYSCALL_INFO_SECCOMP and populates info.seccomp from
pt_regs whose syscall number or argument registers may already have been
overwritten by syscall_set_return_value(), and passing that structure back to
PTRACE_SET_SYSCALL_INFO fails with -EINVAL.

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