arm64: ptrace: Crystallise the pt_regs->syscallno = ~0UL idiom
Subsystems (2)
PATCH 1/1 arm64: ptrace: Crystallise the pt_regs->syscallno = ~0UL idiom
Assigning ~0UL to pt_regs->syscallno to indicate that a task is not executing a syscall is a little obscure.
This patch defines a helper zap_syscall() to make users of this idiom and its intent a bit more readable. This concept allows relaxations to the system call ABI whereby not all userspace state need be preserved by the kernel around an explicit syscall. The Scalable Vector Extension ABI will make use of this with regard to the extra register state added by SVE.
No relaxation of the _existing_ system call ABI is implied here.
--- arch/arm64/include/asm/processor.h | 7 ++++++- arch/arm64/kernel/ptrace.c | 3 ++- arch/arm64/kernel/signal.c | 4 ++-- arch/arm64/kernel/signal32.c | 3 ++- 4 files changed, 12 insertions(+), 5 deletions(-)
arch/arm64/include/asm/processor.h +6 -1
@@ -104,10 +104,15 @@ struct thread_struct {#define INIT_THREAD { } +static inline void zap_syscall (struct pt_regs *regs) +{ + regs->syscallno = ~0UL ; +} +static inline void start_thread_common (struct pt_regs *regs,unsigned long pc) {memset (regs,0 ,sizeof (*regs)); - regs->syscallno = ~0UL ; +zap_syscall (regs); regs->pc = pc; }
arch/arm64/kernel/ptrace.c +2 -1
@@ -42,6 +42,7 @@#include <asm/compat.h> #include <asm/debug-monitors.h> #include <asm/pgtable.h> +#include <asm/processor.h> #include <asm/syscall.h> #include <asm/traps.h> #include <asm/system_misc.h>
@@ -1348,7 +1349,7 @@ static void tracehook_report_syscall(struct pt_regs *regs,if (dir ==PTRACE_SYSCALL_EXIT )tracehook_report_syscall_exit (regs,0 );else if (tracehook_report_syscall_entry (regs)) - regs->syscallno = ~0UL ; +zap_syscall (regs); regs->regs[regno] = saved_reg; }
arch/arm64/kernel/signal.c +2 -2
@@ -351,7 +351,7 @@ static int restore_sigframe(struct pt_regs *regs,/* * Avoid sys_rt_sigreturn() restarting. */ - regs->syscallno = ~0UL ; +zap_syscall (regs); err |= !valid_user_regs (®s->user_regs, current);if (err ==0 )
@@ -634,7 +634,7 @@ static void do_signal(struct pt_regs *regs)/* * Avoid additional syscall restarting via ret_to_user. */ - regs->syscallno = ~0UL ; +zap_syscall (regs);/* * Prepare for system call restart. We do this here so that a
arch/arm64/kernel/signal32.c +2 -1
@@ -27,6 +27,7 @@#include <asm/fpsimd.h> #include <asm/signal32.h> #include <linux/uaccess.h> +#include <asm/processor.h> #include <asm/unistd.h> struct compat_sigcontext {
@@ -354,7 +355,7 @@ static int compat_restore_sigframe(struct pt_regs *regs,/* * Avoid compat_sys_sigreturn() restarting. */ - regs->syscallno = ~0UL ; +zap_syscall (regs); err |= !valid_user_regs (®s->user_regs, current);
Signature · 1 line
2.1.4
PATCH 1/1 arm64: ptrace: Crystallise the pt_regs->syscallno = ~0UL idiom
Hi Dave,
Quoted 11 lines · Dave Martin
Assigning ~0UL to
pt_regs->syscallnoto indicate that a task is not executing a syscall is a little obscure.This patch defines a helper
zap_syscall()to make users of this idiom and its intent a bit more readable. This concept allows relaxations to the system call ABI whereby not all userspace state need be preserved by the kernel around an explicit syscall. The Scalable Vector Extension ABI will make use of this with regard to the extra register state added by SVE.No relaxation of the _existing_ system call ABI is implied here.
Whilst I'm not against this cleanup, I'm also not sure that it goes far enough. For example, lib/syscall.c treats syscall_get_nr returning '-1L' as "not in syscall" and it also looks like negative values can propagate into seccomp and audit via the same "syscall_get_nr" interface.
So I worry that wrapping this up in "zap_syscall" hides the fact that the value being set is so important (it's even used in entry.S!), but without changing the code that actually reads and checks against the magic value. I'd sooner add zap_syscall to asm/syscall.h alongside something like "in_syscall", which can be used instead of open-coded checks.
What do you reckon?
Will
PATCH 1/1 arm64: ptrace: Crystallise the pt_regs->syscallno = ~0UL idiom
Quoted 15 lines · Will Deacon
Hi Dave,
Quoted 11 lines · Dave Martin
Assigning ~0UL to
pt_regs->syscallnoto indicate that a task is not executing a syscall is a little obscure.This patch defines a helper
zap_syscall()to make users of this idiom and its intent a bit more readable. This concept allows relaxations to the system call ABI whereby not all userspace state need be preserved by the kernel around an explicit syscall. The Scalable Vector Extension ABI will make use of this with regard to the extra register state added by SVE.No relaxation of the _existing_ system call ABI is implied here.
Quoted 7 lines
Whilst I'm not against this cleanup, I'm also not sure that it goes far enough. For example,
lib/syscall.ctreatssyscall_get_nrreturning '-1L' as "not in syscall" and it also looks like negative values can propagate into seccomp and audit via the same "syscall_get_nr" interface.So I worry that wrapping this up in "
zap_syscall" hides the fact that the value being set is so important (it's even used inentry.S!), but
I can at least add a comment to say that -1UL has a specific meaning to the core code and can't be changed without breaking things elsewhere.
Quoted 3 lines
without changing the code that actually reads and checks against the magic value. I'd sooner add
zap_syscalltoasm/syscall.halongside something like "in_syscall", which can be used instead of open-coded checks.
I'm happy to move this to asm/syscall.h, and use it for the syscallno poisoning in entry.S:kernel_entry. (This is zap_syscall, but trying to unify this between C and asm seems going a step too far.)
Cheers
---Dave