Re: [PATCH v2 1/2] ARM: ptrace: Fix seccomp seeing stale syscall arguments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ARM: ptrace: missing syscall-entry match in stopped_in_syscall_entry()
- [High] ARM: ptrace: ARM_ORIG_r0 desync in put_user_reg() via PTRACE_POKEUSR
- [High] ARM: ptrace: ARM_ORIG_r0 clobbered in update_syscall_orig_r0_after_ptrace()
--

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

ARM: ptrace: Fix seccomp seeing stale syscall arguments

This commit synchronizes ARM_ORIG_r0 with ARM_r0 after ptrace register writes
when the tracee is stopped in syscall entry or SECCOMP_RET_TRACE, and updates
audit_syscall_entry() to read ARM_ORIG_r0.

Link: https://lore.kernel.org/all/al98jdcleu-6tMDL@willie-the-truck/
Link: https://lkml.org/lkml/2020/9/11/1282

> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index ed7a2a87a6707..6ec81e5a205e7 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -163,6 +163,42 @@ static inline long get_user_reg(struct task_struct *task, int offset)
>  	return task_pt_regs(task)->uregs[offset];
>  }
>  
> +/*
> + * Is @target stopped after entering a syscall, but before it has been
> + * dispatched?  This is the same classification that
> + * ptrace_get_syscall_info_op() performs for PTRACE_GET_SYSCALL_INFO.
> + */
> +static bool stopped_in_syscall_entry(struct task_struct *target)
> +{
> +	struct kernel_siginfo *info = target->last_siginfo;
> +
> +	if (!info)
> +		return false;
> +
> +	if (info->si_code == (SIGTRAP | 0x80))
> +		return target->ptrace_message == PTRACE_EVENTMSG_SYSCALL_ENTRY;

[Severity: High]
What happens when a tracer stops a tracee at syscall entry via PTRACE_SYSCALL
without enabling PTRACE_O_TRACESYSGOOD?

In include/linux/ptrace.h:ptrace_report_syscall(), bit 0x80 is only set in
si_code when PT_TRACESYSGOOD is enabled:

	signr = ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0),
			      message);

When PTRACE_O_TRACESYSGOOD is not set, info->si_code is plain SIGTRAP while
target->ptrace_message is PTRACE_EVENTMSG_SYSCALL_ENTRY. Checking for
info->si_code == (SIGTRAP | 0x80) causes stopped_in_syscall_entry() to return
false in that case, leaving regs->ARM_ORIG_r0 unsynchronized when the tracer
updates ARM_r0.

When the tracee resumes in syscall_trace_enter(), seccomp_permit_syscall(),
trace_sys_enter(), and audit_syscall_entry() (which this patch switches from
regs->ARM_r0 to regs->ARM_ORIG_r0) will all observe the stale
regs->ARM_ORIG_r0 while invoke_syscall runs with the modified regs->ARM_r0.

Should this check (info->si_code & ~0x80) == SIGTRAP, matching arm64's
stopped_in_syscall_entry()?

> +
> +	return info->si_code == (SIGTRAP | (PTRACE_EVENT_SECCOMP << 8));
> +}
> +
> +static void update_syscall_orig_r0_after_ptrace(struct task_struct *target)
> +{
> +	struct pt_regs *regs = task_pt_regs(target);
> +
> +	/*
> +	 * ARM_r0 holds the first syscall argument only before the syscall is
> +	 * dispatched; seccomp, the syscall tracepoints and the syscall restart
> +	 * code all read that argument from ARM_ORIG_r0 instead.  At a syscall
> +	 * exit stop ARM_r0 holds the return value and must not be copied, and
> +	 * for other exceptions ARM_ORIG_r0 is set to ~0 and carries no
> +	 * meaning.
> +	 */
> +	if (!stopped_in_syscall_entry(target))
> +		return;
> +
> +	regs->ARM_ORIG_r0 = regs->ARM_r0;
> +}

[Severity: High]
Can update_syscall_orig_r0_after_ptrace() clobber the saved first argument
when a tracer skips a syscall?

On 32-bit ARM, skipping a syscall at a syscall-entry or PTRACE_EVENT_SECCOMP
stop sets task_thread_info(target)->abi_syscall to -1 via PTRACE_SET_SYSCALL
in arch_ptrace() or via syscall_set_nr():

	if (nr == -1) {
		task_thread_info(task)->abi_syscall = -1;
		syscall_set_return_value(task, regs, -ENOSYS, 0);
		return;
	}

Once the syscall number is -1, regs->ARM_r0 holds the return value (such as
-ENOSYS or a tracer-supplied return value), while regs->ARM_ORIG_r0 preserves
the original first argument. Because update_syscall_orig_r0_after_ptrace()
does not check whether syscall_get_nr(target, regs) == -1 (unlike arm64,
which returns early when regs->syscallno == NO_SYSCALL), any subsequent
GPR write via put_user_reg() or gpr_set() overwrites regs->ARM_ORIG_r0 with
the return value.

That corrupts the first argument reported by syscall_get_arguments() (used by
PTRACE_GET_SYSCALL_INFO, trace_sys_enter(), and seccomp) and
audit_syscall_entry(), breaks syscall_rollback() if a later seccomp filter
triggers SECCOMP_RET_TRAP or SECCOMP_RET_KILL_PROCESS, and causes do_signal()
on the __sys_trace_return_nosave -> ret_slow_syscall path to copy an injected
-ERESTART* return value into ARM_r0 as the restarted syscall's first argument.

Conversely, in arch_ptrace(), PTRACE_SET_SYSCALL does not call
update_syscall_orig_r0_after_ptrace(child) when changing abi_syscall from -1
to a valid syscall number, leaving ARM_ORIG_r0 and ARM_r0 out of sync if
syscall_set_nr() had previously set ARM_r0 to -ENOSYS.

Should update_syscall_orig_r0_after_ptrace() return early when
syscall_get_nr(target, regs) == -1, and should PTRACE_SET_SYSCALL call
update_syscall_orig_r0_after_ptrace(child)?

> +
>  /*
>   * this routine will put a word on the processes privileged stack.
>   * the offset is how far from the base addr as stored in the THREAD.
> @@ -180,6 +216,8 @@ put_user_reg(struct task_struct *task, int offset, long data)
>  
>  	if (valid_user_regs(&newregs)) {
>  		regs->uregs[offset] = data;
> +		if (offset == 0)
> +			update_syscall_orig_r0_after_ptrace(task);
>  		ret = 0;
>  	}

[Severity: High]
Could a tracer still desynchronize regs->ARM_ORIG_r0 and regs->ARM_r0 via
PTRACE_POKEUSR in put_user_reg()?

On 32-bit ARM, struct pt_regs exposes all 18 words (uregs[0..17]) to
ptrace_write_user() -> put_user_reg(), where offset 0 is ARM_r0 and offset 17
is ARM_ORIG_r0. At the same time, invoke_syscall in
arch/arm/kernel/entry-header.S dispatches the syscall by reloading r0-r6 from
regs->ARM_r0:

	add	r1, sp, #S_R0 + S_OFF		@ pointer to regs
	ldmiacc	r1, {r0 - r6}			@ reload r0-r6

Because put_user_reg() only calls update_syscall_orig_r0_after_ptrace(task)
when offset == 0, a tracer stopped at syscall entry or PTRACE_EVENT_SECCOMP
can poke offset 17 (ARM_ORIG_r0) directly (or modify
target->last_siginfo->si_code via PTRACE_SETSIGINFO before writing ARM_r0).
In that case, seccomp_permit_syscall(), trace_sys_enter(), and now
audit_syscall_entry() all inspect the value written to regs->ARM_ORIG_r0,
while invoke_syscall executes the syscall with regs->ARM_r0.

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