Skip to contentSkip to conversation

arm64: syscall: Ensure saved x0 is kept in-sync with tracer updates

PATCH Applied Will Deacon 2 participants 7 messages Jul 16 – Jul 23

Subsystems (2)
lore
Share & tools
lore

Will Deacon

author maintainer
#0 lore raw

PATCH v2 1/1 arm64: syscall: Ensure saved x0 is kept in-sync with tracer updates

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, syscall tracepoints 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 <[email protected]>
Cc Jinjie Ruan <[email protected]>
Cc Mark Rutland <[email protected]>
Fixes a5cd110cb836 ("arm64/ptrace: run seccomp after ptrace")

---

Changes since v1 (https://lore.kernel.org/r/[email protected]):
* Reworded and expanded comments and commit message per Jinjie's feedback

 arch/arm64/kernel/ptrace.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
arch/arm64/kernel/ptrace.c +29 -0
index 4d08598e2891..390c9b2bd966 100644
@@ -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,26 @@ 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 during a regular
+		 * syscall-enter-stop (PTRACE_SYSCALL) are visible to
+		 * subsequent seccomp checks, tracepoints and audit.
+		 */
+		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 tracer changes to x0 during seccomp ptrace exit
+	 * processing (SECCOMP_RET_TRACE) are visible to tracepoints and
+	 * audit.
+	 */
+	update_syscall_orig_x0_after_ptrace(regs);
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_enter(regs, regs->syscallno);
 
Signature · 1 line
2.55.0.141.g00534a21ce-goog
Reviewed-by Tested-by on v2 1/1
#1 lore raw
Quoted 84 lines · Will Deacon

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, syscall tracepoints 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 <[email protected]>
Cc Jinjie Ruan <[email protected]>
Cc Mark Rutland <[email protected]>
Fixes a5cd110cb836 ("arm64/ptrace: run seccomp after ptrace")

---

Changes since v1 (https://lore.kernel.org/r/[email protected]):
* Reworded and expanded comments and commit message per Jinjie's feedback

 arch/arm64/kernel/ptrace.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
arch/arm64/kernel/ptrace.c +22 -0
index 4d08598e2891..390c9b2bd966 100644
@@ -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,26 @@ 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 during a regular
+		 * syscall-enter-stop (PTRACE_SYSCALL) are visible to
+		 * subsequent seccomp checks, tracepoints and audit.
+		 */
+		update_syscall_orig_x0_after_ptrace(regs);
 	}
 

arch/arm64/kernel/ptrace.c int syscall_trace_enter(struct pt_regs *regs) · line 2453

Quoted 10 lines
arch/arm64/kernel/ptrace.c +6 -0
 	/* Do the secure computing after ptrace; failures should be fast. */
 	if (secure_computing() == -1)
 		return NO_SYSCALL;
 
+	/*
+	 * Ensure tracer changes to x0 during seccomp ptrace exit
+	 * processing (SECCOMP_RET_TRACE) are visible to tracepoints and
+	 * audit.
+	 */
+	update_syscall_orig_x0_after_ptrace(regs);

LGTM

Quoted 4 lines
arch/arm64/kernel/ptrace.c +1 -0
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_enter(regs, regs->syscallno);
 
author maintainer Applied
#2 lore raw
In reply to Will Deacon
Quoted 10 lines · Will Deacon

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.

[...]

Applied to arm64 (for-next/fixes), thanks!

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

      https://git.kernel.org/arm64/c/e057b9477232

Cheers,

Signature · 5 lines
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
author maintainer
#3 lore raw
Quoted 10 lines · Will Deacon
Quoted 9 lines · Will Deacon

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.

Quoted 6 lines
Quoted 1 line

[...]

Applied to arm64 (for-next/fixes), thanks!

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

      https://git.kernel.org/arm64/c/e057b9477232

Bah, I've had to revert this. I think Sashiko makes a good point here that the seccomp interaction is still broken when the filter is re-evaluated after the tracer stop, because that all happens inside secure_computing() so we don't get a chance to update 'orig_x0':

https://sashiko.dev/#/patchset/[email protected]

I've got a v3 that takes a different approach, so I'll send that out shortly. Jinjie, thanks for sending the selftests, but maybe we can extend them to cover the loophole above as wel?

Will

#4 lore raw
Quoted 18 lines · Will Deacon
Quoted 10 lines · Will Deacon
Quoted 9 lines · Will Deacon

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.

Quoted 6 lines
Quoted 1 line

[...]

Applied to arm64 (for-next/fixes), thanks!

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

      https://git.kernel.org/arm64/c/e057b9477232
Quoted 7 lines

Bah, I've had to revert this. I think Sashiko makes a good point here that the seccomp interaction is still broken when the filter is re-evaluated after the tracer stop, because that all happens inside secure_computing() so we don't get a chance to update 'orig_x0':

https://sashiko.dev/#/patchset/[email protected]

Hi Will,

Yes, I also think the point raised by Sashiko is meaningful.

After reviewing the relevant code, I believe that the issue Sashiko pointed out regarding the compat task also exists.

My confusion is that on arm64 compat mode, both audit and trace use orig_x0, but the first parameter used for executing system calls is regs->reg[0]. If x0 is modified at the system call entry point in ptrace but orig_x0 is not modified, or if orig_x0 is modified but x0 is not, then the first parameter for audit and the actual system call being executed will be out of sync. Could there be any issues here? Is it the responsibility of ptrace to ensure that orig_x0 and x0 are synchronized at the system call entry point on arm64 compat mode?

Whether it is ptrace or the kernel, ensuring that orig_x0 and x0 are synchronized at the entry point of a system call, I believe it is reasonable to use orig_x0 as the first parameter of the system call execution and pre-set an error code in advance, as this way orig_x0 always retains the latest value of x0.

(quoted diff) +0 -0

b/arch/arm64/include/asm/syscall_wrapper.h
index abb57bc54305..6b13d7c8ad95 100644

arch/arm64/include/asm/syscall_wrapper.h +1 -1
@@ -12,7 +12,7 @@
 
 #define SC_ARM64_REGS_TO_ARGS(x, ...)                          \
        __MAP(x,__SC_ARGS                                       \
-             ,,regs->regs[0],,regs->regs[1],,regs->regs[2]     \
+             ,,regs->orig_x0,,regs->regs[1],,regs->regs[2]     \
              ,,regs->regs[3],,regs->regs[4],,regs->regs[5])
 
 #ifdef CONFIG_COMPAT
arch/arm64/kernel/syscall.c +0 -0
index 2535cae9413d..a978bab59924 100644
@@ -62,6 +62,7 @@ static __always_inline void el0_svc_common(struct

pt_regs *regs, int scno, int s

        unsigned long work;

        regs->orig_x0 = regs->regs[0];
+       syscall_set_return_value(current, regs, -ENOSYS, 0);
        regs->syscallno = scno;

        /*

@@ -94,23 +95,6 @@ static __always_inline void el0_svc_common(struct pt_regs *regs, int scno, int s

        work = READ_ONCE(current_thread_info()->syscall_work);
        if (unlikely(work & SYSCALL_WORK_ENTER)) {

- /*
- * The de-facto standard way to skip a system call using
ptrace
- * is to set the system call to -1 (NO_SYSCALL) and set
x0 to a
- * suitable error code for consumption by userspace.
However,
- * this cannot be distinguished from a user-issued
syscall(-1)
- * and so we must set x0 to -ENOSYS here in case the
tracer doesn't
- * issue the skip and we fall into trace_exit with x0
preserved.
- *
- * This is slightly odd because it also means that if a
tracer
- * sets the system call number to -1 but does not
initialise x0,
- * then x0 will be preserved for all system calls apart
from a
- * user-issued syscall(-1). However, requesting a skip
and not
- * setting the return value is unlikely to do anything
sensible
- * anyway.
- */
- if (scno == NO_SYSCALL)
- syscall_set_return_value(current, regs, -ENOSYS, 0);
if (!syscall_trace_enter(regs, work, scno))
goto trace_exit;

Quoted 3 lines

I've got a v3 that takes a different approach, so I'll send that out shortly. Jinjie, thanks for sending the selftests, but maybe we can extend them to cover the loophole above as wel?

I will update the test cases to cover the corner case it pointed out.

Best regards,
Jinjie

Quoted 2 lines

Will

author maintainer
#5 lore raw

Hi Jinjie,

Quoted 32 lines · Jinjie Ruan
Quoted 18 lines · Will Deacon
Quoted 10 lines · Will Deacon
Quoted 9 lines · Will Deacon

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.

Quoted 6 lines
Quoted 1 line

[...]

Applied to arm64 (for-next/fixes), thanks!

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

      https://git.kernel.org/arm64/c/e057b9477232
Quoted 7 lines

Bah, I've had to revert this. I think Sashiko makes a good point here that the seccomp interaction is still broken when the filter is re-evaluated after the tracer stop, because that all happens inside secure_computing() so we don't get a chance to update 'orig_x0':

https://sashiko.dev/#/patchset/[email protected]

Yes, I also think the point raised by Sashiko is meaningful.

After reviewing the relevant code, I believe that the issue Sashiko pointed out regarding the compat task also exists.

Quoted 8 lines

My confusion is that on arm64 compat mode, both audit and trace use orig_x0, but the first parameter used for executing system calls is regs->reg[0]. If x0 is modified at the system call entry point in ptrace but orig_x0 is not modified, or if orig_x0 is modified but x0 is not, then the first parameter for audit and the actual system call being executed will be out of sync. Could there be any issues here? Is it the responsibility of ptrace to ensure that orig_x0 and x0 are synchronized at the system call entry point on arm64 compat mode?

Even with orig_r0, I think compat suffers from some similar issues here. However, I really don't want to diverge from the arch/arm/ behaviour, so the 32-bit code should be fixed first if we want to change anything there. Having said that, I notice we're already different in how we invoke audit_syscall_entry() :(

I also don't know what the correct behaviour should be when both r0 and orig_r0 are exposed to the tracer. I have a horrible feeling it probably all worked until the API in syscall.h came along, at which point orig_r0 was suddenly used for more than restarting.

Quoted 12 lines

Whether it is ptrace or the kernel, ensuring that orig_x0 and x0 are synchronized at the entry point of a system call, I believe it is reasonable to use orig_x0 as the first parameter of the system call execution and pre-set an error code in advance, as this way orig_x0 always retains the latest value of x0.

arch/arm64/include/asm/syscall_wrapper.h +0 -0
index abb57bc54305..6b13d7c8ad95 100644

arch/arm64/include/asm/syscall_wrapper.h line 16

Quoted 6 lines
arch/arm64/include/asm/syscall_wrapper.h +1 -1
@@ -12,7 +12,7 @@
 
 #define SC_ARM64_REGS_TO_ARGS(x, ...)                          \
        __MAP(x,__SC_ARGS                                       \
-             ,,regs->regs[0],,regs->regs[1],,regs->regs[2]     \
+             ,,regs->orig_x0,,regs->regs[1],,regs->regs[2]     \
              ,,regs->regs[3],,regs->regs[4],,regs->regs[5])

I quite like this idea, but I think we'll have to limit it to native (64-bit) tasks. The 32-bit code in arch/arm/ looks like it passes regs[0] for the first argument and so tracers could easily be relying on that.

I'll do that as a follow-up patch.

Quoted 11 lines
arch/arm64/include/asm/syscall_wrapper.h +0 -0
 #ifdef CONFIG_COMPAT
arch/arm64/kernel/syscall.c +0 -0
index 2535cae9413d..a978bab59924 100644
@@ -62,6 +62,7 @@ static __always_inline void el0_svc_common(struct

pt_regs *regs, int scno, int s

        unsigned long work;

        regs->orig_x0 = regs->regs[0];

+ syscall_set_return_value(current, regs, -ENOSYS, 0);

I don't think it's safe to do this unconditionally as tracers could be relying on reading X0 with ptrace to retrieve the first syscall argument.

Will

#6 lore raw
Quoted 44 lines · Will Deacon

Hi Jinjie,

Quoted 32 lines · Jinjie Ruan
Quoted 18 lines · Will Deacon
Quoted 10 lines · Will Deacon
Quoted 9 lines · Will Deacon

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.

Quoted 6 lines
Quoted 1 line

[...]

Applied to arm64 (for-next/fixes), thanks!

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

      https://git.kernel.org/arm64/c/e057b9477232
Quoted 7 lines

Bah, I've had to revert this. I think Sashiko makes a good point here that the seccomp interaction is still broken when the filter is re-evaluated after the tracer stop, because that all happens inside secure_computing() so we don't get a chance to update 'orig_x0':

https://sashiko.dev/#/patchset/[email protected]

Yes, I also think the point raised by Sashiko is meaningful.

After reviewing the relevant code, I believe that the issue Sashiko pointed out regarding the compat task also exists.

Quoted 8 lines

My confusion is that on arm64 compat mode, both audit and trace use orig_x0, but the first parameter used for executing system calls is regs->reg[0]. If x0 is modified at the system call entry point in ptrace but orig_x0 is not modified, or if orig_x0 is modified but x0 is not, then the first parameter for audit and the actual system call being executed will be out of sync. Could there be any issues here? Is it the responsibility of ptrace to ensure that orig_x0 and x0 are synchronized at the system call entry point on arm64 compat mode?

Quoted 3 lines

Even with orig_r0, I think compat suffers from some similar issues here. However, I really don't want to diverge from the arch/arm/ behaviour, so the 32-bit code should be fixed first if we want to change anything there.

Hi Will,

I will try to clarify whether 32-bit code has similar issues.

Quoted 2 lines

Having said that, I notice we're already different in how we invoke audit_syscall_entry() :(

Yes, arm32 use regs->ARM_r0 instead of orig_r0 for audit, which is different from using orig_x0 for arm64 compat mode.

871 >-------audit_syscall_entry(scno, regs->ARM_r0, regs->ARM_r1, regs->ARM_r2,
872 >------->------->------- regs->ARM_r3);

Quoted 3 lines

I also don't know what the correct behaviour should be when both r0 and orig_r0 are exposed to the tracer. I have a horrible feeling it probably

Yes, more discussion and historical background research are needed here. I will try to look into what the correct behavior for ptrace should be in this context.

Quoted 16 lines

all worked until the API in syscall.h came along, at which point orig_r0 was suddenly used for more than restarting.

Quoted 13 lines

Whether it is ptrace or the kernel, ensuring that orig_x0 and x0 are synchronized at the entry point of a system call, I believe it is reasonable to use orig_x0 as the first parameter of the system call execution and pre-set an error code in advance, as this way orig_x0 always retains the latest value of x0.

arch/arm64/include/asm/syscall_wrapper.h +0 -0
index abb57bc54305..6b13d7c8ad95 100644
@@ -12,7 +12,7 @@
 
Quoted 9 lines

arch/arm64/include/asm/syscall_wrapper.h line 16

Quoted 5 lines
arch/arm64/include/asm/syscall_wrapper.h +1 -1
 #define SC_ARM64_REGS_TO_ARGS(x, ...)                          \
        __MAP(x,__SC_ARGS                                       \
-             ,,regs->regs[0],,regs->regs[1],,regs->regs[2]     \
+             ,,regs->orig_x0,,regs->regs[1],,regs->regs[2]     \
              ,,regs->regs[3],,regs->regs[4],,regs->regs[5])

I quite like this idea, but I think we'll have to limit it to native (64-bit) tasks. The 32-bit code in arch/arm/ looks like it passes regs[0] for the first argument and so tracers could easily be relying on

Yes, compat mode uses regs[0], which is consistent with 32-bit code behavior, so it is reasonable to only modify 64-bit code.

Quoted 3 lines

that.

I'll do that as a follow-up patch.

Thank you for doing this.

Best regards,
Jinjie

Quoted 17 lines
Quoted 8 lines
arch/arm64/include/asm/syscall_wrapper.h +0 -0
 #ifdef CONFIG_COMPAT
arch/arm64/kernel/syscall.c +0 -0
index 2535cae9413d..a978bab59924 100644
@@ -62,6 +62,7 @@ static __always_inline void el0_svc_common(struct

pt_regs *regs, int scno, int s

Quoted 4 lines
        unsigned long work;

        regs->orig_x0 = regs->regs[0];

+ syscall_set_return_value(current, regs, -ENOSYS, 0);

I don't think it's safe to do this unconditionally as tracers could be relying on reading X0 with ptrace to retrieve the first syscall argument.

Will