arm64: Rework valid_user_regs

We validate pstate using PSR_MODE32_BIT, which is part of the user-provided pstate (and cannot be trusted). Also, we conflate validation of AArch32 and AArch64 pstate values, making the code difficult to reason about.

Instead, validate the pstate value based on the associated task. The task may or may not be current (e.g. when using ptrace), so this must be passed explicitly by callers. To avoid circular header dependencies via sched.h, is_compat_task is pulled out of asm/ptrace.h.

To make the code possible to reason about, the AArch64 and AArch32 validation is split into separate functions. Software must respect the RES0 policy for SPSR bits, and thus the kernel mirrors the hardware policy (RAZ/WI) for bits as-yet unallocated. When these acquire an architected meaning writes may be permitted (potentially with additional validation).

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Dave Martin <[email protected]>

Cc James Morse <[email protected]>

Cc Peter Maydell <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/ptrace.h | 32 ++-------------------
 arch/arm64/kernel/ptrace.c      | 64 +++++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/signal.c      |  4 +--
 arch/arm64/kernel/signal32.c    |  2 +-
 4 files changed, 68 insertions(+), 34 deletions(-)

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

index e9e5467..0bb538b 100644
@@ -151,35 +151,9 @@ static inline unsigned long regs_return_value(struct pt_regs *regs)
 	return regs->regs[0];
 }
 
-/*
- * Are the current registers suitable for user mode? (used to maintain
- * security in signal handlers)
- */
-static inline int valid_user_regs(struct user_pt_regs *regs)
-{
-	if (user_mode(regs) && (regs->pstate & PSR_I_BIT) == 0) {
-		regs->pstate &= ~(PSR_F_BIT | PSR_A_BIT);
-
-		/* The T bit is reserved for AArch64 */
-		if (!(regs->pstate & PSR_MODE32_BIT))
-			regs->pstate &= ~COMPAT_PSR_T_BIT;
-
-		return 1;
-	}
-
-	/*
-	 * Force PSR to something logical...
-	 */
-	regs->pstate &= PSR_f | PSR_s | (PSR_x & ~PSR_A_BIT) | \
-			COMPAT_PSR_T_BIT | PSR_MODE32_BIT;
-
-	if (!(regs->pstate & PSR_MODE32_BIT)) {
-		regs->pstate &= ~COMPAT_PSR_T_BIT;
-		regs->pstate |= PSR_MODE_EL0t;
-	}
-
-	return 0;
-}
+/* We must avoid circular header include via sched.h */
+struct task_struct;
+int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task);
 
 #define instruction_pointer(regs)	((unsigned long)(regs)->pc)
 

arch/arm64/kernel/ptrace.c +3 -2

index ff7f132..23c20c2 100644
@@ -500,7 +500,7 @@ static int gpr_set(struct task_struct *target, const struct user_regset *regset,
 	if (ret)
 		return ret;
 
-	if (!valid_user_regs(&newregs))
+	if (!valid_user_regs(&newregs, target))
 		return -EINVAL;
 
 	task_pt_regs(target)->user_regs = newregs;

@@ -770,7 +770,7 @@ static int compat_gpr_set(struct task_struct *target,
 
 	}
 
-	if (valid_user_regs(&newregs.user_regs))
+	if (valid_user_regs(&newregs.user_regs, target))
 		*task_pt_regs(target) = newregs;
 	else
 		ret = -EINVAL;

@@ -1272,3 +1272,63 @@ asmlinkage void syscall_trace_exit(struct pt_regs *regs)
 	if (test_thread_flag(TIF_SYSCALL_TRACE))
 		tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
 }
+

arch/arm64/kernel/ptrace.c +9 -0

+{
+	regs->pstate &= ~SPSR_EL1_AARCH32_RES0_BITS;
+
+	if (user_mode(regs) && (regs->pstate & PSR_MODE32_BIT) &&
+	    (regs->pstate & COMPAT_PSR_A_BIT) == 0 &&
+	    (regs->pstate & COMPAT_PSR_I_BIT) == 0 &&
+	    (regs->pstate & COMPAT_PSR_F_BIT) == 0) {
+		return 1;
+	}

Hi Mark,

arch/arm64/kernel/ptrace.c asmlinkage void syscall_trace_exit(struct pt_regs *regs) · line 1285

Quoted 10 lines · Mark Rutland

arch/arm64/kernel/ptrace.c +10 -0

+/*
+ * Bits which are architecturally RES0 per ARM DDI 0487A.h
+ * The kernel matches the HW policy. Userspace cannot use these until they have
+ * an architectural meaning.
+ */
+#define SPSR_EL1_AARCH64_RES0_BITS \
+	(GENMASK_ULL(63,32) | GENMASK_ULL(27, 22) | GENMASK_ULL(19, 10) | \
+	 GENMASK_ULL(5, 5))
+#define SPSR_EL1_AARCH32_RES0_BITS \
+	(GENMASK_ULL(63,32) | (GENMASK_ULL(23, 22)))

The J bit (24) is also RES0 and the E bit (9) can be RES0/RES1 depending on what the CPU supports.

From a quick look, J is always RES0 for ARMv8. I'll add that to the

It also might be worth negating these in the definition, rather than on every use.

Quoted 1 line

+static int valid_aa32_regs(struct user_pt_regs *regs)

valid_compat_user_regs

Quoted 1 line

+static int valid_aa64_regs(struct user_pt_regs *regs)

valid_native_user_regs

Quoted 6 lines

arch/arm64/kernel/ptrace.c +6 -0

+{
+	regs->pstate &= ~SPSR_EL1_AARCH64_RES0_BITS;
+
+	if (user_mode(regs) && !(regs->pstate & PSR_MODE32_BIT) &&
+	    (regs->pstate & PSR_D_BIT) == 0 &&
+	    (regs->pstate & PSR_A_BIT) == 0 &&

arch/arm64/kernel/ptrace.c asmlinkage void syscall_trace_exit(struct pt_regs *regs) · line 1316

Quoted 4 lines

arch/arm64/kernel/ptrace.c +4 -0

+	    (regs->pstate & PSR_I_BIT) == 0 &&
+	    (regs->pstate & PSR_F_BIT) == 0) {
+		return 1;
+	}

I think we should err on the side of caution and nuke SS and IL for both native and compat too, although that seems a odds with the PSR_s mask. I wonder how relevant those PSR groups are in ARMv8...

arch/arm64/kernel/ptrace.c asmlinkage void syscall_trace_exit(struct pt_regs *regs) · line 1330

Quoted 3 lines

arch/arm64/kernel/ptrace.c +3 -0

+int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
+{
+	if (is_compat_thread(task_thread_info(task)))

is_compat_task

Quoted 3 lines · Will Deacon

I think we should err on the side of caution and nuke SS and IL for both native and compat too, although that seems a odds with the PSR_s mask. I wonder how relevant those PSR groups are in ARMv8...

Ok.

I couldn't spot anything in the ARM ARM regarding PSR bit groups,; I was cargo-culting from the existing code. I'm more than happy to make the PSR_* groups an aarch32/compat thing.

Quoted 5 lines · Mark Rutland

Quoted 3 lines · Will Deacon

I think we should err on the side of caution and nuke SS and IL for both native and compat too, although that seems a odds with the PSR_s mask. I wonder how relevant those PSR groups are in ARMv8...

Quoted 1 line

Ok.

If you nuke SS does that have any side effects in the case of (for instance) interactions between ptrace single step and ptrace syscall tracing? (ie do we ever end up in a situation where the ptracer can read a PSR for the debuggee which has SS set? if so then it should be able to write back the PSR it has just read without any bits being unset.)

Clearing IL should be ok, though it's pretty harmless for the user process to have IL set, it will just cause an exception. (Userspace can't end up with IL set unless we allow it to by doing an exception-return to EL0 with an IL-set SPSR.)

Quoted 3 lines

I couldn't spot anything in the ARM ARM regarding PSR bit groups,; I was cargo-culting from the existing code. I'm more than happy to make the PSR_* groups an aarch32/compat thing.

It's not clear to me that they make much sense for 32-bit either. NZCVQ are in PSR_f, but GE are in PSR_s. I and F are in PSR_c but A is in PSR_x. Presumably we need to leave them in the header file for back-compat with userspace, but I suspect any kernel code using them would benefit from using constants that more clearly reflect what it's doing.

(For instance, why do we clear NZCVQ on entry to a signal handler but not GE? Harmless, since the calling convention doesn't require any particular value for any of those flags on function entry, but an odd inconsistency.)

Quoted 6 lines · Peter Maydell

Quoted 5 lines · Mark Rutland

Quoted 3 lines · Will Deacon

I think we should err on the side of caution and nuke SS and IL for both native and compat too, although that seems a odds with the PSR_s mask. I wonder how relevant those PSR groups are in ARMv8...

Quoted 8 lines

Quoted 1 line

Ok.

If you nuke SS does that have any side effects in the case of (for instance) interactions between ptrace single step and ptrace syscall tracing? (ie do we ever end up in a situation where the ptracer can read a PSR for the debuggee which has SS set? if so then it should be able to write back the PSR it has just read without any bits being unset.)

I don't think so -- the signal dispatch logic "fast-forwards" the stepping state machine so that we step into the signal handler, therefore the SS bit should always be clear on entry afaict.

Quoted 16 lines · Will Deacon

Quoted 14 lines · Peter Maydell

Quoted 6 lines · Mark Rutland

Quoted 3 lines · Will Deacon

I think we should err on the side of caution and nuke SS and IL for both native and compat too, although that seems a odds with the PSR_s mask. I wonder how relevant those PSR groups are in ARMv8...

Ok.

If you nuke SS does that have any side effects in the case of (for instance) interactions between ptrace single step and ptrace syscall tracing? (ie do we ever end up in a situation where the ptracer can read a PSR for the debuggee which has SS set? if so then it should be able to write back the PSR it has just read without any bits being unset.)

Quoted 3 lines

I don't think so -- the signal dispatch logic "fast-forwards" the stepping state machine so that we step into the signal handler, therefore the SS bit should always be clear on entry afaict.

That handles entry, but what about exit?

Is there are a guarantee that we won't call user_enable_single_step() if the return path is traced?

Quoted 21 lines · Mark Rutland

Quoted 19 lines · Will Deacon

Quoted 6 lines · Peter Maydell

Quoted 5 lines · Mark Rutland

Quoted 3 lines · Will Deacon

I think we should err on the side of caution and nuke SS and IL for both native and compat too, although that seems a odds with the PSR_s mask. I wonder how relevant those PSR groups are in ARMv8...

Quoted 8 lines

Quoted 1 line

Ok.

If you nuke SS does that have any side effects in the case of (for instance) interactions between ptrace single step and ptrace syscall tracing? (ie do we ever end up in a situation where the ptracer can read a PSR for the debuggee which has SS set? if so then it should be able to write back the PSR it has just read without any bits being unset.)

I don't think so -- the signal dispatch logic "fast-forwards" the stepping state machine so that we step into the signal handler, therefore the SS bit should always be clear on entry afaict.

Quoted 4 lines

That handles entry, but what about exit?

Is there are a guarantee that we won't call user_enable_single_step() if the return path is traced?

Why would that be a problem? I think I'm missing your point...

Quoted 27 lines · Will Deacon

Quoted 25 lines · Mark Rutland

Quoted 16 lines · Will Deacon

Quoted 6 lines · Peter Maydell

Quoted 5 lines · Mark Rutland

Quoted 3 lines · Will Deacon

I think we should err on the side of caution and nuke SS and IL for both native and compat too, although that seems a odds with the PSR_s mask. I wonder how relevant those PSR groups are in ARMv8...

Quoted 8 lines

Quoted 1 line

Ok.

If you nuke SS does that have any side effects in the case of (for instance) interactions between ptrace single step and ptrace syscall tracing? (ie do we ever end up in a situation where the ptracer can read a PSR for the debuggee which has SS set? if so then it should be able to write back the PSR it has just read without any bits being unset.)

Quoted 3 lines

I don't think so -- the signal dispatch logic "fast-forwards" the stepping state machine so that we step into the signal handler, therefore the SS bit should always be clear on entry afaict.

That handles entry, but what about exit?

Is there are a guarantee that we won't call user_enable_single_step() if the return path is traced?

Quoted 1 line

Why would that be a problem? I think I'm missing your point...

We would nuke the SS bit if the tracing happens after user_enable_single_step, if the tracer fiddled with pstate at all. So you wouldn't get the single stepping you expected.

Maybe I'm missing some reason this is prevented by construction.