arm64: Abstract syscallno manipulation

The upper 32 bits of the syscallno field in struct pt_regs are handled inconsistently, being sometimes zero extended and sometimes sign-extended. The reason for this appears to be that they are not intentionally significant at all.

Hi,

Quoted 5 lines · On Tue, 18 Jul 2017 13:41:43 +0100 Dave Martin wrote:

The upper 32 bits of the syscallno field in struct pt_regs are handled inconsistently, being sometimes zero extended and sometimes sign-extended. The reason for this appears to be that they are not intentionally significant at all.

My english parser fails on "not intentionally significant at all".

The upper 32 bits of the syscallno field in struct pt_regs are handled inconsistently, being sometimes zero extended and sometimes sign-extended. The reason for this appears to be that they are not intentionally significant at all.

Currently, the only place I can find where those bits are significant is in calling trace_sys_enter(), which may be
unintentional: for example, if a compat tracer attempts to cancel a syscall by passing -1 to (COMPAT_)PTRACE_SET_SYSCALL at the syscall-enter-stop, it will be traced as syscall 4294967295 rather than -1 as might be expected (and as occurs for a native tracer doing the same thing). Elsewhere, reads of syscallno cast it to an int or truncate it.

There's also a conspicuous amount of code and casting to bodge around the fact that although semantically an int, syscallno is stored as a u64.

Let's not pretend any more.

In order to preserve the stp x instruction that stores the syscall number in entry.S, this patch special-cases the layout of struct pt_regs for big endian so that the newly 32-bit syscallno field maps onto the low bits of the stored value. This is not beautiful, but benchmarking of the getpid syscall on Juno suggests indicates a minor slowdown if the stp is split into an stp x and stp w.

Signed-off-by Dave Martin <[email protected]>

---
 arch/arm64/include/asm/processor.h |  2 +-
 arch/arm64/include/asm/ptrace.h    |  9 ++++++++-
 arch/arm64/kernel/entry.S          | 34 +++++++++++++++++-----------------
 arch/arm64/kernel/ptrace.c         |  2 +-
 arch/arm64/kernel/signal.c         |  6 +++---
 arch/arm64/kernel/signal32.c       |  2 +-
 arch/arm64/kernel/traps.c          |  2 +-
 7 files changed, 32 insertions(+), 25 deletions(-)

arch/arm64/include/asm/processor.h +1 -1

index 64c9e78..379def1 100644
@@ -112,7 +112,7 @@ void tls_preserve_current_state(void);
 static inline void start_thread_common(struct pt_regs *regs, unsigned long pc)
 {
 	memset(regs, 0, sizeof(*regs));
-	regs->syscallno = ~0UL;
+	regs->syscallno = ~0;
 	regs->pc = pc;
 }
 

arch/arm64/include/asm/ptrace.h +0 -1

index 11403fd..21c87dc 100644
@@ -116,7 +116,14 @@ struct pt_regs {
 		};
 	};
 	u64 orig_x0;
-	u64 syscallno;

Quoted 28 lines · Dave Martin

The upper 32 bits of the syscallno field in struct pt_regs are handled inconsistently, being sometimes zero extended and sometimes sign-extended. The reason for this appears to be that they are not intentionally significant at all.

Currently, the only place I can find where those bits are significant is in calling trace_sys_enter(), which may be
unintentional: for example, if a compat tracer attempts to cancel a syscall by passing -1 to (COMPAT_)PTRACE_SET_SYSCALL at the syscall-enter-stop, it will be traced as syscall 4294967295 rather than -1 as might be expected (and as occurs for a native tracer doing the same thing). Elsewhere, reads of syscallno cast it to an int or truncate it.

There's also a conspicuous amount of code and casting to bodge around the fact that although semantically an int, syscallno is stored as a u64.

Let's not pretend any more.

In order to preserve the stp x instruction that stores the syscall number in entry.S, this patch special-cases the layout of struct pt_regs for big endian so that the newly 32-bit syscallno field maps onto the low bits of the stored value. This is not beautiful, but benchmarking of the getpid syscall on Juno suggests indicates a minor slowdown if the stp is split into an stp x and stp w.

Signed-off-by Dave Martin <[email protected]>

Quoted 14 lines

arch/arm64/include/asm/ptrace.h +5 -1

index 11403fd..21c87dc 100644
@@ -116,7 +116,14 @@ struct pt_regs {
 		};
 	};
 	u64 orig_x0;
-	u64 syscallno;
+#ifdef __AARCH64EB__
+	u32 unused2;
+	s32 syscallno;
+#else
+	s32 syscallno;

arch/arm64/include/asm/ptrace.h struct pt_regs { · line 128

Quoted 5 lines

arch/arm64/include/asm/ptrace.h +3 -0

+	u32 unused2;
+#endif
+
 	u64 orig_addr_limit;
 	u64 unused;	// maintain 16 byte alignment

This isn't a UAPI struct, so we could juggle these about a bit to put all the unused padding at the end.

The -1 "no syscall" value is written in various ways, shared with the user ABI in some places, and generally obscure.

This patch attempts to make things a little more consistent and readable by replacing all these uses with a single #define. A couple of symbolic helpers are provided to clarify the intent further.

Because the in-syscall check in do_signal() is changed from >= 0 to != NO_SYSCALL by this patch, different behaviour may be observable if syscallno is set to values less than -1 by a tracer. However, this is not different from the behaviour that is already observable if a tracer sets syscallno to a value >= __NR_(compat_)syscalls.

It appears that this can cause spurious syscall restarting, but that is not a new behaviour either, and does not appear harmful.

Signed-off-by Dave Martin <[email protected]>

---
 arch/arm64/include/asm/processor.h |  2 +-
 arch/arm64/include/asm/ptrace.h    | 21 +++++++++++++++++++++
 arch/arm64/kernel/entry.S          | 10 ++++------
 arch/arm64/kernel/ptrace.c         |  2 +-
 arch/arm64/kernel/signal.c         |  8 ++++----
 arch/arm64/kernel/signal32.c       |  2 +-
 6 files changed, 32 insertions(+), 13 deletions(-)

arch/arm64/include/asm/processor.h +1 -1

index 379def1..b7334f1 100644
@@ -112,7 +112,7 @@ void tls_preserve_current_state(void);
 static inline void start_thread_common(struct pt_regs *regs, unsigned long pc)
 {
 	memset(regs, 0, sizeof(*regs));
-	regs->syscallno = ~0;
+	forget_syscall(regs);
 	regs->pc = pc;
 }
 

arch/arm64/include/asm/ptrace.h +11 -0

index 21c87dc..3a2d6cc 100644
@@ -72,8 +72,19 @@
 #define COMPAT_PT_TEXT_ADDR		0x10000
 #define COMPAT_PT_DATA_ADDR		0x10004
 #define COMPAT_PT_TEXT_END_ADDR		0x10008
+
+/*
+ * If pt_regs.syscallno == NO_SYSCALL, then the thread is not executing
+ * a syscall -- i.e., its most recent entry into the kernel from
+ * userspace was not via SVC, or otherwise a tracer cancelled the syscall.
+ *
+ * This must have the value -1, for ABI compatibility with ptrace etc.
+ */
+#define NO_SYSCALL (-1)
+
 #ifndef __ASSEMBLY__
 #include <linux/bug.h>
+#include <linux/types.h>
 
 /* sizeof(struct user) for AArch32 */
 #define COMPAT_USER_SZ	296

arch/arm64/kernel/signal.c +1 -1

index 4d04b89..3a59dae 100644
@@ -387,7 +387,7 @@ static int restore_sigframe(struct pt_regs *regs,
 	/*
 	 * Avoid sys_rt_sigreturn() restarting.
 	 */
-	regs->syscallno = ~0;
+	forget_syscall(regs);
 
 	err |= !valid_user_regs(&regs->user_regs, current);
 	if (err == 0)