arm64: syscall: Ensure saved x0 is kept in-sync with tracer updates
Subsystems (2)
Will Deacon
maintainerPATCH 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).
---
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
@@ -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
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 ina5cd110cb836("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 bysyscall_get_arguments(). Unlike 32-bit Arm, this stashed value is not directly exposed viaptrace()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_ENTRYorSECCOMP_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-ENOSYSin that case).Cc Kees Cook <[email protected]>Cc Jinjie Ruan <[email protected]>Cc Mark Rutland <[email protected]>Reported-by Yiqi Sun <[email protected]>Suggested-by Catalin Marinas <[email protected]>Fixes a5cd110cb836 ("arm64/ptrace: run seccomp after ptrace")Signed-off-by Will Deacon <[email protected]>---
Changes since v1 (https://lore.kernel.org/r/[email protected]):
* Reworded and expanded comments and commit message per Jinjie's feedbackarch/arm64/kernel/ptrace.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)arch/arm64/kernel/ptrace.c +22
-0@@ -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);
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 ina5cd110cb836("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
Will Deacon
maintainerQuoted 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 ina5cd110cb836("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
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 ina5cd110cb836("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':
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
@@ -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
Will Deacon
maintainerHi 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 ina5cd110cb836("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/e057b9477232Quoted 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':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 butorig_x0is not modified, or iforig_x0is 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 thatorig_x0and 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_x0and x0 are synchronized at the entry point of a system call, I believe it is reasonable to useorig_x0as the first parameter of the system call execution and pre-set an error code in advance, as this wayorig_x0always retains the latest value of x0.arch/arm64/include/asm/syscall_wrapper.h +0
-0
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@@ -62,6 +62,7 @@ static __always_inline void el0_svc_common(struct
pt_regs*regs, int scno, int sunsigned 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
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 ina5cd110cb836("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/e057b9477232Quoted 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':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 butorig_x0is not modified, or iforig_x0is 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 thatorig_x0and 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 thearch/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_r0are 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.hcame along, at which pointorig_r0was suddenly used for more than restarting.Quoted 13 lines
Whether it is ptrace or the kernel, ensuring that
orig_x0and x0 are synchronized at the entry point of a system call, I believe it is reasonable to useorig_x0as the first parameter of the system call execution and pre-set an error code in advance, as this wayorig_x0always retains the latest value of x0.arch/arm64/include/asm/syscall_wrapper.h +0
-0@@ -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@@ -62,6 +62,7 @@ static __always_inline void el0_svc_common(struct
pt_regs*regs, int scno, int sQuoted 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