Ensure stack is aligned for kernel entries

The pt_regs structure's size is required to be a multiple of 16 to maintain the stack alignment upon kernel entries.

Add a static check to ensure this is the case.

Signed-off-by Julien Thierry <[email protected]>

---
 arch/arm64/include/asm/ptrace.h | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/include/asm/ptrace.h +0 -0

index 177b851..cc17fad 100644

Quoted 16 lines · Julien Thierry

The pt_regs structure's size is required to be a multiple of 16 to maintain the stack alignment upon kernel entries.

Add a static check to ensure this is the case.

Signed-off-by Julien Thierry <[email protected]>

---
 arch/arm64/include/asm/ptrace.h | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/include/asm/ptrace.h +0 -0

index 177b851..cc17fad 100644
@@ -229,6 +229,8 @@ static inline u64 regs_get_register(struct pt_regs *regs, unsigned int offset)
 

arch/arm64/include/asm/ptrace.h static inline u64 regs_get_register(struct pt_regs *regs, unsigned int offset) · line 232

Quoted 3 lines

arch/arm64/include/asm/ptrace.h +1 -0

 	WARN_ON(offset & 7);
 
+	BUILD_BUG_ON((sizeof (struct pt_regs) % 16 != 0));

This seems like a fairly random place to put this, and given that it's not dependent on context I reckon it would be better to put it in a C file rather than in a header that gets included multiple times. I still couldn't really find a good place though :/

Quoted 19 lines · Will Deacon

Quoted 18 lines · Julien Thierry

The pt_regs structure's size is required to be a multiple of 16 to maintain the stack alignment upon kernel entries.

Add a static check to ensure this is the case.

Signed-off-by Julien Thierry <[email protected]>

---
  arch/arm64/include/asm/ptrace.h | 2 ++
  1 file changed, 2 insertions(+)

arch/arm64/include/asm/ptrace.h +0 -0

index 177b851..cc17fad 100644
@@ -229,6 +229,8 @@ static inline u64 regs_get_register(struct pt_regs *regs, unsigned int offset)
 
  	WARN_ON(offset & 7);
 

Quoted 6 lines

arch/arm64/include/asm/ptrace.h static inline u64 regs_get_register(struct pt_regs *regs, unsigned int offset) · line 232

Quoted 1 line

arch/arm64/include/asm/ptrace.h +1 -0

+	BUILD_BUG_ON((sizeof (struct pt_regs) % 16 != 0));

This seems like a fairly random place to put this, and given that it's not dependent on context I reckon it would be better to put it in a C file rather than in a header that gets included multiple times. I still couldn't really find a good place though :/

Perhaps asm-offsets.c, when we define S_FRAME_SIZE?

Quoted 25 lines · Mark Rutland

Quoted 23 lines · Will Deacon

Quoted 15 lines · Julien Thierry

The pt_regs structure's size is required to be a multiple of 16 to maintain the stack alignment upon kernel entries.

Add a static check to ensure this is the case.

Signed-off-by Julien Thierry <[email protected]>

---
  arch/arm64/include/asm/ptrace.h | 2 ++
  1 file changed, 2 insertions(+)

arch/arm64/include/asm/ptrace.h +0 -0

index 177b851..cc17fad 100644

arch/arm64/include/asm/ptrace.h static inline u64 regs_get_register(struct pt_regs *regs, unsigned int offset) · line 230

Quoted 2 lines

arch/arm64/include/asm/ptrace.h +1 -0

@@ -229,6 +229,8 @@ static inline u64 regs_get_register(struct pt_regs *regs, unsigned int offset)
  	WARN_ON(offset & 7);
+	BUILD_BUG_ON((sizeof (struct pt_regs) % 16 != 0));

This seems like a fairly random place to put this, and given that it's not dependent on context I reckon it would be better to put it in a C file rather than in a header that gets included multiple times. I still couldn't really find a good place though :/

Quoted 2 lines

Perhaps asm-offsets.c, when we define S_FRAME_SIZE?

That sounds like a good option to me. If Will thinks so as well I'll move that in asm-offsets.c.

The pt_regs structure's size is required to be a multiple of 16 to maintain the stack alignment upon kernel entries.

Add a static check to ensure this is the case.

Signed-off-by Julien Thierry <[email protected]>

---
 arch/arm64/include/asm/ptrace.h | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/include/asm/ptrace.h +0 -0

index 177b851..cc17fad 100644

arch/arm64/include/asm/ptrace.h +1 -1

+
        offset >>= 3;
        switch (offset) {
        case 0 ... 30:
--

1.9.1

SDEI events can occur at any point, including when the stack pointer is not aligned. SP could be modified to respect alignment 16-byte alignement, but there is a need to deal with code using SP as scratch register.

Always reserve the SDEI stacks to handle its events.

Since stack allocation relies on VMAPed stacks, lets make SDEI depend on VMAP_STACK.

Signed-off-by Julien Thierry <[email protected]>

---
 arch/arm64/include/asm/sdei.h |  2 --
 arch/arm64/kernel/entry.S     |  2 --
 arch/arm64/kernel/sdei.c      | 23 ++++++++---------------
 drivers/firmware/Kconfig      |  1 +
 4 files changed, 9 insertions(+), 19 deletions(-)

Quoted 4 lines · Julien Thierry

SDEI events can occur at any point, including when the stack pointer is not aligned. SP could be modified to respect alignment 16-byte alignement, but there is a need to deal with code using SP as scratch register.

Quoted 4 lines

Always reserve the SDEI stacks to handle its events.

Since stack allocation relies on VMAPed stacks, lets make SDEI depend on VMAP_STACK.

One side-effect of this change is that SDEI cannot be enabled in conjunction with KASAN. James -- are you ok with that?

Arm64 SP needs to be aligned to 16 bytes before being used as base address for loads and stores. When taking some valid exceptions from EL1 (e.g. irq, dbg, data abort), there is no guarantee that SP_EL1 was aligned when taking the exception.

Pad the stack on EL1 entries when misaligned.

Signed-off-by Julien Thierry <[email protected]>

---
 arch/arm64/include/asm/assembler.h |  9 +++++++++
 arch/arm64/kernel/entry.S          | 32 ++++++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 2 deletions(-)

arch/arm64/include/asm/assembler.h +0 -0

index 0bcc98d..a0a5415 100644

arch/arm64/kernel/entry.S +14 -0

+
+	// aligned_sp[S_SP] = old_sp
+	bic	x1, x0, #0xf	// align down to 16-byte
+	str	x0, [x1, #S_SP]
+
+	ldr	x1, [x0]
+	bic	x0, x0, #0xf	// x0 = aligned_sp
+	xchg_sp	x0
+	.endm
+
 /*
  * Bad Abort numbers
  *-----------------

@@ -158,6 +171,10 @@ alternative_cb_end
 	.if	\regsize == 32
 	mov	w0, w0				// zero upper 32 bits of x0
 	.endif
+	.if	\el != 0
+	force_stack_align
+	.endif
+
 	stp	x0, x1, [sp, #16 * 0]
 	stp	x2, x3, [sp, #16 * 1]
 	stp	x4, x5, [sp, #16 * 2]

Quoted 18 lines · Julien Thierry

Arm64 SP needs to be aligned to 16 bytes before being used as base address for loads and stores. When taking some valid exceptions from EL1 (e.g. irq, dbg, data abort), there is no guarantee that SP_EL1 was aligned when taking the exception.

Pad the stack on EL1 entries when misaligned.

Signed-off-by Julien Thierry <[email protected]>

---
  arch/arm64/include/asm/assembler.h |  9 +++++++++
  arch/arm64/kernel/entry.S          | 32 ++++++++++++++++++++++++++++++--
  2 files changed, 39 insertions(+), 2 deletions(-)

arch/arm64/include/asm/assembler.h +0 -0

index 0bcc98d..a0a5415 100644

arch/arm64/include/asm/assembler.h line 705

Quoted 5 lines

arch/arm64/include/asm/assembler.h +2 -0

@@ -701,4 +701,13 @@
  .Lyield_out_\@ :
  	.endm
 
+/*
+ * Echange content of register xt with sp.

Exchange contents of register Xt with SP

arch/arm64/include/asm/assembler.h line 711

Quoted 6 lines

arch/arm64/include/asm/assembler.h +6 -0

+ */
+	.macro xchg_sp	xt
+	add	sp, sp, \xt	// sp' = sp + xt
+	sub	\xt, sp, \xt	// xt' = sp' - xt = sp + xt - xt = sp
+	sub	sp, sp, \xt	// sp'' = sp' - xt' = sp + xt - sp = xt
+	.endm

Having said that, this is clearly inspired by the code in kernel_ventry for dealing with stack overflow, yet the macro you've got here cannot be reused there.

Maybe we're better off only macro-ising:

	.macro sp_fetch_add xt
	add	sp, sp, \xt
	sub	\xt, sp, \xt
	.endm

and then letting the callers handle the rest, or build an swizzle_sp macro on top of it?

Quoted 10 lines

arch/arm64/include/asm/assembler.h +1 -0

+
  #endif	/* __ASM_ASSEMBLER_H */

arch/arm64/kernel/entry.S +0 -0

index fc5842b..8fb66e4 100644
@@ -59,6 +59,19 @@
  	.endr
  	.endm
 

arch/arm64/kernel/entry.S line 64

Quoted 3 lines

arch/arm64/kernel/entry.S +3 -0

+	.macro force_stack_align
+	xchg_sp	x0
+	str	x1, [x0] // store x1 far away from S_SP

So this temporarily trashes pt_regs.regs[0] if the stack is already aligned, right? I think that's fine, but we should comment /that/ rather than vaguely saying it's far away from S_SP. In fact, having a block comment above this macro saying exactly what is clobbered would generally be a good idea (esp as you nobble the flags in your next patch).

Quoted 27 lines

arch/arm64/kernel/entry.S +14 -0

+
+	// aligned_sp[S_SP] = old_sp
+	bic	x1, x0, #0xf	// align down to 16-byte
+	str	x0, [x1, #S_SP]
+
+	ldr	x1, [x0]
+	bic	x0, x0, #0xf	// x0 = aligned_sp
+	xchg_sp	x0
+	.endm
+
  /*
   * Bad Abort numbers
   *-----------------

@@ -158,6 +171,10 @@ alternative_cb_end
  	.if	\regsize == 32
  	mov	w0, w0				// zero upper 32 bits of x0
  	.endif
+	.if	\el != 0
+	force_stack_align
+	.endif
+
  	stp	x0, x1, [sp, #16 * 0]
  	stp	x2, x3, [sp, #16 * 1]
  	stp	x4, x5, [sp, #16 * 2]

@@ -184,7 +201,8 @@ alternative_cb_end
  	apply_ssbd 1, x22, x23
 

arch/arm64/kernel/entry.S alternative_cb_end · line 205

Quoted 4 lines

arch/arm64/kernel/entry.S +2 -1

  	.else
-	add	x21, sp, #S_FRAME_SIZE
+	ldr	x21, [sp, #S_SP]
+	add	x21, x21, #S_FRAME_SIZE		// adjust stored sp

Can we not just store the corrected SP in force_stack_align, rather than store something bogus and then have to add the frame size back here?

Avoiding stack alignment for already aligned stack can give a small performance boost.

Branch out of the aligning code when the stack is known to be aligned.

Signed-off-by Julien Thierry <[email protected]>

---
 arch/arm64/kernel/entry.S | 9 +++++++++
 1 file changed, 9 insertions(+)

arch/arm64/kernel/entry.S +4 -0

index 8fb66e4..bd8d52c 100644
@@ -61,6 +61,10 @@
 
 	.macro force_stack_align
 	xchg_sp	x0
+
+	tst	x0, #0xf
+	b.eq	0f
+
 	str	x1, [x0] // store x1 far away from S_SP
 
 	// aligned_sp[S_SP] = old_sp

Quoted 27 lines · Julien Thierry

Avoiding stack alignment for already aligned stack can give a small performance boost.

Branch out of the aligning code when the stack is known to be aligned.

Signed-off-by Julien Thierry <[email protected]>

---
  arch/arm64/kernel/entry.S | 9 +++++++++
  1 file changed, 9 insertions(+)

arch/arm64/kernel/entry.S +4 -0

index 8fb66e4..bd8d52c 100644
@@ -61,6 +61,10 @@
 
  	.macro force_stack_align
  	xchg_sp	x0
+
+	tst	x0, #0xf
+	b.eq	0f
+
  	str	x1, [x0] // store x1 far away from S_SP
 
  	// aligned_sp[S_SP] = old_sp

@@ -69,6 +73,11 @@
 

arch/arm64/kernel/entry.S line 80

Quoted 7 lines

arch/arm64/kernel/entry.S +5 -0

  	ldr	x1, [x0]
  	bic	x0, x0, #0xf	// x0 = aligned_sp
+	b	1f
+
+0:
+	str	x0, [x0, #S_SP]
+1:

I couldn't figure out a better way to do this, but please remove the empty lines that you're adding here. With that:

Acked-by Will Deacon <[email protected]>

When an EL2 entry of __kvm_hyp_vector is taken, it means an entry from a lower EL was previously taken to exit the guest. Taking that lower EL entry already applied BP hardening if it was needed, so there is no need to do it again.

Only apply BP hardening for exception coming from lower EL.

Signed-off-by Julien Thierry <[email protected]>

Cc kvmarm at lists.cs.columbia.edu

---
 arch/arm64/kernel/cpu_errata.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

arch/arm64/kernel/cpu_errata.c +2 -1

index dec1089..9db5ecc 100644
@@ -103,10 +103,16 @@ static void __copy_hyp_vect_bpi(int slot, const char *hyp_vecs_start,
 	void *dst = lm_alias(__bp_harden_hyp_vecs_start + slot * SZ_2K);
 	int i;
 
-	for (i = 0; i < SZ_2K; i += 0x80)
+	/*
+	 * Only overwrite hyp entries for exceptions from lower EL.

Quoted 24 lines · Julien Thierry

When an EL2 entry of __kvm_hyp_vector is taken, it means an entry from a lower EL was previously taken to exit the guest. Taking that lower EL entry already applied BP hardening if it was needed, so there is no need to do it again.

Only apply BP hardening for exception coming from lower EL.

Signed-off-by Julien Thierry <[email protected]>

Cc kvmarm at lists.cs.columbia.edu

---
  arch/arm64/kernel/cpu_errata.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

arch/arm64/kernel/cpu_errata.c +2 -1

index dec1089..9db5ecc 100644
@@ -103,10 +103,16 @@ static void __copy_hyp_vect_bpi(int slot, const char *hyp_vecs_start,
  	void *dst = lm_alias(__bp_harden_hyp_vecs_start + slot * SZ_2K);
  	int i;
 
-	for (i = 0; i < SZ_2K; i += 0x80)
+	/*
+	 * Only overwrite hyp entries for exceptions from lower EL.

arch/arm64/kernel/cpu_errata.c static void __copy_hyp_vect_bpi(int slot, const char *hyp_vecs_start, · line 116

Quoted 10 lines

arch/arm64/kernel/cpu_errata.c +6 -1

+	 * Exception vection vector is 2K bytes, first 1K bytes concern
+	 * exception from EL2 (EL2t, EL2h), last 1K concert lower exception
+	 * levels (ELx-64bits, ELx-32bits).
+	 */
+	for (i = SZ_1K; i < SZ_2K; i += 0x80)
  		memcpy(dst + i, hyp_vecs_start, hyp_vecs_end - hyp_vecs_start);
 
-	__flush_icache_range((uintptr_t)dst, (uintptr_t)dst + SZ_2K);
+	__flush_icache_range((uintptr_t)dst + SZ_1K, (uintptr_t)dst + SZ_2K);
  }

I'd personally find this clearer if you did:

	dst += SZ_1K;

before the for loop and with your comment above it. Then the for loop becomes:

	for (i = 0; i < SZ_1K; i += 0x80)

and the range of the cache maintenance is [dst, dst + SZ_1K)

Quoted 1 line

But I'll leave it up to Marc.

Quoted 30 lines · Julien Thierry

When an EL2 entry of __kvm_hyp_vector is taken, it means an entry from a lower EL was previously taken to exit the guest. Taking that lower EL entry already applied vector hardening if needed, so there is no need to do it again.

Only apply vector hardening for exception coming from lower EL.

Signed-off-by Julien Thierry <[email protected]>

Cc kvmarm at lists.cs.columbia.edu

---
  arch/arm64/kvm/hyp/hyp-entry.S | 17 ++++++++++++++---
  1 file changed, 14 insertions(+), 3 deletions(-)

arch/arm64/kvm/hyp/hyp-entry.S +8 -1

index 24b4fba..da31386 100644
@@ -257,7 +257,15 @@ ENTRY(__kvm_hyp_vector)
  ENDPROC(__kvm_hyp_vector)
 
  #ifdef CONFIG_KVM_INDIRECT_VECTORS
-.macro hyp_ventry
+.macro hyp_el2_ventry
+	.align 7
+1:	b	__kvm_hyp_vector + (1b - 0b)
+	.rept 31
+	nop
+	.endr
+.endm
+

arch/arm64/kvm/hyp/hyp-entry.S alternative_cb_end

Quoted 5 lines

arch/arm64/kvm/hyp/hyp-entry.S +1 -0

+.macro hyp_el1_ventry
  	.align 7
  1:	.rept 27
  	nop

@@ -290,8 +298,11 @@ alternative_cb_end

I think it would be cleaner to take the EL as a macro parameter, but again, I defer to Marc and Christoffer.

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned. The entry code needs to make sure that the stack is aligned before using it to save the context.

This is only a concern when taking exception from an EL using the same SP_ELx as the handler. In other cases it can be assumed that the SP being picked up on exception entry is aligned under the condition that SP is always aligned when doing eret to an EL using a different SP.

On Juno I see a runtime difference <1% for hackbench. If I do not include the fast path at EL1 (patch 4) I see a diff of 1-2%.

For EL2 entries, a bit of clean up of stuff getting patched in the vector has been needed.

Cheers,

Julien

-->

Julien Thierry (7):
arm64: Add static check for pt_regs alignment
arm64: sdei: Always use sdei stack for sdei events
arm64: Align stack when taking exception from EL1
arm64: Add fast-path for stack alignment
arm64: Do not apply BP hardening for hyp entries from EL2
arm64: Do not apply vector harderning for hyp entries from EL2
arm64: kvm: Align stack for exception coming from EL2

  arch/arm64/include/asm/assembler.h |  9 +++++
  arch/arm64/include/asm/ptrace.h    |  2 +
  arch/arm64/include/asm/sdei.h      |  2 -
  arch/arm64/kernel/cpu_errata.c     | 10 ++++-
  arch/arm64/kernel/entry.S          | 43 +++++++++++++++++++--
  arch/arm64/kernel/sdei.c           | 23 ++++-------
  arch/arm64/kvm/hyp/hyp-entry.S     | 78 +++++++++++++++++++++++++++++++-------
  drivers/firmware/Kconfig           |  1 +
  8 files changed, 132 insertions(+), 36 deletions(-)

--
1.9.1

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned. The entry code needs to make sure that the stack is aligned before using it to save the context.

Hi Julien,

Quoted 5 lines · Julien Thierry

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned. The entry code needs to make sure that the stack is aligned before using it to save the context.

Do you know what we haven't had reports of this crashing? Is it because GCC tends to keep the SP aligned anyway, so we're getting away with it for the moment? Trying to work out whether this is a candidate for -stable.

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

The
entry code needs to make sure that the stack is aligned before using it to save the context.

This is only a concern when taking exception from an EL using the same SP_ELx as the handler. In other cases it can be assumed that the SP being picked up on exception entry is aligned under the condition that SP is always aligned when doing eret to an EL using a different SP.

On Juno I see a runtime difference <1% for hackbench. If I do not include the fast path at EL1 (patch 4) I see a diff of 1-2%.

For EL2 entries, a bit of clean up of stuff getting patched in the vector has been needed.

Cheers,

Julien

-->

Julien Thierry (7):
arm64: Add static check for pt_regs alignment
arm64: sdei: Always use sdei stack for sdei events
arm64: Align stack when taking exception from EL1
arm64: Add fast-path for stack alignment
arm64: Do not apply BP hardening for hyp entries from EL2
arm64: Do not apply vector harderning for hyp entries from EL2
arm64: kvm: Align stack for exception coming from EL2

 arch/arm64/include/asm/assembler.h |  9 +++++
 arch/arm64/include/asm/ptrace.h    |  2 +
 arch/arm64/include/asm/sdei.h      |  2 -
 arch/arm64/kernel/cpu_errata.c     | 10 ++++-
 arch/arm64/kernel/entry.S          | 43 +++++++++++++++++++--
 arch/arm64/kernel/sdei.c           | 23 ++++-------
 arch/arm64/kvm/hyp/hyp-entry.S     | 78 +++++++++++++++++++++++++++++++-------
 drivers/firmware/Kconfig           |  1 +
 8 files changed, 132 insertions(+), 36 deletions(-)

--
1.9.1

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

Quoted 5 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 48 lines

The
entry code needs to make sure that the stack is aligned before using it to save the context.

This is only a concern when taking exception from an EL using the same SP_ELx as the handler. In other cases it can be assumed that the SP being picked up on exception entry is aligned under the condition that SP is always aligned when doing eret to an EL using a different SP.

On Juno I see a runtime difference <1% for hackbench. If I do not include the fast path at EL1 (patch 4) I see a diff of 1-2%.

For EL2 entries, a bit of clean up of stuff getting patched in the vector has been needed.

Cheers,

Julien

-->

Julien Thierry (7):
arm64: Add static check for pt_regs alignment
arm64: sdei: Always use sdei stack for sdei events
arm64: Align stack when taking exception from EL1
arm64: Add fast-path for stack alignment
arm64: Do not apply BP hardening for hyp entries from EL2
arm64: Do not apply vector harderning for hyp entries from EL2
arm64: kvm: Align stack for exception coming from EL2

  arch/arm64/include/asm/assembler.h |  9 +++++
  arch/arm64/include/asm/ptrace.h    |  2 +
  arch/arm64/include/asm/sdei.h      |  2 -
  arch/arm64/kernel/cpu_errata.c     | 10 ++++-
  arch/arm64/kernel/entry.S          | 43 +++++++++++++++++++--
  arch/arm64/kernel/sdei.c           | 23 ++++-------
  arch/arm64/kvm/hyp/hyp-entry.S     | 78 +++++++++++++++++++++++++++++++-------
  drivers/firmware/Kconfig           |  1 +
  8 files changed, 132 insertions(+), 36 deletions(-)

--
1.9.1

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

Quoted 11 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

Quoted 5 lines

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

The series can be dropped if there is enough confidence that this won't happen.

Thanks,

Quoted 43 lines

Quoted 43 lines

The
entry code needs to make sure that the stack is aligned before using it to
save the context.

This is only a concern when taking exception from an EL using the same SP_ELx as the handler. In other cases it can be assumed that the SP being picked up on exception entry is aligned under the condition that SP is always aligned when doing eret to an EL using a different SP.

On Juno I see a runtime difference <1% for hackbench. If I do not include the fast path at EL1 (patch 4) I see a diff of 1-2%.

For EL2 entries, a bit of clean up of stuff getting patched in the vector has been needed.

Cheers,

Julien

-->

Julien Thierry (7):
arm64: Add static check for pt_regs alignment
arm64: sdei: Always use sdei stack for sdei events
arm64: Align stack when taking exception from EL1
arm64: Add fast-path for stack alignment
arm64: Do not apply BP hardening for hyp entries from EL2
arm64: Do not apply vector harderning for hyp entries from EL2
arm64: kvm: Align stack for exception coming from EL2

  arch/arm64/include/asm/assembler.h |  9 +++++
  arch/arm64/include/asm/ptrace.h    |  2 +
  arch/arm64/include/asm/sdei.h      |  2 -
  arch/arm64/kernel/cpu_errata.c     | 10 ++++-
  arch/arm64/kernel/entry.S          | 43 +++++++++++++++++++--
  arch/arm64/kernel/sdei.c           | 23 ++++-------
  arch/arm64/kvm/hyp/hyp-entry.S     | 78
+++++++++++++++++++++++++++++++-------
  drivers/firmware/Kconfig           |  1 +
  8 files changed, 132 insertions(+), 36 deletions(-)

Quoted 7 lines

Quoted 7 lines

--
1.9.1

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

--
Julien Thierry

(+ Ramana)

Quoted 20 lines · Julien Thierry

Quoted 16 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 4 lines

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

I think we should get that clarified then. I don't think it makes sense for GCC to have to reason about whether SP currently has a value that permits dereferencing.

Quoted 23 lines · Ard Biesheuvel

(+ Ramana)

Quoted 20 lines · Julien Thierry

Quoted 16 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 8 lines

Quoted 4 lines

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

I think we should get that clarified then. I don't think it makes sense for GCC to have to reason about whether SP currently has a value that permits dereferencing.

The ABI gives that guarantee.

http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf

<quote>

5.2.2.1 Universal stack constraints

<...>

Additionally, at any point at which memory is accessed via SP, the hardware requires that SP mod 16 = 0. The stack must be quad-word aligned

</end quote>

Quoted 47 lines · Ramana Radhakrishnan

Quoted 23 lines · Ard Biesheuvel

(+ Ramana)

Quoted 20 lines · Julien Thierry

Quoted 16 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 8 lines

Quoted 4 lines

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

I think we should get that clarified then. I don't think it makes sense for GCC to have to reason about whether SP currently has a value that permits dereferencing.

The ABI gives that guarantee.

http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf

<quote>

5.2.2.1 Universal stack constraints

<...>

Additionally, at any point at which memory is accessed via SP, the hardware requires that SP mod 16 = 0. The stack must be quad-word aligned

Quoted 2 lines

</end quote>

Thanks Ramana. Sadly I don't think this clarifies things. The issue is that the guarantee is only that SP is aligned when we will use it to access memory, but still allows for a scenario like:

-> Updating SP with non 16bytes-aligned value (it's fine as long as the following code takes care to align it before accessing memory) -> IRQ is raised before SP gets aligned
-> Vector entry starts saving context on misaligned stack
-> Misaligned SP exception

The only thing that would avoid us the trouble is a guarantee that GCC never modifies SP in such a way that SP is not 16-bytes aligned.

Thanks,

Quoted 50 lines · Julien Thierry

Quoted 47 lines · Ramana Radhakrishnan

Quoted 23 lines · Ard Biesheuvel

(+ Ramana)

Quoted 20 lines · Julien Thierry

Quoted 16 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 8 lines

Quoted 4 lines

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

I think we should get that clarified then. I don't think it makes sense for GCC to have to reason about whether SP currently has a value that permits dereferencing.

The ABI gives that guarantee.

http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf

<quote>

5.2.2.1 Universal stack constraints

<...>

Additionally, at any point at which memory is accessed via SP, the hardware requires that SP mod 16 = 0. The stack must be quad-word aligned

Quoted 6 lines

Quoted 2 lines

</end quote>

Thanks Ramana. Sadly I don't think this clarifies things. The issue is that the guarantee is only that SP is aligned when we will use it to access memory, but still allows for a scenario like:

That is certainly a correct interpretation of the ABI language.

Quoted 9 lines

-> Updating SP with non 16bytes-aligned value (it's fine as long as the following code takes care to align it before accessing memory) -> IRQ is raised before SP gets aligned
-> Vector entry starts saving context on misaligned stack
-> Misaligned SP exception

The only thing that would avoid us the trouble is a guarantee that GCC never modifies SP in such a way that SP is not 16-bytes aligned.

I think it sort of falls out in the implementation from what I remember and see (and empirically checked with a couple of colleagues).

I don't think there is an ABI requirement that SP should left 16 byte aligned even for intermediate calculations.

Whether GCC does this or not today is immaterial and for userland it's certainly not the only code generator in town for this sort of question. The question also arises with other jits and other code generators which may leave the stack temporarily not aligned at 16 bytes. So it does sound like the right thing to do in the kernel defensively.

Quoted 23 lines · Ard Biesheuvel

(+ Ramana)

Quoted 20 lines · Julien Thierry

Quoted 16 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 8 lines

Quoted 4 lines

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

I think we should get that clarified then. I don't think it makes sense for GCC to have to reason about whether SP currently has a value that permits dereferencing.

The ABI gives that guarantee.

http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf

<quote>

Quoted 33 lines · Ramana Radhakrishnan

Quoted 31 lines · Ard Biesheuvel

(+ Ramana)

Quoted 20 lines · Julien Thierry

Quoted 11 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

Quoted 5 lines

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 4 lines

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

I think we should get that clarified then. I don't think it makes sense for GCC to have to reason about whether SP currently has a value that permits dereferencing.

Quoted 5 lines

The ABI gives that guarantee.

http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf

<quote>

Surely This only applies at public interfaces?

Quoted 38 lines · Dave Martin

Quoted 37 lines · Ramana Radhakrishnan

Quoted 23 lines · Ard Biesheuvel

(+ Ramana)

Quoted 20 lines · Julien Thierry

Quoted 16 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 8 lines

Quoted 4 lines

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

I think we should get that clarified then. I don't think it makes sense for GCC to have to reason about whether SP currently has a value that permits dereferencing.

The ABI gives that guarantee.

http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf

Quoted 4 lines

Quoted 1 line

<quote>

Surely This only applies at public interfaces?

I don't think this has anything to do with public interfaces. If there is a trap with a 16byte misaligned access of the SP then it doesn't matter whether it's a public interface or not.

Quoted 39 lines · Ramana Radhakrishnan

Quoted 38 lines · Dave Martin

Quoted 37 lines · Ramana Radhakrishnan

Quoted 23 lines · Ard Biesheuvel

(+ Ramana)

Quoted 20 lines · Julien Thierry

Quoted 16 lines · Ard Biesheuvel

Quoted 6 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned.

How is this possible? GCC clearly only manipulates the stack pointer in 16 byte multiples, and so if we do the same in our asm code (which I think we already do, given the lack of reports about this issue), is this handling really necessary?

Quoted 8 lines

Quoted 4 lines

Is there anything that actually gives us that guarantee from GCC? I agree that currently it looks like aarch64-<...>-gcc only manipulates SP aligned to 16 bytes, but I don't know whether that is certain.

I think we should get that clarified then. I don't think it makes sense for GCC to have to reason about whether SP currently has a value that permits dereferencing.

The ABI gives that guarantee.

http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

<quote>

Surely This only applies at public interfaces?

I don't think this has anything to do with public interfaces. If there is a trap with a 16byte misaligned access of the SP then it doesn't matter whether it's a public interface or not.

We're not talking about SP alignment faults here particluarly.

We're talking about any exception that may be taken from EL1h to EL1h, which may happen on random instructions inside a function, for random reasons.

Indeed.

But my question remains how likely it is that the compiler we use for generating the kernel code (so we are not talking about userland JITs or other crazy stuff here) would play with SP like that, especially since it is no longer a general purpose register. To me, it would make sense to attempt to reach an agreement with the GCC folks that compiler generated code does not muck about with SP like that.

Quoted 10 lines

Indeed.

But my question remains how likely it is that the compiler we use for generating the kernel code (so we are not talking about userland JITs or other crazy stuff here) would play with SP like that, especially since it is no longer a general purpose register. To me, it would make sense to attempt to reach an agreement with the GCC folks that compiler generated code does not muck about with SP like that.

It falls out from the choice of instructions we use for this sort of thing and because frame sizes really get rounded up to 16 bytes.

In the explanation below assume the stack is aligned to 16 bytes on entry. Frame sizes are always a multiple of 16.

For frame sizes up to 240 bytes that's a stp fp, lr, [sp, #-<off>]!

For frame sizes up to 4k bytes, that's a single sub instruction. Again all frame sizes are 16 byte aligned and therefore it's all ok.

For frame sizes greater than this and up to 64k that's a mov to a temporary register with a 16 bit immediate followed by a single subtract, again not going to leave your stack frame misaligned.

From frame sizes above that we split this into a subtract with a
multiple of 4k (again 16 byte aligned) and the rest.

For frame size above 16MB we use a sequence of mov / movk's with a temporary register and then do a single subtract of SP and therefore that's also fine.

I think for the sake of this conversation with respect to compiling the kernel, I think we can safely say that GCC will end up leaving the stack 16 byte aligned even with the intermediate computations and that shouldn't be an issue from my reading of the AArch64 backend.

So, probably move on but if you really want to be defensive you may want to carry this patch. However there's nothing that I will say about hand-written assembly (and I say that in the interest of completeness).

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned. The entry code needs to make sure that the stack is aligned before using it to save the context.

Hi,

Quoted 8 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned. The entry code needs to make sure that the stack is aligned before using it to save the context.

So, as discussed in this thread:
https://www.spinics.net/lists/arm-kernel/msg688342.html

We might have the compiler provide the guarantee that SP is kept at multiples of 16-bytes throughout C functions. If this is accepted, it avoids the complexity of this series.

There is just one case remaining, which is less important as it is mostly a debug concern. If we take an SP alignment fault from EL1 (due to bad implementation) we take recursive exceptions:

- Without CONFIG_VMAP_STACK, kernel just freezes always taking SP alignment faults
- With CONFIG_VMAP_STACK after taking a number of exceptions, we overflow the stack and the kernel switches to the overflow stack where it finally manages to display a kernel panic message

So the question is, do we care about doing something for the above case?

Quoted 16 lines · Julien Thierry

Hi,

Quoted 8 lines · Julien Thierry

Hi,

Having SCTLR_ELx.SA enabled requires the SP to be 16-bytes aligned before using it to access memory. When taking an exception, it is possible that the context during which the exception occured had SP mis-aligned. The entry code needs to make sure that the stack is aligned before using it to save the context.

So, as discussed in this thread:
https://www.spinics.net/lists/arm-kernel/msg688342.html

Quoted 4 lines

We might have the compiler provide the guarantee that SP is kept at multiples of 16-bytes throughout C functions. If this is accepted, it avoids the complexity of this series.

Yes, but we still want the build time sizeof(pt_regs) check and perhaps other pieces of this series.

Quoted 10 lines

There is just one case remaining, which is less important as it is mostly a debug concern. If we take an SP alignment fault from EL1 (due to bad implementation) we take recursive exceptions:

- Without CONFIG_VMAP_STACK, kernel just freezes always taking SP alignment faults
- With CONFIG_VMAP_STACK after taking a number of exceptions, we overflow the stack and the kernel switches to the overflow stack where it finally manages to display a kernel panic message

Quoted 2 lines

So the question is, do we care about doing something for the above case?

So in the latter case, it is obvious from the debug output that the stack pointer was misaligned? I'd expect so, given that each recursively taken exception has the same cause, and so it would be clear what happened.

So I'm leaning towards just relying on that, given that CONFIG_VMAP_STACK is the default, although it is unfortunate that KASAN still disables it.

Quoted 5 lines · Julien Thierry

On 26/09/18 14:56, Julien Thierry wrote:
There is just one case remaining, which is less important as it is mostly a debug concern. If we take an SP alignment fault from EL1 (due to bad implementation) we take recursive exceptions:

Quoted 8 lines

- Without CONFIG_VMAP_STACK, kernel just freezes always taking SP alignment faults
- With CONFIG_VMAP_STACK after taking a number of exceptions, we overflow the stack and the kernel switches to the overflow stack where it finally manages to display a kernel panic message

So the question is, do we care about doing something for the above case?

So in the latter case, it is obvious from the debug output that the stack pointer was misaligned? I'd expect so, given that each recursively taken exception has the same cause, and so it would be clear what happened.

So I'm leaning towards just relying on that, given that CONFIG_VMAP_STACK is the default, although it is unfortunate that KASAN still disables it.