Skip to contentSkip to conversation

arm64: Rework valid_user_regs

PATCH Superseded Mark Rutland 3 participants 3 messages Feb 12, 2016

Subsystems (2)
arm64 ptrace support

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Mark Rutland

author reviewer
#0 lore raw

PATCH v2 1/1 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      | 74 +++++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/signal.c      |  4 +--
 arch/arm64/kernel/signal32.c    |  2 +-
 4 files changed, 78 insertions(+), 34 deletions(-)

Since v1 [1]
* Clear the full complement of RES0 bits
* Treat IL as RES0
* Treat SS as RES0 when not tracing
* renames: s/aa64/native, s/aa32/compat/

Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/406424.html

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 +72 -2

89 lines not on this page · raw message

arch/arm64/kernel/signal.c +2 -2
index e18c48c..a8eafdb 100644
@@ -115,7 +115,7 @@ static int restore_sigframe(struct pt_regs *regs,
 	 */
 	regs->syscallno = ~0UL;
 
-	err |= !valid_user_regs(&regs->user_regs);
+	err |= !valid_user_regs(&regs->user_regs, current);
 
 	if (err == 0) {
 		struct fpsimd_context *fpsimd_ctx =

@@ -307,7 +307,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	/*
 	 * Check that the resulting registers are actually sane.
 	 */
-	ret |= !valid_user_regs(&regs->user_regs);
+	ret |= !valid_user_regs(&regs->user_regs, current);
 
 	/*
 	 * Fast forward the stepping logic so we step into the signal
arch/arm64/kernel/signal32.c +1 -1
index 71ef6dc..1073356 100644
@@ -356,7 +356,7 @@ static int compat_restore_sigframe(struct pt_regs *regs,
 	 */
 	regs->syscallno = ~0UL;
 
-	err |= !valid_user_regs(&regs->user_regs);
+	err |= !valid_user_regs(&regs->user_regs, current);
 
 	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
 	if (err == 0)
Signature · 1 line
1.9.1
#1 lore raw

PATCH v2 1/1 arm64: Rework valid_user_regs

Quoted 17 lines · Mark Rutland

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).

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

Quoted 5 lines
arch/arm64/kernel/ptrace.c +5 -0
+       /* Force PSR to a valid 32-bit EL0t */
+       regs->pstate |= PSR_MODE32_BIT;
+       regs->pstate &= PSR_f | PSR_s | (PSR_x & ~COMPAT_PSR_A_BIT) | \
+                       COMPAT_PSR_T_BIT;
+

I think the consensus from the v1 review was to drop these PSR_[fsx] masks?

thanks
-- PMM

#2 lore raw

PATCH v2 1/1 arm64: Rework valid_user_regs

Quoted 18 lines · Peter Maydell
Quoted 16 lines · Mark Rutland

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).

Quoted 7 lines

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

Quoted 5 lines
arch/arm64/kernel/ptrace.c +5 -0
+       /* Force PSR to a valid 32-bit EL0t */
+       regs->pstate |= PSR_MODE32_BIT;
+       regs->pstate &= PSR_f | PSR_s | (PSR_x & ~COMPAT_PSR_A_BIT) | \
+                       COMPAT_PSR_T_BIT;
+

I think the consensus from the v1 review was to drop these PSR_[fsx] masks?

Yup. Mark, if you respin with that change then I can take this as a fix.

Thanks,

Will