linux-kernel - Re: [PATCH v2 12/18] arm64: entry: Explicitly pass exception level to kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Dec 2017 17:51:44 +0000
From:   Will Deacon <will.deacon@....com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        catalin.marinas@....com, ard.biesheuvel@...aro.org,
        sboyd@...eaurora.org, dave.hansen@...ux.intel.com,
        keescook@...omium.org, msalter@...hat.com, labbott@...hat.com,
        tglx@...utronix.de
Subject: Re: [PATCH v2 12/18] arm64: entry: Explicitly pass exception level
 to kernel_ventry macro

On Fri, Dec 01, 2017 at 11:58:36AM +0000, Mark Rutland wrote:
> On Thu, Nov 30, 2017 at 04:39:40PM +0000, Will Deacon wrote:
> > We will need to treat exceptions from EL0 differently in kernel_ventry,
> > so rework the macro to take the exception level as an argument and
> > construct the branch target using that.
> > 
> > Signed-off-by: Will Deacon <will.deacon@....com>
> > ---
> >  arch/arm64/kernel/entry.S | 46 +++++++++++++++++++++++-----------------------
> >  1 file changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index dea196f287a0..688e52f65a8d 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -71,7 +71,7 @@
> >  #define BAD_FIQ		2
> >  #define BAD_ERROR	3
> >  
> > -	.macro kernel_ventry	label
> > +	.macro kernel_ventry, el, label, regsize = 64
> >  	.align 7
> >  	sub	sp, sp, #S_FRAME_SIZE
> >  #ifdef CONFIG_VMAP_STACK
> > @@ -84,7 +84,7 @@
> >  	tbnz	x0, #THREAD_SHIFT, 0f
> >  	sub	x0, sp, x0			// x0'' = sp' - x0' = (sp + x0) - sp = x0
> >  	sub	sp, sp, x0			// sp'' = sp' - x0 = (sp + x0) - x0 = sp
> > -	b	\label
> > +	b	el\()\el\()_\label
> >  
> >  0:
> >  	/*
> > @@ -116,7 +116,7 @@
> >  	sub	sp, sp, x0
> >  	mrs	x0, tpidrro_el0
> >  #endif
> > -	b	\label
> > +	b	el\()\el\()_\label
> >  	.endm
> >  
> >  	.macro	kernel_entry, el, regsize = 64
> > @@ -369,31 +369,31 @@ tsk	.req	x28		// current thread_info
> >  
> >  	.align	11
> >  ENTRY(vectors)
> > -	kernel_ventry	el1_sync_invalid		// Synchronous EL1t
> > -	kernel_ventry	el1_irq_invalid			// IRQ EL1t
> > -	kernel_ventry	el1_fiq_invalid			// FIQ EL1t
> > -	kernel_ventry	el1_error_invalid		// Error EL1t
> > +	kernel_ventry	1, sync_invalid			// Synchronous EL1t
> > +	kernel_ventry	1, irq_invalid			// IRQ EL1t
> > +	kernel_ventry	1, fiq_invalid			// FIQ EL1t
> > +	kernel_ventry	1, error_invalid		// Error EL1t
> 
> Using the el paramter to build the branch name has the unfortunate
> property of obscuring the branch name. For example, that makes it
> difficult to jump around the entry asm with ctags, which is somewhat
> painful.
> 
> Could we leave the full branch name in place, e.g.
> 
> 	kernel_ventry	1, el1_sync_invalid		// Synchronous EL1t
> 	kernel_ventry	1, el1_irq_invalid		// IRQ EL1t
> 	kernel_ventry	1, el1_fiq_invalid		// FIQ EL1t
> 	kernel_ventry	1, el1_error_invalid		// Error EL1t
> 
> ... or have separate kernel_ventry and user_ventry macros that
> implicitly encoded the source EL, also leaving the label name as-is.

The downside of doing that is that it makes it possible to say things like:

	kernel_ventry	0, el1_sync

which I don't want to be expressible.

Given that ctags already chokes on lots of entry.S (for example, any macro
that is defined outside of the file) *and* that you can easily search for
things like el1_sync_invalid within the file, I'm inclined to leave this
patch as-is, but I'll note your objection and buy you a pint.

Will