| Message ID | 1455041501-30018-1-git-send-email-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mark, On Tue, Feb 09, 2016 at 06:11:41PM +0000, Mark Rutland wrote: > 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). Thanks for doing this. Comments inline. > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Dave Martin <dave.martin@arm.com> > Cc: James Morse <james.morse@arm.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Will Deacon <will.deacon@arm.com> > --- > 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(-) > > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h > index e9e5467..0bb538b 100644 > --- a/arch/arm64/include/asm/ptrace.h > +++ b/arch/arm64/include/asm/ptrace.h > @@ -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) > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > index ff7f132..23c20c2 100644 > --- a/arch/arm64/kernel/ptrace.c > +++ b/arch/arm64/kernel/ptrace.c > @@ -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); > } > + > +/* > + * 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. It also might be worth negating these in the definition, rather than on every use. > +static int valid_aa32_regs(struct user_pt_regs *regs) valid_compat_user_regs > +{ > + 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; > + } > + > + /* 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; > + > + return 0; > +} > + > +static int valid_aa64_regs(struct user_pt_regs *regs) valid_native_user_regs > +{ > + 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 && > + (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... > + /* Force PSR to a valid 64-bit EL0t */ > + regs->pstate &= PSR_f | PSR_s; > + > + return 0; > +} > + > +/* > + * Are the current registers suitable for user mode? (used to maintain > + * security in signal handlers) > + */ > +int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task) > +{ > + if (is_compat_thread(task_thread_info(task))) is_compat_task Will
On Wed, Feb 10, 2016 at 11:58:53AM +0000, Will Deacon wrote: > Hi Mark, > > On Tue, Feb 09, 2016 at 06:11:41PM +0000, Mark Rutland wrote: > > +/* > > + * 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 mask. E is a bit more complex; I'll see about handling that dynamically in valid_compat_user_regs. > It also might be worth negating these in the definition, rather than > on every use. Does that make anything clearer? They're constants which get used once each, and SPSR_EL1_AARCH*_MASK wouldn't make it clear what the bits actually are. If you have a better suggestion for a name I'm happy to change it. > > +static int valid_aa32_regs(struct user_pt_regs *regs) > > valid_compat_user_regs Ok > > +static int valid_aa64_regs(struct user_pt_regs *regs) > > valid_native_user_regs Ok. > > +{ > > + 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 && > > + (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... 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. > > +int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task) > > +{ > > + if (is_compat_thread(task_thread_info(task))) > > is_compat_task Can't do; is_compat_task() takes no task_struct and assumes we're asking about current (which doesn't work for the ptrace cases). We could add a new helper for this, but I think that should be future cleanup as we'd want to alter the other uses: [mark@leverpostej:~/src/linux]% git grep is_compat_thread origin/master origin/master:arch/arm64/include/asm/compat.h:static inline int is_compat_thread(struct thread_info *thread) origin/master:arch/arm64/include/asm/compat.h:static inline int is_compat_thread(struct thread_info *thread) origin/master:arch/arm64/include/asm/processor.h: if (is_compat_thread(task_thread_info(t))) \ origin/master:arch/arm64/kernel/hw_breakpoint.c: return tsk && is_compat_thread(task_thread_info(tsk)); origin/master:arch/arm64/kernel/perf_regs.c: if (is_compat_thread(task_thread_info(task))) origin/master:arch/arm64/kernel/process.c: if (is_compat_thread(task_thread_info(p))) origin/master:arch/arm64/kernel/process.c: tpidrro = is_compat_thread(task_thread_info(next)) ? origin/master:arch/arm64/kernel/ptrace.c: else if (is_compat_thread(task_thread_info(task))) I'm happy to look at cleaning that up as a later patch/series. Thanks, Mark.
On 10 February 2016 at 12:31, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Feb 10, 2016 at 11:58:53AM +0000, Will Deacon wrote: >> 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.) 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.) > 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.) thanks -- PMM
On Wed, Feb 10, 2016 at 02:23:29PM +0000, Peter Maydell wrote: > On 10 February 2016 at 12:31, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Feb 10, 2016 at 11:58:53AM +0000, Will Deacon wrote: > >> 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.) 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. > 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.) I was just musing about potential hardware bugs and thought it cleaner /safer to clear those bits. > > 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.) No idea. This whole area is pretty crufty, so we could probably clean that up while we're here. Will
On Wed, Feb 10, 2016 at 02:43:24PM +0000, Will Deacon wrote: > On Wed, Feb 10, 2016 at 02:23:29PM +0000, Peter Maydell wrote: > > On 10 February 2016 at 12:31, Mark Rutland <mark.rutland@arm.com> wrote: > > > On Wed, Feb 10, 2016 at 11:58:53AM +0000, Will Deacon wrote: > > >> 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.) > > 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? Mark.
On Wed, Feb 10, 2016 at 04:01:27PM +0000, Mark Rutland wrote: > On Wed, Feb 10, 2016 at 02:43:24PM +0000, Will Deacon wrote: > > On Wed, Feb 10, 2016 at 02:23:29PM +0000, Peter Maydell wrote: > > > On 10 February 2016 at 12:31, Mark Rutland <mark.rutland@arm.com> wrote: > > > > On Wed, Feb 10, 2016 at 11:58:53AM +0000, Will Deacon wrote: > > > >> 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.) > > > > 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? Why would that be a problem? I think I'm missing your point... Will
On Wed, Feb 10, 2016 at 04:04:21PM +0000, Will Deacon wrote: > On Wed, Feb 10, 2016 at 04:01:27PM +0000, Mark Rutland wrote: > > On Wed, Feb 10, 2016 at 02:43:24PM +0000, Will Deacon wrote: > > > On Wed, Feb 10, 2016 at 02:23:29PM +0000, Peter Maydell wrote: > > > > On 10 February 2016 at 12:31, Mark Rutland <mark.rutland@arm.com> wrote: > > > > > On Wed, Feb 10, 2016 at 11:58:53AM +0000, Will Deacon wrote: > > > > >> 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.) > > > > > > 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? > > 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. Mark.
On Wed, Feb 10, 2016 at 04:05:50PM +0000, Mark Rutland wrote: > On Wed, Feb 10, 2016 at 04:04:21PM +0000, Will Deacon wrote: > > On Wed, Feb 10, 2016 at 04:01:27PM +0000, Mark Rutland wrote: > > > On Wed, Feb 10, 2016 at 02:43:24PM +0000, Will Deacon wrote: > > > > On Wed, Feb 10, 2016 at 02:23:29PM +0000, Peter Maydell wrote: > > > > > On 10 February 2016 at 12:31, Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > On Wed, Feb 10, 2016 at 11:58:53AM +0000, Will Deacon wrote: > > > > > >> 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.) > > > > > > > > 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? > > > > 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. Ok, for some reason I thought this was all about signal handling, but I now see Peter was referring to general ptrace register manipulation. In which case, we could force SS to zero if TIF_SINGLESTEP is not set on the tracee, otherwise letting it be set. Ultimately, MDSCR_EL1.SS prevents PSTATE.SS from taking effect if !TIF_SINGLESTEP, but it's worth cleaning this up imo. Will
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index e9e5467..0bb538b 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -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) diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index ff7f132..23c20c2 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -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); } + +/* + * 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))) + +static int valid_aa32_regs(struct user_pt_regs *regs) +{ + 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; + } + + /* 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; + + return 0; +} + +static int valid_aa64_regs(struct user_pt_regs *regs) +{ + 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 && + (regs->pstate & PSR_I_BIT) == 0 && + (regs->pstate & PSR_F_BIT) == 0) { + return 1; + } + + /* Force PSR to a valid 64-bit EL0t */ + regs->pstate &= PSR_f | PSR_s; + + return 0; +} + +/* + * Are the current registers suitable for user mode? (used to maintain + * security in signal handlers) + */ +int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task) +{ + if (is_compat_thread(task_thread_info(task))) + return valid_aa32_regs(regs); + else + return valid_aa64_regs(regs); +} diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index e18c48c..a8eafdb 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -115,7 +115,7 @@ static int restore_sigframe(struct pt_regs *regs, */ regs->syscallno = ~0UL; - err |= !valid_user_regs(®s->user_regs); + err |= !valid_user_regs(®s->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(®s->user_regs); + ret |= !valid_user_regs(®s->user_regs, current); /* * Fast forward the stepping logic so we step into the signal diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c index 71ef6dc..1073356 100644 --- a/arch/arm64/kernel/signal32.c +++ b/arch/arm64/kernel/signal32.c @@ -356,7 +356,7 @@ static int compat_restore_sigframe(struct pt_regs *regs, */ regs->syscallno = ~0UL; - err |= !valid_user_regs(®s->user_regs); + err |= !valid_user_regs(®s->user_regs, current); aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace; if (err == 0)
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 <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Dave Martin <dave.martin@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Will Deacon <will.deacon@arm.com> --- 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(-)