diff mbox series

[v2,5/5] crypto: x86/crc32c - Tweak jump table to validate objtool logic

Message ID 20241010122801.1321976-12-ardb+git@google.com (mailing list archive)
State Superseded
Headers show
Series Improve objtool jump table handling | expand

Commit Message

Ard Biesheuvel Oct. 10, 2024, 12:28 p.m. UTC 
From: Ard Biesheuvel <ardb@kernel.org>

Tweak the jump table so
- the address is taken far way from its use
- its offset from the start of .rodata is != 0x0
- its type is STT_OBJECT and its size is set to the size of the actual
  table
- the indirect jump is annotated with a R_X86_64_NONE relocation
  pointing to the jump table

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/crypto/crc32c-pcl-intel-asm_64.S | 39 +++++++++++---------
 1 file changed, 22 insertions(+), 17 deletions(-)

Comments

Josh Poimboeuf Oct. 10, 2024, 8:34 p.m. UTC | #1 
On Thu, Oct 10, 2024 at 02:28:07PM +0200, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Tweak the jump table so
> - the address is taken far way from its use
> - its offset from the start of .rodata is != 0x0
> - its type is STT_OBJECT and its size is set to the size of the actual
>   table
> - the indirect jump is annotated with a R_X86_64_NONE relocation
>   pointing to the jump table
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

This needs more "why", I assume the goals are to add the annotations +
confuse objtool if it doesn't read them properly?
Ard Biesheuvel Oct. 11, 2024, 6:32 a.m. UTC | #2 
On Thu, 10 Oct 2024 at 22:34, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Thu, Oct 10, 2024 at 02:28:07PM +0200, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Tweak the jump table so
> > - the address is taken far way from its use
> > - its offset from the start of .rodata is != 0x0
> > - its type is STT_OBJECT and its size is set to the size of the actual
> >   table
> > - the indirect jump is annotated with a R_X86_64_NONE relocation
> >   pointing to the jump table
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> This needs more "why", I assume the goals are to add the annotations +
> confuse objtool if it doesn't read them properly?
>

As presented, this is just a vehicle to test the other changes in the
series. That is why I split it off from the previous one.

Whether or not we want this code in the tree is up for debate, but I
guess it could be useful as a canary for objtool, given that most
configs now disable jump tables entirely.
Josh Poimboeuf Oct. 11, 2024, 4:04 p.m. UTC | #3 
On Fri, Oct 11, 2024 at 08:32:33AM +0200, Ard Biesheuvel wrote:
> On Thu, 10 Oct 2024 at 22:34, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >
> > On Thu, Oct 10, 2024 at 02:28:07PM +0200, Ard Biesheuvel wrote:
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > Tweak the jump table so
> > > - the address is taken far way from its use
> > > - its offset from the start of .rodata is != 0x0
> > > - its type is STT_OBJECT and its size is set to the size of the actual
> > >   table
> > > - the indirect jump is annotated with a R_X86_64_NONE relocation
> > >   pointing to the jump table
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > This needs more "why", I assume the goals are to add the annotations +
> > confuse objtool if it doesn't read them properly?
> >
> 
> As presented, this is just a vehicle to test the other changes in the
> series. That is why I split it off from the previous one.
> 
> Whether or not we want this code in the tree is up for debate, but I
> guess it could be useful as a canary for objtool, given that most
> configs now disable jump tables entirely.

The annotations are definitely needed since that's the future of jump
table handling.

The rest of the changes aren't worth the effort IMO.  In general we
don't compromise code quality to make objtool happy.

And "unit test for deprecated jump table detection" is even less of a
justification than would be something like "objtool can't otherwise
follow the code".
Ard Biesheuvel Oct. 11, 2024, 4:22 p.m. UTC | #4 
On Fri, 11 Oct 2024 at 18:05, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Fri, Oct 11, 2024 at 08:32:33AM +0200, Ard Biesheuvel wrote:
> > On Thu, 10 Oct 2024 at 22:34, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > >
> > > On Thu, Oct 10, 2024 at 02:28:07PM +0200, Ard Biesheuvel wrote:
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > >
> > > > Tweak the jump table so
> > > > - the address is taken far way from its use
> > > > - its offset from the start of .rodata is != 0x0
> > > > - its type is STT_OBJECT and its size is set to the size of the actual
> > > >   table
> > > > - the indirect jump is annotated with a R_X86_64_NONE relocation
> > > >   pointing to the jump table
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > This needs more "why", I assume the goals are to add the annotations +
> > > confuse objtool if it doesn't read them properly?
> > >
> >
> > As presented, this is just a vehicle to test the other changes in the
> > series. That is why I split it off from the previous one.
> >
> > Whether or not we want this code in the tree is up for debate, but I
> > guess it could be useful as a canary for objtool, given that most
> > configs now disable jump tables entirely.
>
> The annotations are definitely needed since that's the future of jump
> table handling.
>

Yeah, I'll split those off

> The rest of the changes aren't worth the effort IMO.  In general we
> don't compromise code quality to make objtool happy.
>
> And "unit test for deprecated jump table detection" is even less of a
> justification than would be something like "objtool can't otherwise
> follow the code".
>

No, quite the opposite - the changes will confuse objtool and so it
will only work correctly if the annotation is provided. That was the
point of this patch, but as I said, I never intended those changes to
be merged.
diff mbox series

Patch

diff --git a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
index 45b005935194..ba1cca66875b 100644
--- a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
+++ b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
@@ -93,10 +93,13 @@  SYM_FUNC_START(crc_pcl)
 #define    crc1		%r9
 #define    crc2		%r10
 
+	pushq	%rbp
 	pushq   %rbx
 	pushq   %rdi
 	pushq   %rsi
 
+	leaq	jump_table(%rip), %rbp
+
 	## Move crc_init for Linux to a different
 	mov     crc_init_arg, crc_init
 
@@ -168,9 +171,9 @@  SYM_FUNC_START(crc_pcl)
 	xor     crc2, crc2
 
 	## branch into array
-	leaq	jump_table(%rip), %bufp
-	movslq	(%bufp,%rax,4), len
-	addq	len, %bufp
+	movslq	(%rbp,%rax,4), %bufp
+	addq	%rbp, %bufp
+	.reloc	., R_X86_64_NONE, jump_table
 	JMP_NOSPEC bufp
 
 	################################################################
@@ -310,24 +313,11 @@  LABEL less_than_ %j			# less_than_j: Length should be in
 	popq    %rsi
 	popq    %rdi
 	popq    %rbx
+	popq    %rbp
         RET
 SYM_FUNC_END(crc_pcl)
 
 .section	.rodata, "a", @progbits
-        ################################################################
-        ## jump table        Table is 129 entries x 2 bytes each
-        ################################################################
-.align 4
-jump_table:
-	i=0
-.rept 129
-.altmacro
-JMPTBL_ENTRY %i
-.noaltmacro
-	i=i+1
-.endr
-
-
 	################################################################
 	## PCLMULQDQ tables
 	## Table is 128 entries x 2 words (8 bytes) each
@@ -462,3 +452,18 @@  K_table:
 	.long 0x45cddf4e, 0xe0ac139e
 	.long 0xacfa3103, 0x6c23e841
 	.long 0xa51b6135, 0x170076fa
+
+        ################################################################
+        ## jump table        Table is 129 entries x 2 bytes each
+        ################################################################
+.align 4
+jump_table:
+	i=0
+.rept 129
+.altmacro
+JMPTBL_ENTRY %i
+.noaltmacro
+	i=i+1
+.endr
+.size	jump_table, . - jump_table
+.type	jump_table, @object