Re: [PATCH] arm64: syscall: Ensure saved x0 is kept in-sync with tracer updates

[Kees Cook]

On Thu, Jul 16, 2026 at 10:57:34AM +0800, Jinjie Ruan wrote:
> 
> 
> On 7/14/2026 10:35 PM, Will Deacon wrote:
> > When seccomp support was originally added to arm64 in a1ae65b21941
> > ("arm64: add seccomp support"), seccomp was erroneously called _before_
> > the ptrace syscall-enter-stop and therefore the tracer could trivially
> > manipulate the syscall register state after the seccomp check had
> > passed. This was subsequently fixed in a5cd110cb836 ("arm64/ptrace: run
> > seccomp after ptrace") by moving the seccomp check after the tracer has
> > run. Unfortunately, a decade later, that fix has been reported to be
> > incomplete.
> > 
> > On arm64, both the first argument to a syscall and its eventual return
> > value are allocated to register x0. In order to facilitate syscall
> > restarting and querying of syscall arguments on the syscall exit path,
> > the original value of x0 is stashed in 'struct pt_regs::orig_x0' early
> > during the syscall entry path and is returned for the first argument by
> > syscall_get_arguments(). Unlike 32-bit Arm, this stashed value is not
> > directly exposed via ptrace() and so changes to register x0 made by the
> > tracer on a syscall-enter-stop are not reflected in 'orig_x0'. This
> > means that seccomp and audit can observe a stale value for the register
> > compared to the argument that will be observed by the actual syscall.
> > 
> > Re-sync 'orig_x0' from x0 on the syscall entry path following a
> > potential ptrace stop (i.e. PTRACE_EVENTMSG_SYSCALL_ENTRY or
> > SECCOMP_RET_TRACE). This behaviour is limited to native tasks (because
> > compat tasks expose 'orig_r0' to ptrace) where the syscall is not being
> > skipped (because x0 is updated to hold the return value of -ENOSYS in
> > that case).
> > 
> > Cc: Kees Cook <kees@xxxxxxxxxx>
> > Cc: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
> > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > Reported-by: Yiqi Sun <sunyiqixm@xxxxxxxxx>
> > Link: https://lore.kernel.org/all/20260529065444.1336608-1-sunyiqixm@xxxxxxxxx/
> > Suggested-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Fixes: a5cd110cb836 ("arm64/ptrace: run seccomp after ptrace")
> > Signed-off-by: Will Deacon <will@xxxxxxxxxx>
> > ---
> >  arch/arm64/kernel/ptrace.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index 4d08598e2891..57e8c6714d44 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -2408,6 +2408,21 @@ static void report_syscall_exit(struct pt_regs *regs)
> >  	}
> >  }
> >  
> > +static void update_syscall_orig_x0_after_ptrace(struct pt_regs *regs)
> > +{
> > +	/*
> > +	 * Keep orig_x0 authoritative so that seccomp (via
> > +	 * syscall_get_arguments()), audit and the restart path all see the same
> > +	 * first argument the syscall is dispatched with, even if it has been
> > +	 * updated by a tracer. Skip this for NO_SYSCALL (set either by the user
> > +	 * or the tracer), as regs[0] holds the return value (see the comment in
> > +	 * el0_svc_common()) and can be unwound using syscall_rollback().
> > +	 * For compat tasks, orig_r0 is provided directly through GPR index 17.
> > +	 */
> > +	if (!is_compat_task() && regs->syscallno != NO_SYSCALL)
> > +		regs->orig_x0 = regs->regs[0];
> > +}
> > +
> >  int syscall_trace_enter(struct pt_regs *regs)
> >  {
> >  	unsigned long flags = read_thread_flags();
> > @@ -2417,12 +2432,21 @@ int syscall_trace_enter(struct pt_regs *regs)
> >  		ret = report_syscall_entry(regs);
> >  		if (ret || (flags & _TIF_SYSCALL_EMU))
> >  			return NO_SYSCALL;
> > +
> > +		/*
> > +		 * Ensure ptrace changes to x0 are visible to seccomp
> > +		 * ptrace exits (SECCOMP_RET_TRACE).
> > +		 */
> > +		update_syscall_orig_x0_after_ptrace(regs);
> >  	}
> >  
> >  	/* Do the secure computing after ptrace; failures should be fast. */
> >  	if (secure_computing() == -1)
> >  		return NO_SYSCALL;
> >  
> > +	/* Ensure seccomp updates to x0 are visible to audit. */
> > +	update_syscall_orig_x0_after_ptrace(regs);
> 
> 
> Hi, will
> 
> I think unconditionally updating orig_x0 here is unnecessary, we could
> Expand seccomp check in place as below the same as generic entry.
> 
> In this way, in most cases where seccomp is not used, the overhead of
> updating orig_x0 is eliminated. Moreover, we only need to define an
> architecture-specific version of the seccomp function, thus avoiding the
> pain of switching from arm64 to the generic entry.
> 
> So this patch can be like below.
> 
> int syscall_trace_enter(struct pt_regs *regs)
>  {
>         unsigned long flags = read_thread_flags();
> @@ -2420,12 +2435,24 @@ int syscall_trace_enter(struct pt_regs *regs)
> 
>                 /* ptrace might have changed work flags */
>                 flags = read_thread_flags();
> +               /*
> +                * Ensure ptrace changes to x0 during a regular
> syscall-enter-stop
> +                * (PTRACE_SYSCALL) are visible to subsequent seccomp trace
> +                * and audit checking.
> +                */
> +               update_syscall_orig_x0_after_ptrace(regs);
>         }
> 
>         /* Do the secure computing after ptrace; failures should be fast. */
>         if (unlikely(flags & _TIF_SECCOMP)) {
>                 if (!__seccomp_permit_syscall())
>                         return NO_SYSCALL;
> +
> +               /*
> +                * Ensure tracer changes to x0 during seccomp ptrace
> exit processing
> +                * (SECCOMP_RET_TRACE) are visible to audit.
> +                */
> +               update_syscall_orig_x0_after_ptrace(regs);
>         }
> 
> 
> Author: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
> Date:   Tue Oct 29 19:08:03 2024 +0800
> 
>     arm64: ptrace: Expand seccomp check in place
> 
>     Refactor syscall_trace_enter() by open-coding the seccomp check
>     to align with the generic entry framework. While the original call to
>     seccomp_permit_syscall() internally re-reads the thread flags and is
>     therefore safe against flag changes during ptrace stops, the new
>     open-coded version must explicitly re-read the flags after ptrace
>     handling to preserve that safety.
> 
>     [Background]
>     The generic entry implementation expands the seccomp check in-place
>     instead of using the seccomp_permit_syscall() wrapper. It directly
>     tests SYSCALL_WORK_SECCOMP and calls the underlying
>     __seccomp_permit_syscall() function to handle syscall filtering.
> 
>     [Changes]
>     1. After ptrace handling, re-read thread flags:
>        This ensures that any _TIF_SECCOMP set during the ptrace stop is
>        observed before the seccomp check.
> 
>     2. Open-code seccomp check:
>        - Instead of calling the seccomp_permit_syscall() wrapper, explicitly
>          check the updated 'flags' parameter for _TIF_SECCOMP.
>        - Call __seccomp_permit_syscall() directly if the flag is set.
> 
>     [Why this matters]
>     - Aligns the arm64 syscall path with the generic entry implementation,
>       simplifying future migration to the generic entry framework.
> 
>     - No functional changes are intended; seccomp behavior remains
> identical.
>       The explicit re-read ensures the open-coded version retains the same
>       safety as the original wrapper, preventing the race condition
> described
>       in the generic entry fix.
> 
>     - Performance: Non-ptrace fast path avoids atomic test_bit overhead via
>       cached flags.
> 
>     Cc: Mark Rutland <mark.rutland@xxxxxxx>
>     Cc: Will Deacon <will@xxxxxxxxxx>
>     Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>     Link:
> https://lore.kernel.org/all/20260713025712.416366-1-ruanjinjie@xxxxxxxxxx/
>     Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@xxxxxxx>
>     Reviewed-by: Linus Walleij <linusw@xxxxxxxxxx>
>     Reviewed-by: Yeoreum Yun <yeoreum.yun@xxxxxxx>
>     Reviewed-by: Kevin Brodsky <kevin.brodsky@xxxxxxx>
>     Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
> 
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 5709e9d3c321..941752656ea6 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -2417,11 +2417,16 @@ int syscall_trace_enter(struct pt_regs *regs)
>                 ret = report_syscall_entry(regs);
>                 if (ret || (flags & _TIF_SYSCALL_EMU))
>                         return NO_SYSCALL;
> +
> +               /* ptrace might have changed the flags */
> +               flags = read_thread_flags();
>         }
> 
>         /* Do the secure computing after ptrace; failures should be fast. */
> -       if (!seccomp_permit_syscall())
> -               return NO_SYSCALL;
> +       if (unlikely(flags & _TIF_SECCOMP)) {
> +               if (!__seccomp_permit_syscall())
> +                       return NO_SYSCALL;
> +       }
> 
>         if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
>                 trace_sys_enter(regs, regs->syscallno);

Do we have a corresponding seccomp_bpf.c selftest we can add for this? I
would really like to have a regression test that would catch this
issue...

-Kees

-- 
Kees Cook