diff mbox

arm64: Rework valid_user_regs

Message ID 1455041501-30018-1-git-send-email-mark.rutland@arm.com
State Superseded
Headers show

Commit Message

Mark Rutland Feb. 9, 2016, 6:11 p.m. UTC
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(-)

-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Will Deacon Feb. 10, 2016, 11:58 a.m. UTC | #1
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland Feb. 10, 2016, 12:31 p.m. UTC | #2
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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Peter Maydell Feb. 10, 2016, 2:23 p.m. UTC | #3
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon Feb. 10, 2016, 2:43 p.m. UTC | #4
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland Feb. 10, 2016, 4:01 p.m. UTC | #5
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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon Feb. 10, 2016, 4:04 p.m. UTC | #6
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland Feb. 10, 2016, 4:05 p.m. UTC | #7
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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon Feb. 10, 2016, 4:36 p.m. UTC | #8
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox

Patch

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(&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
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(&regs->user_regs);
+	err |= !valid_user_regs(&regs->user_regs, current);
 
 	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
 	if (err == 0)