| Message ID | 1490370933-24057-1-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dave, On Fri, Mar 24, 2017 at 03:55:33PM +0000, Dave Martin wrote: > 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. 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
On Mon, Apr 03, 2017 at 10:42:38AM +0100, Will Deacon wrote: > Hi Dave, > > On Fri, Mar 24, 2017 at 03:55:33PM +0000, Dave Martin wrote: > > 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. > > 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 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. > 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. 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
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index c97b8bd..0502007 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -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; } diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index c142459..d92b422 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -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; } diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 49c30df..1aef3d7 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -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 diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c index c747a0f..53f1cc0 100644 --- a/arch/arm64/kernel/signal32.c +++ b/arch/arm64/kernel/signal32.c @@ -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);
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. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- 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(-)