linux-kernel - Re: [PATCHv4 11/19] arm64: don't reload GPRs after apply

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180710103722.GH9486@e103592.cambridge.arm.com>
Date:   Tue, 10 Jul 2018 11:37:24 +0100
From:   Dave Martin <Dave.Martin@....com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Will Deacon <Will.Deacon@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Catalin Marinas <Catalin.Marinas@....com>,
        "hch@...radead.org" <hch@...radead.org>,
        James Morse <James.Morse@....com>,
        "linux@...inikbrodowski.net" <linux@...inikbrodowski.net>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        Marc Zyngier <Marc.Zyngier@....com>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>
Subject: Re: [PATCHv4 11/19] arm64: don't reload GPRs after apply_ssbd

On Mon, Jul 09, 2018 at 03:21:59PM +0100, Mark Rutland wrote:
> On Fri, Jul 06, 2018 at 05:38:45PM +0100, Will Deacon wrote:
> > On Mon, Jul 02, 2018 at 12:04:07PM +0100, Mark Rutland wrote:
> > > Now that all of the syscall logic works on the saved pt_regs, apply_ssbd
> > > can safely corrupt x0-x3 in the entry paths, and we no longer need to
> > > restore them. So let's remove the logic doing so.
> > > 
> > > With that logic gone, we can fold the branch target into the macro, so
> > > that callers need not deal with this. GAS provides \@, which provides a
> > > unique value per macro invocation, which we can use to create a unique
> > > label.
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@....com>
> > > Acked-by: Marc Zyngier <marc.zyngier@....com>
> > > Acked-by: Catalin Marinas <catalin.marinas@....com>
> > > Cc: Will Deacon <will.deacon@....com>
> > > ---
> > >  arch/arm64/kernel/entry.S | 20 +++++++-------------
> > >  1 file changed, 7 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > index c41b84d06644..728bc7cc5bbb 100644
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -130,20 +130,21 @@ alternative_else_nop_endif
> > >  
> > >  	// This macro corrupts x0-x3. It is the caller's duty
> > >  	// to save/restore them if required.
> > > -	.macro	apply_ssbd, state, targ, tmp1, tmp2
> > > +	.macro	apply_ssbd, state, tmp1, tmp2
> > >  #ifdef CONFIG_ARM64_SSBD
> > >  alternative_cb	arm64_enable_wa2_handling
> > > -	b	\targ
> > > +	b	skip_apply_ssbd\@
> > >  alternative_cb_end
> > >  	ldr_this_cpu	\tmp2, arm64_ssbd_callback_required, \tmp1
> > > -	cbz	\tmp2, \targ
> > > +	cbz	\tmp2, skip_apply_ssbd\@
> > >  	ldr	\tmp2, [tsk, #TSK_TI_FLAGS]
> > > -	tbnz	\tmp2, #TIF_SSBD, \targ
> > > +	tbnz	\tmp2, #TIF_SSBD, skip_apply_ssbd\@
> > 
> > Talking to Dave, he makes a good point that this is pretty fragile if a
> > macro expansion within the macro itself uses \@, since this would result
> > in an unexpected label update and everything would go wrong.
> 
> I don't believe that's a problem; \@ is handled as-if it's a named
> argument to the macro, and is not incremented within the scope of a
> single macro expansion.

From
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/macro.c
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/read.c

it looks like the \@ counter (macro_number) is only incremented at the
end expansion of a whole macro body before gas reads the expansion
output (including recursive macro expansions).

So, your conclusion looks right for gas today.  The code implementing
this looks crufty enough to be pretty old.

Can you throw a bug into https://sourceware.org/bugzilla/ to get this
properly documented?  The current wording is ambiguous.  It would be
nice to get gas properly committed to behaving this way.


For the kernel, I suggest using a .L prefix so that the generated
symbols don't bloat the vmlinux symbol table (similar to numbered local
labels) -- unless you really want the symbols retained.

Having a common prefix for all "unique" assembler symbols may help us
to avoid namespace collisions, say

.L__asm__foo_\@
.L__asm__bar_\@

etc.

Cheers
---Dave

> 
> e.g. if I assemble:
> 
> ----
> .macro nop_macro
> .endm
> 
> .macro a n
>         a_pre_\n\()_\@:
>         .quad   0
>         a_post_\n\()_\@:
>         .quad   0
> .endm
> 
> .macro b n
>         b_pre_\n\()_\@:
>         .quad   0
> 
>         a \n
> 
>         b_post_\n\()_\@:
>         .quad   0
> .endm
> 
> .macro c n
>         c_pre_\n\()_\@:
>         .quad   0
> 
>         b \n
> 
>         c_post_\n\()_\@:
>         .quad   0
> .endm
> 
> .data
> 
>         a 0
> 
>         b 1
> 
>         c 2
> ----
> 
> ... then objdump -D gives me:
> 
> ----
> Disassembly of section .data:
> 
> 0000000000000000 <a_pre_0_0>:
>         ...
> 
> 0000000000000008 <a_post_0_0>:
>         ...
> 
> 0000000000000010 <b_pre_1_1>:
>         ...
> 
> 0000000000000018 <a_pre_1_2>:
>         ...
> 
> 0000000000000020 <a_post_1_2>:
>         ...
> 
> 0000000000000028 <b_post_1_1>:
>         ...
> 
> 0000000000000030 <c_pre_2_3>:
>         ...
> 
> 0000000000000038 <b_pre_2_4>:
>         ...
> 
> 0000000000000040 <a_pre_2_5>:
>         ...
> 
> 0000000000000048 <a_post_2_5>:
>         ...
> 
> 0000000000000050 <b_post_2_4>:
>         ...
> 
> 0000000000000058 <c_post_2_3>:
>         ...
> 
> ----
> 
> ... where things are obviously nesting just fine.
> 
> Thanks,
> Mark.