Skip to contentSkip to conversation

lib/crypto: x86/sha: Follow the normal %rbp convention

PATCH Applied Eric Biggers 2 participants 6 messages Oct 11 – Oct 13

Subsystems (2)
lore
Share & tools
lore

Eric Biggers

author maintainer
#0 lore raw

cover lib/crypto: x86/sha: Follow the normal %rbp convention

This series updates the x86 SHA-1, SHA-256, and SHA-512 assembly code to set up %rbp before pushing GPRs rather than after, so that the frame pointer unwinder will generate call traces reliably.

This doesn't matter much, given that the ORC unwinder is the default, and the fallback to scanning the stack made call traces sort of "work" anyway. But it annoyed me that these assembly functions use the "wrong" %rbp convention, differing from the C code.

This series is targeting libcrypto-next.

Eric Biggers (3):
lib/crypto: x86/sha1: Follow the normal %rbp convention lib/crypto: x86/sha256: Follow the normal %rbp convention lib/crypto: x86/sha512: Follow the normal %rbp convention

 lib/crypto/x86/sha1-avx2-asm.S      | 10 +++++-----
 lib/crypto/x86/sha1-ssse3-and-avx.S |  9 +++++----
 lib/crypto/x86/sha256-avx-asm.S     | 10 +++++-----
 lib/crypto/x86/sha256-avx2-asm.S    | 10 ++++------
 lib/crypto/x86/sha256-ni-asm.S      |  9 +++++----
 lib/crypto/x86/sha256-ssse3-asm.S   |  8 ++++----
 lib/crypto/x86/sha512-avx-asm.S     | 11 ++++++-----
 lib/crypto/x86/sha512-avx2-asm.S    | 11 ++++++-----
 lib/crypto/x86/sha512-ssse3-asm.S   | 11 ++++++-----
 9 files changed, 46 insertions(+), 43 deletions(-)

base-commit: 6a50ce0af5c91fd665d65d357af8ae55db1df7d0

Signature · 1 line
2.56.0
author maintainer
#1 lore raw

PATCH 1/3 lib/crypto: x86/sha1: Follow the normal %rbp convention

Some of the SHA-1 assembly functions push callee-save GPRs to the stack *before* they set up the frame pointer (%rbp). (They are leaf functions, but they need to set up a frame pointer anyway because they allocate a 16- or 32-byte-aligned buffer on the stack.)

As a result, during these functions, 8(%rbp) doesn't contain a return address like it is supposed to.

This differs from the convention used by C code, and it interferes with the frame pointer unwinder (CONFIG_UNWINDER_FRAME_POINTER=y). Specifically, if a call trace is dumped in these functions, then all entries are populated purely from the fallback scan of the entire stack:

    RIP: 0010:sha1_transform_avx2+0x1921/0x1ea0
    [...]
    Call Trace:
     <TASK>
     ? sha1_transform_avx+0x2a9/0xc90
     ? sha1_blocks_avx2+0x78/0x140
     ? kernel_fpu_end+0x2d/0x40
     ? sha1_blocks_avx2+0x52/0x140
     ? sha1_update+0x77/0x220
     [...]
     ? do_syscall_64+0x6f/0x3e0

With this fix to follow the normal %rbp convention, the call trace is generated reliably:

    RIP: 0010:sha1_transform_avx2+0x66e/0x1ea0
    [...]
    Call Trace:
     <TASK>
     sha1_blocks_avx2+0x78/0x140
     sha1_update+0x77/0x220
     [...]
     do_syscall_64+0x6f/0x3e0
---
 lib/crypto/x86/sha1-avx2-asm.S      | 10 +++++-----
 lib/crypto/x86/sha1-ssse3-and-avx.S |  9 +++++----
 2 files changed, 10 insertions(+), 9 deletions(-)
lib/crypto/x86/sha1-avx2-asm.S +5 -5
index 91b2dc6db179..ca64d7104a47 100644
@@ -622,6 +622,9 @@
 .macro SHA1_VECTOR_ASM  name
 	SYM_FUNC_START(\name)
 
+	push	%rbp
+	mov	%rsp, %rbp
+
 	push	%rbx
 	push	%r12
 	push	%r13
@@ -631,8 +634,6 @@
 	RESERVE_STACK  = (W_SIZE*4 + 8+24)
 
 	/* Align stack */
-	push	%rbp
-	mov	%rsp, %rbp
 	and	$~(0x20-1), %rsp
 	sub	$RESERVE_STACK, %rsp
 
@@ -651,14 +652,13 @@
 
 	avx2_zeroupper
 
-	mov	%rbp, %rsp
-	pop	%rbp
-
+	lea	-40(%rbp), %rsp
 	pop	%r15
 	pop	%r14
 	pop	%r13
 	pop	%r12
 	pop	%rbx
+	pop	%rbp
 
 	RET
 
lib/crypto/x86/sha1-ssse3-and-avx.S +5 -4
index 78b48cb09c5b..fe0faaf494c7 100644
@@ -69,11 +69,12 @@
 .macro SHA1_VECTOR_ASM  name
 	SYM_FUNC_START(\name)
 
-	push	%rbx
-	push	%r12
 	push	%rbp
 	mov	%rsp, %rbp
 
+	push	%rbx
+	push	%r12
+
 	sub	$64, %rsp		# allocate workspace
 	and	$~15, %rsp		# align stack
 
@@ -95,10 +96,10 @@
 	xor	%eax, %eax
 	rep stosq
 
-	mov	%rbp, %rsp		# deallocate workspace
-	pop	%rbp
+	lea	-16(%rbp), %rsp		# deallocate workspace
 	pop	%r12
 	pop	%rbx
+	pop	%rbp
 	RET
 
 	SYM_FUNC_END(\name)
Signature · 1 line
2.56.0
author maintainer
#2 lore raw

PATCH 2/3 lib/crypto: x86/sha256: Follow the normal %rbp convention

In reply to Eric Biggers

Some of the SHA-256 assembly functions push callee-save GPRs to the stack *before* they set up the frame pointer (%rbp). (They are leaf functions, but they need to set up a frame pointer anyway because they allocate a 16- or 32-byte-aligned buffer on the stack.)

As a result, during these functions, 8(%rbp) doesn't contain a return address like it is supposed to.

This differs from the convention used by C code, and it interferes with the frame pointer unwinder (CONFIG_UNWINDER_FRAME_POINTER=y). Specifically, if a call trace is dumped in these functions, then all entries are populated purely from the fallback scan of the entire stack:

    RIP: 0010:sha256_transform_rorx+0xd78/0x1120
    [...]
    Call Trace:
     <TASK>
     ? sha256_blocks_avx2+0x37/0x70
     ? __sha256_update+0x77/0x220
     ? sha256+0x9d/0x110
     [...]
     ? do_syscall_64+0x6f/0x3e0

With this fix to follow the normal %rbp convention, the call trace is generated reliably:

    RIP: 0010:sha256_transform_rorx+0xdae/0x1120
    [...]
    Call Trace:
     <TASK>
     sha256_blocks_avx2+0x37/0x70
     __sha256_update+0x77/0x220
     sha256+0x9d/0x110
     [...]
     do_syscall_64+0x6f/0x3e0
---
 lib/crypto/x86/sha256-avx-asm.S   | 10 +++++-----
 lib/crypto/x86/sha256-avx2-asm.S  | 10 ++++------
 lib/crypto/x86/sha256-ni-asm.S    |  9 +++++----
 lib/crypto/x86/sha256-ssse3-asm.S |  8 ++++----
 4 files changed, 18 insertions(+), 19 deletions(-)
lib/crypto/x86/sha256-avx-asm.S +5 -5
index c1aceb3ba3a3..efb8dff5e8a9 100644
@@ -345,13 +345,13 @@ a = TMP_
 ########################################################################
 .text
 SYM_FUNC_START(sha256_transform_avx)
+	pushq   %rbp
+	movq    %rsp, %rbp
 	pushq   %rbx
 	pushq   %r12
 	pushq   %r13
 	pushq   %r14
 	pushq   %r15
-	pushq	%rbp
-	movq	%rsp, %rbp
 
 	subq    $STACK_SIZE, %rsp	# allocate stack space
 	and	$~15, %rsp		# align stack pointer
@@ -445,13 +445,13 @@ SYM_FUNC_START(sha256_transform_avx)
 	cmp     _INP_END(%rsp), INP
 	jne     .Lloop0
 
-	mov	%rbp, %rsp
-	popq	%rbp
+	lea     -40(%rbp), %rsp
 	popq    %r15
 	popq    %r14
 	popq    %r13
-	popq	%r12
+	popq    %r12
 	popq    %rbx
+	popq    %rbp
 	RET
 SYM_FUNC_END(sha256_transform_avx)
 
lib/crypto/x86/sha256-avx2-asm.S +4 -6
index eb8836fb9695..a719c2173b61 100644
@@ -522,15 +522,14 @@ STACK_SIZE	= _CTX      + _CTX_SIZE
 ########################################################################
 .text
 SYM_FUNC_START(sha256_transform_rorx)
+	pushq	%rbp
+	mov	%rsp, %rbp
 	pushq	%rbx
 	pushq	%r12
 	pushq	%r13
 	pushq	%r14
 	pushq	%r15
 
-	push	%rbp
-	mov	%rsp, %rbp
-
 	subq	$STACK_SIZE, %rsp
 	and	$-32, %rsp	# align rsp to 32 byte boundary
 
@@ -704,14 +703,13 @@ SYM_FUNC_START(sha256_transform_rorx)
 
 .Ldone_hash:
 
-	mov	%rbp, %rsp
-	pop	%rbp
-
+	lea	-40(%rbp), %rsp
 	popq	%r15
 	popq	%r14
 	popq	%r13
 	popq	%r12
 	popq	%rbx
+	popq	%rbp
 	vzeroupper
 	RET
 SYM_FUNC_END(sha256_transform_rorx)
lib/crypto/x86/sha256-ni-asm.S +5 -4
index de5f707e7ef7..e47263273f43 100644
@@ -276,10 +276,11 @@ SYM_FUNC_END(sha256_ni_transform)
 // to taking better advantage of the CPU's SHA-256 and SIMD throughput.
 //
 SYM_FUNC_START(sha256_ni_finup2x)
-	// Allocate 128 bytes of stack space, 16-byte aligned.
-	push		%rbx
 	push		%rbp
 	mov		%rsp, %rbp
+	push		%rbx
+
+	// Allocate 128 bytes of stack space, 16-byte aligned.
 	sub		$128, %rsp
 	and		$~15, %rsp
 
@@ -527,9 +528,9 @@ SYM_FUNC_START(sha256_ni_finup2x)
 	movdqu		STATE1_A, 0*16(OUT1)
 	movdqu		STATE1_B, 0*16(OUT2)
 
-	mov		%rbp, %rsp
-	pop		%rbp
+	lea		-8(%rbp), %rsp
 	pop		%rbx
+	pop		%rbp
 	RET
 SYM_FUNC_END(sha256_ni_finup2x)
 
lib/crypto/x86/sha256-ssse3-asm.S +4 -4
index 383b8eec7ebe..cc91b3e15a83 100644
@@ -352,13 +352,13 @@ a = TMP_
 ########################################################################
 .text
 SYM_FUNC_START(sha256_transform_ssse3)
+	pushq   %rbp
+	mov     %rsp, %rbp
 	pushq   %rbx
 	pushq   %r12
 	pushq   %r13
 	pushq   %r14
 	pushq   %r15
-	pushq   %rbp
-	mov	%rsp, %rbp
 
 	subq    $STACK_SIZE, %rsp
 	and	$~15, %rsp
@@ -456,13 +456,13 @@ SYM_FUNC_START(sha256_transform_ssse3)
 	cmp     _INP_END(%rsp), INP
 	jne     .Lloop0
 
-	mov	%rbp, %rsp
-	popq	%rbp
+	lea     -40(%rbp), %rsp
 	popq    %r15
 	popq    %r14
 	popq    %r13
 	popq    %r12
 	popq    %rbx
+	popq    %rbp
 
 	RET
 SYM_FUNC_END(sha256_transform_ssse3)
Signature · 1 line
2.56.0
author maintainer
#3 lore raw

PATCH 3/3 lib/crypto: x86/sha512: Follow the normal %rbp convention

In reply to Eric Biggers

The SHA-512 assembly functions push callee-save GPRs to the stack *before* they set up the frame pointer (%rbp). (They are leaf functions, but they need to set up a frame pointer anyway because they allocate a 32-byte-aligned buffer on the stack.)

As a result, during these functions, 8(%rbp) doesn't contain a return address like it is supposed to.

This differs from the convention used by C code, and it interferes with the frame pointer unwinder (CONFIG_UNWINDER_FRAME_POINTER=y). Specifically, if a call trace is dumped in these functions, then all entries are populated purely from the fallback scan of the entire stack:

    RIP: 0010:sha512_transform_rorx+0x673/0xdf0
    [...]
    Call Trace:
     <TASK>
     ? sha512_blocks_avx2+0x37/0x60
     ? __sha512_update+0x80/0x230
     ? sha512+0x31/0x190
     ? sha512+0x114/0x190
     [...]
     ? do_syscall_64+0x6f/0x3e0

With this fix to follow the normal %rbp convention, the call trace is generated reliably:

    RIP: 0010:sha512_transform_rorx+0x90c/0xdf0
    [...]
    Call Trace:
     <TASK>
     sha512_blocks_avx2+0x37/0x60
     __sha512_update+0x80/0x230
     sha512+0x114/0x190
     [...]
     do_syscall_64+0x6f/0x3e0
---
 lib/crypto/x86/sha512-avx-asm.S   | 11 ++++++-----
 lib/crypto/x86/sha512-avx2-asm.S  | 11 ++++++-----
 lib/crypto/x86/sha512-ssse3-asm.S | 11 ++++++-----
 3 files changed, 18 insertions(+), 15 deletions(-)
lib/crypto/x86/sha512-avx-asm.S +6 -5
index 7732aa8fd850..a17dfedc1c51 100644
@@ -276,6 +276,9 @@ frame_size = frame_WK + WK_SIZE
 ########################################################################
 SYM_FUNC_START(sha512_transform_avx)
 
+	push	%rbp
+	mov	%rsp, %rbp
+
 	# Save GPRs
 	push	%rbx
 	push	%r12
@@ -284,8 +287,6 @@ SYM_FUNC_START(sha512_transform_avx)
 	push	%r15
 
 	# Allocate Stack Space
-	push	%rbp
-	mov	%rsp, %rbp
 	sub     $frame_size, %rsp
 	and	$~(0x20 - 1), %rsp
 
@@ -348,9 +349,8 @@ SYM_FUNC_START(sha512_transform_avx)
 	dec     msglen
 	jnz     .Lupdateblock
 
-	# Restore Stack Pointer
-	mov	%rbp, %rsp
-	pop	%rbp
+	# Deallocate Stack Space
+	lea	-40(%rbp), %rsp
 
 	# Restore GPRs
 	pop	%r15
@@ -358,6 +358,7 @@ SYM_FUNC_START(sha512_transform_avx)
 	pop	%r13
 	pop	%r12
 	pop	%rbx
+	pop	%rbp
 
 	RET
 SYM_FUNC_END(sha512_transform_avx)
lib/crypto/x86/sha512-avx2-asm.S +6 -5
index 22bdbfd899d0..2256d51297e1 100644
@@ -568,6 +568,9 @@ frame_size = frame_CTX + CTX_SIZE
 ########################################################################
 SYM_FUNC_START(sha512_transform_rorx)
 
+	push	%rbp
+	mov	%rsp, %rbp
+
 	# Save GPRs
 	push	%rbx
 	push	%r12
@@ -576,8 +579,6 @@ SYM_FUNC_START(sha512_transform_rorx)
 	push	%r15
 
 	# Allocate Stack Space
-	push	%rbp
-	mov	%rsp, %rbp
 	sub	$frame_size, %rsp
 	and	$~(0x20 - 1), %rsp
 
@@ -667,9 +668,8 @@ SYM_FUNC_START(sha512_transform_rorx)
 	cmp	frame_INPEND(%rsp), INP
 	jne	.Lloop0
 
-	# Restore Stack Pointer
-	mov	%rbp, %rsp
-	pop	%rbp
+	# Deallocate Stack Space
+	lea	-40(%rbp), %rsp
 
 	# Restore GPRs
 	pop	%r15
@@ -677,6 +677,7 @@ SYM_FUNC_START(sha512_transform_rorx)
 	pop	%r13
 	pop	%r12
 	pop	%rbx
+	pop	%rbp
 
 	vzeroupper
 	RET
lib/crypto/x86/sha512-ssse3-asm.S +6 -5
index 4cae7445b2a8..1a37b7a735cf 100644
@@ -275,6 +275,9 @@ frame_size = frame_WK + WK_SIZE
 ########################################################################
 SYM_FUNC_START(sha512_transform_ssse3)
 
+	push	%rbp
+	mov	%rsp, %rbp
+
 	# Save GPRs
 	push	%rbx
 	push	%r12
@@ -283,8 +286,6 @@ SYM_FUNC_START(sha512_transform_ssse3)
 	push	%r15
 
 	# Allocate Stack Space
-	push	%rbp
-	mov	%rsp, %rbp
 	sub	$frame_size, %rsp
 	and	$~(0x20 - 1), %rsp
 
@@ -347,9 +348,8 @@ SYM_FUNC_START(sha512_transform_ssse3)
 	dec	msglen
 	jnz	.Lupdateblock
 
-	# Restore Stack Pointer
-	mov	%rbp, %rsp
-	pop	%rbp
+	# Deallocate Stack Space
+	lea	-40(%rbp), %rsp
 
 	# Restore GPRs
 	pop	%r15
@@ -357,6 +357,7 @@ SYM_FUNC_START(sha512_transform_ssse3)
 	pop	%r13
 	pop	%r12
 	pop	%rbx
+	pop	%rbp
 
 	RET
 SYM_FUNC_END(sha512_transform_ssse3)
Signature · 1 line
2.56.0
maintainer Reviewed-by on the v1 series
#4 lore raw
In reply to Eric Biggers
Quoted 9 lines · Eric Biggers

This series updates the x86 SHA-1, SHA-256, and SHA-512 assembly code to set up %rbp before pushing GPRs rather than after, so that the frame pointer unwinder will generate call traces reliably.

This doesn't matter much, given that the ORC unwinder is the default, and the fallback to scanning the stack made call traces sort of "work" anyway. But it annoyed me that these assembly functions use the "wrong" %rbp convention, differing from the C code.

Quoted 7 lines

This series is targeting libcrypto-next.

Eric Biggers (3):
lib/crypto: x86/sha1: Follow the normal %rbp convention lib/crypto: x86/sha256: Follow the normal %rbp convention lib/crypto: x86/sha512: Follow the normal %rbp convention

author maintainer Applied
#5 lore raw
In reply to Eric Biggers
Quoted 16 lines · Eric Biggers

This series updates the x86 SHA-1, SHA-256, and SHA-512 assembly code to set up %rbp before pushing GPRs rather than after, so that the frame pointer unwinder will generate call traces reliably.

This doesn't matter much, given that the ORC unwinder is the default, and the fallback to scanning the stack made call traces sort of "work" anyway. But it annoyed me that these assembly functions use the "wrong" %rbp convention, differing from the C code.

This series is targeting libcrypto-next.

Eric Biggers (3):
lib/crypto: x86/sha1: Follow the normal %rbp convention lib/crypto: x86/sha256: Follow the normal %rbp convention lib/crypto: x86/sha512: Follow the normal %rbp convention

Quoted 10 lines
 lib/crypto/x86/sha1-avx2-asm.S      | 10 +++++-----
 lib/crypto/x86/sha1-ssse3-and-avx.S |  9 +++++----
 lib/crypto/x86/sha256-avx-asm.S     | 10 +++++-----
 lib/crypto/x86/sha256-avx2-asm.S    | 10 ++++------
 lib/crypto/x86/sha256-ni-asm.S      |  9 +++++----
 lib/crypto/x86/sha256-ssse3-asm.S   |  8 ++++----
 lib/crypto/x86/sha512-avx-asm.S     | 11 ++++++-----
 lib/crypto/x86/sha512-avx2-asm.S    | 11 ++++++-----
 lib/crypto/x86/sha512-ssse3-asm.S   | 11 ++++++-----
 9 files changed, 46 insertions(+), 43 deletions(-)

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/log/?h=libcrypto-next

- Eric