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(-)
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
--
2.47.0.rc0.187.ge670bccf7e-googOn 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? -- Josh
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.
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". -- Josh
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.
© 2016 - 2024 Red Hat, Inc.