diff mbox

[v2,4/9] arm64: head.S: move KASLR processing out of __enable_mmu()

Message ID 1472049366-10922-5-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show

Commit Message

Ard Biesheuvel Aug. 24, 2016, 2:36 p.m. UTC 
The KASLR processing in __enable_mmu() is only used by the primary boot
path, and complements the processing that takes place in __primary_switch().
Move the two parts together, to make the code easier to understand.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 arch/arm64/kernel/head.S | 66 ++++++++++++--------
 1 file changed, 39 insertions(+), 27 deletions(-)

-- 
2.7.4


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

Comments

Mark Rutland Aug. 24, 2016, 8:36 p.m. UTC | #1 
Hi,

On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
> The KASLR processing in __enable_mmu() is only used by the primary boot

> path, and complements the processing that takes place in __primary_switch().

> Move the two parts together, to make the code easier to understand.


As a heads-up, while reviewing this I spotted an existing issue [1]. I'd meant
to comment so when posting that patch, but in my hubris from making
git-send-email work I forgot to do so. :/

[...]

> @@ -770,11 +748,11 @@ __no_granule_support:

>  1:

>  	wfe

>  	wfi

> -	b 1b

> +	b	1b

>  ENDPROC(__no_granule_support)


Unrelated change? Perhaps it's worth putting all the whitespace fixup in a
preparatory patch?

[...]

> +__primary_switch:

> +#ifdef CONFIG_RANDOMIZE_BASE

> +	mov	x19, x0				// preserve new SCTLR_EL1 value

> +	mrs	x20, sctlr_el1			// preserve old SCTLR_EL1 value

> +#endif

> +

> +	adr	x27, 0f

> +	b	__enable_mmu


As we do elsewhere, it's probably worth a comment on the line with the ADR into
x27, mentioning that __enable_mmu will branch there.

... or perhaps we should just have __enable_mmu return to the LR like a normal
AAPCS function, place the switch routines in the idmap, and use the idiomatic
sequence:

__thing_switch:
	bl	__enable_mmu
	ldr	xN, =__thing
	blr	xN

[...]

> +	/*

> +	 * If we return here, we have a KASLR displacement in x23 which we need

> +	 * to take into account by discarding the current kernel mapping and

> +	 * creating a new one.

> +	 */

> +	msr	sctlr_el1, x20			// disable the MMU

> +	isb

> +	bl	__create_page_tables		// recreate kernel mapping


As per the issue I mentioned above [1], here we also need:

	tlbi	vmalle1
	dsb	nsh

... in order to avoid TLB conflicts and other issues resulting from BBM
violations.

> +

> +	msr	sctlr_el1, x19			// re-enable the MMU

> +	isb

> +	ic	iallu				// flush instructions fetched

> +	dsb	nsh				// via old mapping

> +	isb


Thanks,
Mark.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Ard Biesheuvel Aug. 24, 2016, 8:44 p.m. UTC | #2 
On 24 August 2016 at 22:36, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,

>

> On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:

>> The KASLR processing in __enable_mmu() is only used by the primary boot

>> path, and complements the processing that takes place in __primary_switch().

>> Move the two parts together, to make the code easier to understand.

>

> As a heads-up, while reviewing this I spotted an existing issue [1]. I'd meant

> to comment so when posting that patch, but in my hubris from making

> git-send-email work I forgot to do so. :/

>

> [...]

>

>> @@ -770,11 +748,11 @@ __no_granule_support:

>>  1:

>>       wfe

>>       wfi

>> -     b 1b

>> +     b       1b

>>  ENDPROC(__no_granule_support)

>

> Unrelated change? Perhaps it's worth putting all the whitespace fixup in a

> preparatory patch?

>

> [...]

>


I couldn't resist. It's the only occurrence in this series apart from #2

>> +__primary_switch:

>> +#ifdef CONFIG_RANDOMIZE_BASE

>> +     mov     x19, x0                         // preserve new SCTLR_EL1 value

>> +     mrs     x20, sctlr_el1                  // preserve old SCTLR_EL1 value

>> +#endif

>> +

>> +     adr     x27, 0f

>> +     b       __enable_mmu

>

> As we do elsewhere, it's probably worth a comment on the line with the ADR into

> x27, mentioning that __enable_mmu will branch there.

>

> ... or perhaps we should just have __enable_mmu return to the LR like a normal

> AAPCS function, place the switch routines in the idmap, and use the idiomatic

> sequence:

>

> __thing_switch:

>         bl      __enable_mmu

>         ldr     xN, =__thing

>         blr     xN

>

> [...]

>


Yes, that is more or less the point of the two subsequent patches.

>> +     /*

>> +      * If we return here, we have a KASLR displacement in x23 which we need

>> +      * to take into account by discarding the current kernel mapping and

>> +      * creating a new one.

>> +      */

>> +     msr     sctlr_el1, x20                  // disable the MMU

>> +     isb

>> +     bl      __create_page_tables            // recreate kernel mapping

>

> As per the issue I mentioned above [1], here we also need:

>

>         tlbi    vmalle1

>         dsb     nsh

>

> ... in order to avoid TLB conflicts and other issues resulting from BBM

> violations.

>


Indeed.

Thanks,
Ard.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland Aug. 24, 2016, 8:46 p.m. UTC | #3 
On Wed, Aug 24, 2016 at 09:36:10PM +0100, Mark Rutland wrote:
> On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:

> > +__primary_switch:

> > +#ifdef CONFIG_RANDOMIZE_BASE

> > +	mov	x19, x0				// preserve new SCTLR_EL1 value

> > +	mrs	x20, sctlr_el1			// preserve old SCTLR_EL1 value

> > +#endif

> > +

> > +	adr	x27, 0f

> > +	b	__enable_mmu

> 

> As we do elsewhere, it's probably worth a comment on the line with the ADR into

> x27, mentioning that __enable_mmu will branch there.

> 

> ... or perhaps we should just have __enable_mmu return to the LR like a normal

> AAPCS function, place the switch routines in the idmap, and use the idiomatic

> sequence:

> 

> __thing_switch:

> 	bl	__enable_mmu

> 	ldr	xN, =__thing

> 	blr	xN


... and now I see that this is what subsequent patches do ;)

Is it possible to first AAPCS-ify __enable_mmu (with shuffling of callers as
above) in one patch, prior to this? That would avoid introducing the unusual 0f
label above, and the temporary x30 usage in a subsequent patch.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Ard Biesheuvel Aug. 25, 2016, 1:59 p.m. UTC | #4 
On 24 August 2016 at 21:46, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Aug 24, 2016 at 09:36:10PM +0100, Mark Rutland wrote:

>> On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:

>> > +__primary_switch:

>> > +#ifdef CONFIG_RANDOMIZE_BASE

>> > +   mov     x19, x0                         // preserve new SCTLR_EL1 value

>> > +   mrs     x20, sctlr_el1                  // preserve old SCTLR_EL1 value

>> > +#endif

>> > +

>> > +   adr     x27, 0f

>> > +   b       __enable_mmu

>>

>> As we do elsewhere, it's probably worth a comment on the line with the ADR into

>> x27, mentioning that __enable_mmu will branch there.

>>

>> ... or perhaps we should just have __enable_mmu return to the LR like a normal

>> AAPCS function, place the switch routines in the idmap, and use the idiomatic

>> sequence:

>>

>> __thing_switch:

>>       bl      __enable_mmu

>>       ldr     xN, =__thing

>>       blr     xN

>

> ... and now I see that this is what subsequent patches do ;)

>

> Is it possible to first AAPCS-ify __enable_mmu (with shuffling of callers as

> above) in one patch, prior to this?


Yes, but that would result in an __enable_mmu() that needs to stash
the link register value, and essentially returns twice in the KASLR
case. As an intermediate step working towards the result after the
series, I think the adr + label above is the lesser evil

> That would avoid introducing the unusual 0f

> label above, and the temporary x30 usage in a subsequent patch.

>

> Thanks,

> Mark.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland Aug. 30, 2016, 10:24 a.m. UTC | #5 
On Thu, Aug 25, 2016 at 02:59:51PM +0100, Ard Biesheuvel wrote:
> On 24 August 2016 at 21:46, Mark Rutland <mark.rutland@arm.com> wrote:

> > On Wed, Aug 24, 2016 at 09:36:10PM +0100, Mark Rutland wrote:

> >> On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:

> >> > +__primary_switch:

> >> > +#ifdef CONFIG_RANDOMIZE_BASE

> >> > +   mov     x19, x0                         // preserve new SCTLR_EL1 value

> >> > +   mrs     x20, sctlr_el1                  // preserve old SCTLR_EL1 value

> >> > +#endif

> >> > +

> >> > +   adr     x27, 0f

> >> > +   b       __enable_mmu

> >>

> >> As we do elsewhere, it's probably worth a comment on the line with the ADR into

> >> x27, mentioning that __enable_mmu will branch there.

> >>

> >> ... or perhaps we should just have __enable_mmu return to the LR like a normal

> >> AAPCS function, place the switch routines in the idmap, and use the idiomatic

> >> sequence:

> >>

> >> __thing_switch:

> >>       bl      __enable_mmu

> >>       ldr     xN, =__thing

> >>       blr     xN

> >

> > ... and now I see that this is what subsequent patches do ;)

> >

> > Is it possible to first AAPCS-ify __enable_mmu (with shuffling of callers as

> > above) in one patch, prior to this?

> 

> Yes, but that would result in an __enable_mmu() that needs to stash

> the link register value, and essentially returns twice in the KASLR

> case.


Ah, good point. I had missed that.

> As an intermediate step working towards the result after the series, I

> think the adr + label above is the lesser evil


Yes, it probably is.

I'll try to flip back into review mode, keeping the above in mind.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland Aug. 30, 2016, 1:45 p.m. UTC | #6 
Hi,

On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
> @@ -742,25 +739,6 @@ ENTRY(__enable_mmu)

>  	ic	iallu

>  	dsb	nsh

>  	isb

> -#ifdef CONFIG_RANDOMIZE_BASE

> -	mov	x19, x0				// preserve new SCTLR_EL1 value

> -	blr	x27

> -

> -	/*

> -	 * If we return here, we have a KASLR displacement in x23 which we need

> -	 * to take into account by discarding the current kernel mapping and

> -	 * creating a new one.

> -	 */

> -	msr	sctlr_el1, x22			// disable the MMU

> -	isb

> -	bl	__create_page_tables		// recreate kernel mapping

> -

> -	msr	sctlr_el1, x19			// re-enable the MMU

> -	isb

> -	ic	iallu				// flush instructions fetched

> -	dsb	nsh				// via old mapping

> -	isb

> -#endif

>  	br	x27

>  ENDPROC(__enable_mmu)


As a heads-up, this clashes with fd363bd417ddb610 ("arm64: avoid TLB
conflict with CONFIG_RANDOMIZE_BASE") [1], which went in for v4.8-rc4.

The fixup (moving the new TLBI; DSB into __primary_switch) is
trivial/obvious, but beyond git's automated resolution capabilities.

> @@ -770,11 +748,11 @@ __no_granule_support:

>  1:

>  	wfe

>  	wfi

> -	b 1b

> +	b	1b

>  ENDPROC(__no_granule_support)


As mentioned in another reply, it might be worth moving the whitespace
fixups into a preparatory patch, so as to make it less distracting when
looking at the diff.

Regardless, FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>


Thanks,
Mark.

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

_______________________________________________
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/kernel/head.S b/arch/arm64/kernel/head.S
index 2871271123e7..d390feb92730 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -222,9 +222,7 @@  ENTRY(stext)
 	 * the TCR will have been set.
 	 */
 	bl	__cpu_setup			// initialise processor
-	adr_l	x27, __primary_switch		// address to jump to after
-						// MMU has been enabled
-	b	__enable_mmu
+	b	__primary_switch
 ENDPROC(stext)
 
 /*
@@ -453,7 +451,7 @@  __primary_switched:
 	cbz	x0, 0f				// KASLR disabled? just proceed
 	orr	x23, x23, x0			// record KASLR offset
 	ret	x28				// we must enable KASLR, return
-						// to __enable_mmu()
+						// to __primary_switch()
 0:
 #endif
 	b	start_kernel
@@ -721,7 +719,6 @@  ENTRY(__early_cpu_boot_status)
  */
 	.section	".idmap.text", "ax"
 ENTRY(__enable_mmu)
-	mrs	x22, sctlr_el1			// preserve old SCTLR_EL1 value
 	mrs	x1, ID_AA64MMFR0_EL1
 	ubfx	x2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
 	cmp	x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
@@ -742,25 +739,6 @@  ENTRY(__enable_mmu)
 	ic	iallu
 	dsb	nsh
 	isb
-#ifdef CONFIG_RANDOMIZE_BASE
-	mov	x19, x0				// preserve new SCTLR_EL1 value
-	blr	x27
-
-	/*
-	 * If we return here, we have a KASLR displacement in x23 which we need
-	 * to take into account by discarding the current kernel mapping and
-	 * creating a new one.
-	 */
-	msr	sctlr_el1, x22			// disable the MMU
-	isb
-	bl	__create_page_tables		// recreate kernel mapping
-
-	msr	sctlr_el1, x19			// re-enable the MMU
-	isb
-	ic	iallu				// flush instructions fetched
-	dsb	nsh				// via old mapping
-	isb
-#endif
 	br	x27
 ENDPROC(__enable_mmu)
 
@@ -770,11 +748,11 @@  __no_granule_support:
 1:
 	wfe
 	wfi
-	b 1b
+	b	1b
 ENDPROC(__no_granule_support)
 
-__primary_switch:
 #ifdef CONFIG_RELOCATABLE
+__relocate_kernel:
 	/*
 	 * Iterate over each entry in the relocation table, and apply the
 	 * relocations in place.
@@ -796,8 +774,42 @@  __primary_switch:
 	add	x13, x13, x23			// relocate
 	str	x13, [x11, x23]
 	b	0b
+1:	ret
+ENDPROC(__relocate_kernel)
+#endif
 
-1:
+__primary_switch:
+#ifdef CONFIG_RANDOMIZE_BASE
+	mov	x19, x0				// preserve new SCTLR_EL1 value
+	mrs	x20, sctlr_el1			// preserve old SCTLR_EL1 value
+#endif
+
+	adr	x27, 0f
+	b	__enable_mmu
+0:
+#ifdef CONFIG_RELOCATABLE
+	bl	__relocate_kernel
+#ifdef CONFIG_RANDOMIZE_BASE
+	ldr	x8, =__primary_switched
+	blr	x8
+
+	/*
+	 * If we return here, we have a KASLR displacement in x23 which we need
+	 * to take into account by discarding the current kernel mapping and
+	 * creating a new one.
+	 */
+	msr	sctlr_el1, x20			// disable the MMU
+	isb
+	bl	__create_page_tables		// recreate kernel mapping
+
+	msr	sctlr_el1, x19			// re-enable the MMU
+	isb
+	ic	iallu				// flush instructions fetched
+	dsb	nsh				// via old mapping
+	isb
+
+	bl	__relocate_kernel
+#endif
 #endif
 	ldr	x8, =__primary_switched
 	br	x8