| Message ID | 20200109160300.26150-1-jthierry@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | objtool: Add support for arm64 | expand |
On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: > Hi, > > This patch series is the continuation of Raphael's work [1]. All the > patches can be retrieved from: > git clone -b arm64-objtool-v5 https://github.com/julien-thierry/linux.git > > There still are some outstanding issues but the series is starting to > get pretty big so it is probably good to start having some discussions > on the current state of things. > > It felt necessary to split some of the patches (especially the arm64 > decoder). In order to give Raphael credit for his work I used the > "Suggested-by" tag. If this is not the right way to give credit or if > it should be present on more patches do let me know. > > There still are some shortcomings. On defconfig here are the remaining > warnings: > * arch/arm64/crypto/crct10dif-ce-core.o: warning: objtool: crc_t10dif_pmull_p8()+0xf0: unsupported intra-function call > * arch/arm64/kernel/cpu_errata.o: warning: objtool: qcom_link_stack_sanitization()+0x4: unsupported intra-function call > Objtool currently does not support bl from a procedure to itself. This > is also an issue with retpolines. I need to investigate more to figure > out whether something can be done for this or if this file should not be > validated by objtool. > > * arch/arm64/kernel/efi-entry.o: warning: objtool: entry()+0xb0: sibling call from callable instruction with modified stack frame > The EFI entry jumps to code mapped by EFI. Objtool cannot know statically where the code flow is going. > > * arch/arm64/kernel/entry.o: warning: objtool: .entry.tramp.text+0x404: unsupported intra-function call > Need to figure out what is needed to handle aarch64 trampolines. x86 > explicitly annotates theirs with ANNOTATE_NOSPEC_ALTERNATIVE and > patching them as alternatives. > > * arch/arm64/kernel/head.o: warning: objtool: .head.text+0x58: can't find jump dest instruction at .head.text+0x80884 > This is actually a constant that turns out to be a valid branch opcode. > A possible solution could be to introduce a marco that explicitly > annotates constants placed in code sections. > > * arch/arm64/kernel/hibernate-asm.o: warning: objtool: el1_sync()+0x4: unsupported instruction in callable function > Symbols el<x>_* shouldn't be considered as callable functions. Should we > use SYM_CODE_END instead of PROC_END? > > * arch/arm64/kvm/hyp/hyp-entry.o: warning: objtool: .hyp.text: empty alternative at end of section > This is due to the arm64 alternative_cb. Currently, the feature > corresponding to the alternative_cb is defined as the current number of > features supported by the kernel, meaning the identifier is not fixed > accross kernel versions. This makes it a bit hard to detect these > alternative_cb for external tools. > > Would it be acceptable to set a fixed identifier for alternative_cb? > (probably 0xFF so it is always higher than the number of real features) > > * drivers/ata/libata-scsi.o: warning: objtool: ata_sas_queuecmd() falls through to next function ata_scsi_scan_host() > This is due to a limitation in the switch table metadata interpretation. > The compiler might create a table of unsigned offsets and then > compute the final offset as follows: > > ldrb offset_reg, [<offset_table>, <offset_idx>, uxtw] > adr base_reg, <base_addr> > add res_addr, base_reg, offset_reg, sxtb #2 > > Effectively using the loaded offset as a signed value. > I don't have a simple way to solve this at the moment, I'd like to > avoid decoding the instructions to check which ones might sign extend > the loaded offset. > > * kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x44: sibling call from callable instruction with modified stack frame > This is because the function uses a C jump table which differ from > basic jump tables. Also, the code generated for C jump tables on arm64 > does not follow the same form as the one for x86. So the existing x86 objtool > code handling C jump tables can't be used. > > I'll focus on understanding the arm64 pattern so objtool can handle them. > > > In the mean time, any feedback on the current state is appreciated. > > * Patches 1 to 18 adapts the current objtool code to make it easier to > support new architectures. > * Patches 19 to 45 add the support for arm64 architecture to objtool. > * Patches 46 to 57 fix warnings reported by objtool on the existing > arm64 code. > > Changes since RFCv4[1]: > * Rebase on v5.5-rc5 > * Misc cleanup/bug fixes > * Fix some new objtool warnings reported on arm64 objects > * Make ORC subcommand optional since arm64 does not currently support it > * Support branch instructions in alternative sections when they jump > within the same set of alternative instructions > * Replace the "extra" stack_op with a list of stack_op > * Split the decoder into multiple patches to ease review > * Mark constants generated by load literal instructions as bytes that > should not be reached by execution flow > * Rework the switch table handling > > [1] https://lkml.org/lkml/2019/8/16/400 > > Thanks, > > Julien > > --> > > Julien Thierry (43): > objtool: check: Remove redundant checks on operand type > objtool: check: Clean instruction state before each function > validation > objtool: check: Use arch specific values in restore_reg() > objtool: check: Ignore empty alternative groups > objtool: Give ORC functions consistent name > objtool: Make ORC support optional > objtool: Split generic and arch specific CFI definitions > objtool: Abstract alternative special case handling > objtool: check: Allow jumps from an alternative group to itself > objtool: Do not look for STT_NOTYPE symbols > objtool: Support addition to set frame pointer > objtool: Support restoring BP from the stack without POP > objtool: Make stack validation more generic > objtool: Support multiple stack_op per instruction > objtool: arm64: Decode unknown instructions > objtool: arm64: Decode simple data processing instructions > objtool: arm64: Decode add/sub immediate instructions > objtool: arm64: Decode logical data processing instructions > objtool: arm64: Decode system instructions not affecting the flow > objtool: arm64: Decode calls to higher EL > objtool: arm64: Decode brk instruction > objtool: arm64: Decode instruction triggering context switch > objtool: arm64: Decode branch instructions with PC relative immediates > objtool: arm64: Decode branch to register instruction > objtool: arm64: Decode basic load/stores > objtool: arm64: Decode load/store with register offset > objtool: arm64: Decode load/store register pair instructions > objtool: arm64: Decode FP/SIMD load/store instructions > objtool: arm64: Decode load/store exclusive > objtool: arm64: Decode atomic load/store > objtool: arm64: Decode pointer auth load instructions > objtool: arm64: Decode load acquire/store release > objtool: arm64: Decode load/store with memory tag > objtool: arm64: Decode load literal > objtool: arm64: Decode register data processing instructions > objtool: arm64: Decode FP/SIMD data processing instructions > objtool: arm64: Decode SVE instructions > objtool: arm64: Implement functions to add switch tables alternatives > arm64: Generate no-ops to pad executable section > arm64: Move constant to rodata > arm64: Mark sigreturn32.o as containing non standard code > arm64: entry: Avoid empty alternatives entries > arm64: crypto: Remove redundant branch > > Raphael Gault (14): > objtool: Add abstraction for computation of symbols offsets > objtool: orc: Refactor ORC API for other architectures to implement. > objtool: Move registers and control flow to arch-dependent code > objtool: Refactor switch-tables code to support other architectures > objtool: arm64: Add required implementation for supporting the aarch64 > architecture in objtool. > gcc-plugins: objtool: Add plugin to detect switch table on arm64 > objtool: arm64: Enable stack validation for arm64 > arm64: alternative: Mark .altinstr_replacement as containing > executable instructions > arm64: assembler: Add macro to annotate asm function having non > standard stack-frame. > arm64: sleep: Prevent stack frame warnings from objtool > arm64: kvm: Annotate non-standard stack frame functions > arm64: kernel: Add exception on kuser32 to prevent stack analysis > arm64: crypto: Add exceptions for crypto object to prevent stack > analysis > arm64: kernel: Annotate non-standard stack frame functions > > arch/arm64/Kconfig | 2 + > arch/arm64/crypto/Makefile | 3 + > arch/arm64/crypto/sha1-ce-core.S | 3 +- > arch/arm64/crypto/sha2-ce-core.S | 3 +- > arch/arm64/crypto/sha3-ce-core.S | 3 +- > arch/arm64/crypto/sha512-ce-core.S | 3 +- > arch/arm64/include/asm/alternative.h | 2 +- > arch/arm64/kernel/Makefile | 4 + > arch/arm64/kernel/entry.S | 4 +- > arch/arm64/kernel/hyp-stub.S | 3 + > arch/arm64/kernel/relocate_kernel.S | 5 + > arch/arm64/kernel/sleep.S | 5 + > arch/arm64/kvm/hyp-init.S | 3 + > arch/arm64/kvm/hyp/entry.S | 3 + > include/linux/frame.h | 19 +- > scripts/Makefile.gcc-plugins | 2 + > scripts/gcc-plugins/Kconfig | 4 + > .../arm64_switch_table_detection_plugin.c | 94 + > tools/objtool/Build | 4 +- > tools/objtool/Makefile | 13 +- > tools/objtool/arch.h | 14 +- > tools/objtool/arch/arm64/Build | 5 + > tools/objtool/arch/arm64/arch_special.c | 262 ++ > tools/objtool/arch/arm64/bit_operations.c | 69 + > tools/objtool/arch/arm64/decode.c | 2866 +++++++++++++++++ > .../objtool/arch/arm64/include/arch_special.h | 23 + > .../arch/arm64/include/bit_operations.h | 31 + > tools/objtool/arch/arm64/include/cfi_regs.h | 44 + > .../objtool/arch/arm64/include/insn_decode.h | 206 ++ > tools/objtool/arch/x86/Build | 3 + > tools/objtool/arch/x86/arch_special.c | 182 ++ > tools/objtool/arch/x86/decode.c | 29 +- > tools/objtool/arch/x86/include/arch_special.h | 28 + > tools/objtool/arch/x86/include/cfi_regs.h | 25 + > tools/objtool/{ => arch/x86}/orc_dump.c | 4 +- > tools/objtool/{ => arch/x86}/orc_gen.c | 114 +- > tools/objtool/cfi.h | 21 +- > tools/objtool/check.c | 461 +-- > tools/objtool/check.h | 13 +- > tools/objtool/elf.c | 3 +- > tools/objtool/objtool.c | 2 + > tools/objtool/orc.h | 38 +- > tools/objtool/special.c | 44 +- > tools/objtool/special.h | 13 + > 44 files changed, 4282 insertions(+), 400 deletions(-) > create mode 100644 scripts/gcc-plugins/arm64_switch_table_detection_plugin.c > create mode 100644 tools/objtool/arch/arm64/Build > create mode 100644 tools/objtool/arch/arm64/arch_special.c > create mode 100644 tools/objtool/arch/arm64/bit_operations.c > create mode 100644 tools/objtool/arch/arm64/decode.c > create mode 100644 tools/objtool/arch/arm64/include/arch_special.h > create mode 100644 tools/objtool/arch/arm64/include/bit_operations.h > create mode 100644 tools/objtool/arch/arm64/include/cfi_regs.h > create mode 100644 tools/objtool/arch/arm64/include/insn_decode.h > create mode 100644 tools/objtool/arch/x86/arch_special.c > create mode 100644 tools/objtool/arch/x86/include/arch_special.h > create mode 100644 tools/objtool/arch/x86/include/cfi_regs.h > rename tools/objtool/{ => arch/x86}/orc_dump.c (98%) > rename tools/objtool/{ => arch/x86}/orc_gen.c (62%) > > -- > 2.21.0 > Hi Julien, The 0day bot reported a couple of issues with clang with this series; the full report is available here (clang reports are only sent to our mailing lists for manual triage for the time being): https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ The first obvious issue is that this series appears to depend on a GCC plugin? I'll be quite honest, objtool and everything it does is rather over my head but I see this warning during configuration (allyesconfig): WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] Selected by [y]: - ARM64 [=y] && STACK_VALIDATION [=y] Followed by the actual error: error: unable to load plugin './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot open shared object file: No such file or directory' If this plugin is absolutely necessary and can't be implemented in another way so that clang can be used, seems like STACK_VALIDATION should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero. The second issue I see is the -Wenum-conversion warnings; they are pretty trivial to fix (see commit e7e83dd3ff1d ("objtool: Fix Clang enum conversion warning") upstream and the below diff). Would you mind addressing these in a v6 if you happen to do one? Cheers, Nathan diff --git a/tools/objtool/arch/arm64/decode.c b/tools/objtool/arch/arm64/decode.c index 5a5f82b5cb81..1ed6bf0c85ce 100644 --- a/tools/objtool/arch/arm64/decode.c +++ b/tools/objtool/arch/arm64/decode.c @@ -1518,7 +1518,7 @@ int arm_decode_ld_st_regs_unsc_imm(u32 instr, enum insn_type *type, op->dest.type = OP_DEST_REG_INDIRECT; op->dest.reg = rn; op->dest.offset = SIGN_EXTEND(imm9, 9); - op->src.type = OP_DEST_REG; + op->src.type = OP_SRC_REG; op->src.reg = rt; op->src.offset = 0; break; @@ -1605,7 +1605,7 @@ int arm_decode_ld_st_regs_unsigned(u32 instr, enum insn_type *type, op->dest.type = OP_DEST_REG_INDIRECT; op->dest.reg = rn; op->dest.offset = imm12; - op->src.type = OP_DEST_REG; + op->src.type = OP_SRC_REG; op->src.reg = rt; op->src.offset = 0; } @@ -1772,7 +1772,7 @@ int arm_decode_ld_st_imm_unpriv(u32 instr, enum insn_type *type, op->dest.type = OP_DEST_REG_INDIRECT; op->dest.reg = rn; op->dest.offset = SIGN_EXTEND(imm9, 9); - op->src.type = OP_DEST_REG; + op->src.type = OP_SRC_REG; op->src.reg = rt; op->src.offset = 0; break; @@ -1852,7 +1852,7 @@ int arm_decode_atomic(u32 instr, enum insn_type *type, list_add_tail(&op->list, ops_list); op->src.reg = rn; - op->src.type = OP_DEST_REG_INDIRECT; + op->src.type = OP_SRC_REG_INDIRECT; op->src.offset = 0; op->dest.type = OP_DEST_REG; op->dest.reg = rt; @@ -2187,7 +2187,7 @@ int arm_decode_ldapr_stlr_unsc_imm(u32 instr, enum insn_type *type, break; default: /* store */ - op->dest.type = OP_SRC_REG_INDIRECT; + op->dest.type = OP_DEST_REG_INDIRECT; op->dest.reg = rn; op->dest.offset = SIGN_EXTEND(imm9, 9); op->src.type = OP_SRC_REG;
Hi Nathan, On 1/12/20 8:42 AM, Nathan Chancellor wrote: > > Hi Julien, > > The 0day bot reported a couple of issues with clang with this series; > the full report is available here (clang reports are only sent to our > mailing lists for manual triage for the time being): > > https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ > Thanks, I'll have a look at those. > The first obvious issue is that this series appears to depend on a GCC > plugin? I'll be quite honest, objtool and everything it does is rather > over my head but I see this warning during configuration (allyesconfig): > > WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES > Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] > Selected by [y]: > - ARM64 [=y] && STACK_VALIDATION [=y] > > Followed by the actual error: > > error: unable to load plugin > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot > open shared object file: No such file or directory' > > If this plugin is absolutely necessary and can't be implemented in > another way so that clang can be used, seems like STACK_VALIDATION > should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero. > So currently the plugin is necessary for proper validation. One option can be to just let objtool output false positives on files containing jump tables when the plugin cannot be used. But overall I guess it makes more sense to disable stack validation for non-gcc builds, for now. Once people are happy with the state of things of arm64 objtool with gcc it might be worth looking at a clang plugin (although I don't know if the kernel has any support to build those at the moment). In the mean time, I'll do as you suggest and rely on CC_IS_GCC. > The second issue I see is the -Wenum-conversion warnings; they are > pretty trivial to fix (see commit e7e83dd3ff1d ("objtool: Fix Clang > enum conversion warning") upstream and the below diff). > Oh yes, these are valid warnings. I'll fix those. > Would you mind addressing these in a v6 if you happen to do one? > Yes, I'll most likely do one as I don't expect this to be a final version. Thanks for reporting those. I'll fix them in the next iteration. Cheers,
On Sun, Jan 12, 2020 at 12:43 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: > > Hi, > > > > This patch series is the continuation of Raphael's work [1]. All the > > patches can be retrieved from: > > git clone -b arm64-objtool-v5 https://github.com/julien-thierry/linux.git > > > > There still are some outstanding issues but the series is starting to > > get pretty big so it is probably good to start having some discussions > > on the current state of things. > > > > It felt necessary to split some of the patches (especially the arm64 > > decoder). In order to give Raphael credit for his work I used the > > "Suggested-by" tag. If this is not the right way to give credit or if > > it should be present on more patches do let me know. > > > > There still are some shortcomings. On defconfig here are the remaining > > warnings: > > * arch/arm64/crypto/crct10dif-ce-core.o: warning: objtool: crc_t10dif_pmull_p8()+0xf0: unsupported intra-function call > > * arch/arm64/kernel/cpu_errata.o: warning: objtool: qcom_link_stack_sanitization()+0x4: unsupported intra-function call > > Objtool currently does not support bl from a procedure to itself. This > > is also an issue with retpolines. I need to investigate more to figure > > out whether something can be done for this or if this file should not be > > validated by objtool. > > > > * arch/arm64/kernel/efi-entry.o: warning: objtool: entry()+0xb0: sibling call from callable instruction with modified stack frame > > The EFI entry jumps to code mapped by EFI. Objtool cannot know statically where the code flow is going. > > > > * arch/arm64/kernel/entry.o: warning: objtool: .entry.tramp.text+0x404: unsupported intra-function call > > Need to figure out what is needed to handle aarch64 trampolines. x86 > > explicitly annotates theirs with ANNOTATE_NOSPEC_ALTERNATIVE and > > patching them as alternatives. > > > > * arch/arm64/kernel/head.o: warning: objtool: .head.text+0x58: can't find jump dest instruction at .head.text+0x80884 > > This is actually a constant that turns out to be a valid branch opcode. > > A possible solution could be to introduce a marco that explicitly > > annotates constants placed in code sections. > > > > * arch/arm64/kernel/hibernate-asm.o: warning: objtool: el1_sync()+0x4: unsupported instruction in callable function > > Symbols el<x>_* shouldn't be considered as callable functions. Should we > > use SYM_CODE_END instead of PROC_END? > > > > * arch/arm64/kvm/hyp/hyp-entry.o: warning: objtool: .hyp.text: empty alternative at end of section > > This is due to the arm64 alternative_cb. Currently, the feature > > corresponding to the alternative_cb is defined as the current number of > > features supported by the kernel, meaning the identifier is not fixed > > accross kernel versions. This makes it a bit hard to detect these > > alternative_cb for external tools. > > > > Would it be acceptable to set a fixed identifier for alternative_cb? > > (probably 0xFF so it is always higher than the number of real features) > > > > * drivers/ata/libata-scsi.o: warning: objtool: ata_sas_queuecmd() falls through to next function ata_scsi_scan_host() > > This is due to a limitation in the switch table metadata interpretation. > > The compiler might create a table of unsigned offsets and then > > compute the final offset as follows: > > > > ldrb offset_reg, [<offset_table>, <offset_idx>, uxtw] > > adr base_reg, <base_addr> > > add res_addr, base_reg, offset_reg, sxtb #2 > > > > Effectively using the loaded offset as a signed value. > > I don't have a simple way to solve this at the moment, I'd like to > > avoid decoding the instructions to check which ones might sign extend > > the loaded offset. > > > > * kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x44: sibling call from callable instruction with modified stack frame > > This is because the function uses a C jump table which differ from > > basic jump tables. Also, the code generated for C jump tables on arm64 > > does not follow the same form as the one for x86. So the existing x86 objtool > > code handling C jump tables can't be used. > > > > I'll focus on understanding the arm64 pattern so objtool can handle them. > > > > > > In the mean time, any feedback on the current state is appreciated. > > > > * Patches 1 to 18 adapts the current objtool code to make it easier to > > support new architectures. > > * Patches 19 to 45 add the support for arm64 architecture to objtool. > > * Patches 46 to 57 fix warnings reported by objtool on the existing > > arm64 code. > > > > Changes since RFCv4[1]: > > * Rebase on v5.5-rc5 > > * Misc cleanup/bug fixes > > * Fix some new objtool warnings reported on arm64 objects > > * Make ORC subcommand optional since arm64 does not currently support it > > * Support branch instructions in alternative sections when they jump > > within the same set of alternative instructions > > * Replace the "extra" stack_op with a list of stack_op > > * Split the decoder into multiple patches to ease review > > * Mark constants generated by load literal instructions as bytes that > > should not be reached by execution flow > > * Rework the switch table handling > > > > [1] https://lkml.org/lkml/2019/8/16/400 > > > > Thanks, > > > > Julien > > > > --> > > > > Julien Thierry (43): > > objtool: check: Remove redundant checks on operand type > > objtool: check: Clean instruction state before each function > > validation > > objtool: check: Use arch specific values in restore_reg() > > objtool: check: Ignore empty alternative groups > > objtool: Give ORC functions consistent name > > objtool: Make ORC support optional > > objtool: Split generic and arch specific CFI definitions > > objtool: Abstract alternative special case handling > > objtool: check: Allow jumps from an alternative group to itself > > objtool: Do not look for STT_NOTYPE symbols > > objtool: Support addition to set frame pointer > > objtool: Support restoring BP from the stack without POP > > objtool: Make stack validation more generic > > objtool: Support multiple stack_op per instruction > > objtool: arm64: Decode unknown instructions > > objtool: arm64: Decode simple data processing instructions > > objtool: arm64: Decode add/sub immediate instructions > > objtool: arm64: Decode logical data processing instructions > > objtool: arm64: Decode system instructions not affecting the flow > > objtool: arm64: Decode calls to higher EL > > objtool: arm64: Decode brk instruction > > objtool: arm64: Decode instruction triggering context switch > > objtool: arm64: Decode branch instructions with PC relative immediates > > objtool: arm64: Decode branch to register instruction > > objtool: arm64: Decode basic load/stores > > objtool: arm64: Decode load/store with register offset > > objtool: arm64: Decode load/store register pair instructions > > objtool: arm64: Decode FP/SIMD load/store instructions > > objtool: arm64: Decode load/store exclusive > > objtool: arm64: Decode atomic load/store > > objtool: arm64: Decode pointer auth load instructions > > objtool: arm64: Decode load acquire/store release > > objtool: arm64: Decode load/store with memory tag > > objtool: arm64: Decode load literal > > objtool: arm64: Decode register data processing instructions > > objtool: arm64: Decode FP/SIMD data processing instructions > > objtool: arm64: Decode SVE instructions > > objtool: arm64: Implement functions to add switch tables alternatives > > arm64: Generate no-ops to pad executable section > > arm64: Move constant to rodata > > arm64: Mark sigreturn32.o as containing non standard code > > arm64: entry: Avoid empty alternatives entries > > arm64: crypto: Remove redundant branch > > > > Raphael Gault (14): > > objtool: Add abstraction for computation of symbols offsets > > objtool: orc: Refactor ORC API for other architectures to implement. > > objtool: Move registers and control flow to arch-dependent code > > objtool: Refactor switch-tables code to support other architectures > > objtool: arm64: Add required implementation for supporting the aarch64 > > architecture in objtool. > > gcc-plugins: objtool: Add plugin to detect switch table on arm64 > > objtool: arm64: Enable stack validation for arm64 > > arm64: alternative: Mark .altinstr_replacement as containing > > executable instructions > > arm64: assembler: Add macro to annotate asm function having non > > standard stack-frame. > > arm64: sleep: Prevent stack frame warnings from objtool > > arm64: kvm: Annotate non-standard stack frame functions > > arm64: kernel: Add exception on kuser32 to prevent stack analysis > > arm64: crypto: Add exceptions for crypto object to prevent stack > > analysis > > arm64: kernel: Annotate non-standard stack frame functions > > > > arch/arm64/Kconfig | 2 + > > arch/arm64/crypto/Makefile | 3 + > > arch/arm64/crypto/sha1-ce-core.S | 3 +- > > arch/arm64/crypto/sha2-ce-core.S | 3 +- > > arch/arm64/crypto/sha3-ce-core.S | 3 +- > > arch/arm64/crypto/sha512-ce-core.S | 3 +- > > arch/arm64/include/asm/alternative.h | 2 +- > > arch/arm64/kernel/Makefile | 4 + > > arch/arm64/kernel/entry.S | 4 +- > > arch/arm64/kernel/hyp-stub.S | 3 + > > arch/arm64/kernel/relocate_kernel.S | 5 + > > arch/arm64/kernel/sleep.S | 5 + > > arch/arm64/kvm/hyp-init.S | 3 + > > arch/arm64/kvm/hyp/entry.S | 3 + > > include/linux/frame.h | 19 +- > > scripts/Makefile.gcc-plugins | 2 + > > scripts/gcc-plugins/Kconfig | 4 + > > .../arm64_switch_table_detection_plugin.c | 94 + > > tools/objtool/Build | 4 +- > > tools/objtool/Makefile | 13 +- > > tools/objtool/arch.h | 14 +- > > tools/objtool/arch/arm64/Build | 5 + > > tools/objtool/arch/arm64/arch_special.c | 262 ++ > > tools/objtool/arch/arm64/bit_operations.c | 69 + > > tools/objtool/arch/arm64/decode.c | 2866 +++++++++++++++++ > > .../objtool/arch/arm64/include/arch_special.h | 23 + > > .../arch/arm64/include/bit_operations.h | 31 + > > tools/objtool/arch/arm64/include/cfi_regs.h | 44 + > > .../objtool/arch/arm64/include/insn_decode.h | 206 ++ > > tools/objtool/arch/x86/Build | 3 + > > tools/objtool/arch/x86/arch_special.c | 182 ++ > > tools/objtool/arch/x86/decode.c | 29 +- > > tools/objtool/arch/x86/include/arch_special.h | 28 + > > tools/objtool/arch/x86/include/cfi_regs.h | 25 + > > tools/objtool/{ => arch/x86}/orc_dump.c | 4 +- > > tools/objtool/{ => arch/x86}/orc_gen.c | 114 +- > > tools/objtool/cfi.h | 21 +- > > tools/objtool/check.c | 461 +-- > > tools/objtool/check.h | 13 +- > > tools/objtool/elf.c | 3 +- > > tools/objtool/objtool.c | 2 + > > tools/objtool/orc.h | 38 +- > > tools/objtool/special.c | 44 +- > > tools/objtool/special.h | 13 + > > 44 files changed, 4282 insertions(+), 400 deletions(-) > > create mode 100644 scripts/gcc-plugins/arm64_switch_table_detection_plugin.c > > create mode 100644 tools/objtool/arch/arm64/Build > > create mode 100644 tools/objtool/arch/arm64/arch_special.c > > create mode 100644 tools/objtool/arch/arm64/bit_operations.c > > create mode 100644 tools/objtool/arch/arm64/decode.c > > create mode 100644 tools/objtool/arch/arm64/include/arch_special.h > > create mode 100644 tools/objtool/arch/arm64/include/bit_operations.h > > create mode 100644 tools/objtool/arch/arm64/include/cfi_regs.h > > create mode 100644 tools/objtool/arch/arm64/include/insn_decode.h > > create mode 100644 tools/objtool/arch/x86/arch_special.c > > create mode 100644 tools/objtool/arch/x86/include/arch_special.h > > create mode 100644 tools/objtool/arch/x86/include/cfi_regs.h > > rename tools/objtool/{ => arch/x86}/orc_dump.c (98%) > > rename tools/objtool/{ => arch/x86}/orc_gen.c (62%) > > > > -- > > 2.21.0 > > > > Hi Julien, > > The 0day bot reported a couple of issues with clang with this series; > the full report is available here (clang reports are only sent to our > mailing lists for manual triage for the time being): > > https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ > > The first obvious issue is that this series appears to depend on a GCC > plugin? I'll be quite honest, objtool and everything it does is rather > over my head but I see this warning during configuration (allyesconfig): > > WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES > Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] > Selected by [y]: > - ARM64 [=y] && STACK_VALIDATION [=y] > > Followed by the actual error: > > error: unable to load plugin > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot > open shared object file: No such file or directory' > > If this plugin is absolutely necessary and can't be implemented in > another way so that clang can be used, seems like STACK_VALIDATION > should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero. Ah, cool. I look forward to having objtool check additional architectures. It's found legitimate codegen bugs in Clang before. We should make sure this series doesn't regress Clang builds. Can you please help ensure it doesn't? $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make CC=clang We're happy to help take a look at anything that looks suspicious, but some code sequences may be quite different than GCC. We can't be adding hard dependencies on GCC plugins. > > The second issue I see is the -Wenum-conversion warnings; they are > pretty trivial to fix (see commit e7e83dd3ff1d ("objtool: Fix Clang > enum conversion warning") upstream and the below diff). > > Would you mind addressing these in a v6 if you happen to do one? > > Cheers, > Nathan > > diff --git a/tools/objtool/arch/arm64/decode.c b/tools/objtool/arch/arm64/decode.c > index 5a5f82b5cb81..1ed6bf0c85ce 100644 > --- a/tools/objtool/arch/arm64/decode.c > +++ b/tools/objtool/arch/arm64/decode.c > @@ -1518,7 +1518,7 @@ int arm_decode_ld_st_regs_unsc_imm(u32 instr, enum insn_type *type, > op->dest.type = OP_DEST_REG_INDIRECT; > op->dest.reg = rn; > op->dest.offset = SIGN_EXTEND(imm9, 9); > - op->src.type = OP_DEST_REG; > + op->src.type = OP_SRC_REG; > op->src.reg = rt; > op->src.offset = 0; > break; > @@ -1605,7 +1605,7 @@ int arm_decode_ld_st_regs_unsigned(u32 instr, enum insn_type *type, > op->dest.type = OP_DEST_REG_INDIRECT; > op->dest.reg = rn; > op->dest.offset = imm12; > - op->src.type = OP_DEST_REG; > + op->src.type = OP_SRC_REG; > op->src.reg = rt; > op->src.offset = 0; > } > @@ -1772,7 +1772,7 @@ int arm_decode_ld_st_imm_unpriv(u32 instr, enum insn_type *type, > op->dest.type = OP_DEST_REG_INDIRECT; > op->dest.reg = rn; > op->dest.offset = SIGN_EXTEND(imm9, 9); > - op->src.type = OP_DEST_REG; > + op->src.type = OP_SRC_REG; > op->src.reg = rt; > op->src.offset = 0; > break; > @@ -1852,7 +1852,7 @@ int arm_decode_atomic(u32 instr, enum insn_type *type, > list_add_tail(&op->list, ops_list); > > op->src.reg = rn; > - op->src.type = OP_DEST_REG_INDIRECT; > + op->src.type = OP_SRC_REG_INDIRECT; > op->src.offset = 0; > op->dest.type = OP_DEST_REG; > op->dest.reg = rt; > @@ -2187,7 +2187,7 @@ int arm_decode_ldapr_stlr_unsc_imm(u32 instr, enum insn_type *type, > break; > default: > /* store */ > - op->dest.type = OP_SRC_REG_INDIRECT; > + op->dest.type = OP_DEST_REG_INDIRECT; > op->dest.reg = rn; > op->dest.offset = SIGN_EXTEND(imm9, 9); > op->src.type = OP_SRC_REG; > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200112084258.GA44004%40ubuntu-x2-xlarge-x86.
On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: > In the mean time, any feedback on the current state is appreciated. > > * Patches 1 to 18 adapts the current objtool code to make it easier to > support new architectures. In the interrest of moving things along; I've looked through these and 1-14,16 look good to me, 17,18 hurt my brain. Josh, what say you?
On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: > This patch series is the continuation of Raphael's work [1]. All the > patches can be retrieved from: > git clone -b arm64-objtool-v5 https://github.com/julien-thierry/linux.git [...] > objtool: arm64: Decode unknown instructions > objtool: arm64: Decode simple data processing instructions > objtool: arm64: Decode add/sub immediate instructions > objtool: arm64: Decode logical data processing instructions > objtool: arm64: Decode system instructions not affecting the flow > objtool: arm64: Decode calls to higher EL > objtool: arm64: Decode brk instruction > objtool: arm64: Decode instruction triggering context switch > objtool: arm64: Decode branch instructions with PC relative immediates > objtool: arm64: Decode branch to register instruction > objtool: arm64: Decode basic load/stores > objtool: arm64: Decode load/store with register offset > objtool: arm64: Decode load/store register pair instructions > objtool: arm64: Decode FP/SIMD load/store instructions > objtool: arm64: Decode load/store exclusive > objtool: arm64: Decode atomic load/store > objtool: arm64: Decode pointer auth load instructions > objtool: arm64: Decode load acquire/store release > objtool: arm64: Decode load/store with memory tag > objtool: arm64: Decode load literal > objtool: arm64: Decode register data processing instructions > objtool: arm64: Decode FP/SIMD data processing instructions > objtool: arm64: Decode SVE instructions That's a lot of decoding logic which we already have in arch/arm64/{kernel/insn.c,include/asm/insn.h}. I'd prefer to see this stuff reused or generated from a single source, since it's really easy to get it wrong, has a tendency to bitrot and is nasty to debug. Will
On Mon, Jan 13, 2020 at 07:57:48AM +0000, Julien Thierry wrote: > On 1/12/20 8:42 AM, Nathan Chancellor wrote: > > The 0day bot reported a couple of issues with clang with this series; > > the full report is available here (clang reports are only sent to our > > mailing lists for manual triage for the time being): > > > > https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ > > > > Thanks, I'll have a look at those. > > > The first obvious issue is that this series appears to depend on a GCC > > plugin? I'll be quite honest, objtool and everything it does is rather > > over my head but I see this warning during configuration (allyesconfig): > > > > WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES > > Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] > > Selected by [y]: > > - ARM64 [=y] && STACK_VALIDATION [=y] > > > > Followed by the actual error: > > > > error: unable to load plugin > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot > > open shared object file: No such file or directory' > > > > If this plugin is absolutely necessary and can't be implemented in > > another way so that clang can be used, seems like STACK_VALIDATION > > should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero. > > > > So currently the plugin is necessary for proper validation. One option can > be to just let objtool output false positives on files containing jump > tables when the plugin cannot be used. But overall I guess it makes more > sense to disable stack validation for non-gcc builds, for now. Alternatively, could we add '-fno-jump-tables' to the KBUILD_CFLAGS if STACK_VALIDATION is selected but we're not using GCC? Is that sufficient to prevent generation of these things? Will
On Tue, Jan 21, 2020 at 2:31 AM Will Deacon <will@kernel.org> wrote: > > On Mon, Jan 13, 2020 at 07:57:48AM +0000, Julien Thierry wrote: > > On 1/12/20 8:42 AM, Nathan Chancellor wrote: > > > The 0day bot reported a couple of issues with clang with this series; > > > the full report is available here (clang reports are only sent to our > > > mailing lists for manual triage for the time being): > > > > > > https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ > > > > > > > Thanks, I'll have a look at those. > > > > > The first obvious issue is that this series appears to depend on a GCC > > > plugin? I'll be quite honest, objtool and everything it does is rather > > > over my head but I see this warning during configuration (allyesconfig): > > > > > > WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES > > > Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] > > > Selected by [y]: > > > - ARM64 [=y] && STACK_VALIDATION [=y] > > > > > > Followed by the actual error: > > > > > > error: unable to load plugin > > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': > > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot > > > open shared object file: No such file or directory' > > > > > > If this plugin is absolutely necessary and can't be implemented in > > > another way so that clang can be used, seems like STACK_VALIDATION > > > should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero. > > > > > > > So currently the plugin is necessary for proper validation. One option can > > be to just let objtool output false positives on files containing jump > > tables when the plugin cannot be used. But overall I guess it makes more > > sense to disable stack validation for non-gcc builds, for now. > > Alternatively, could we add '-fno-jump-tables' to the KBUILD_CFLAGS if > STACK_VALIDATION is selected but we're not using GCC? Is that sufficient > to prevent generation of these things? Surely we wouldn't want to replace jump tables with long chains of comparisons just because objtool couldn't validate jump tables without a GCC plugin for aarch64 for some reason, right? objtool validation is valuable, but tying runtime performance to a GCC plugin used for validation seems bad.
On Mon, Jan 20, 2020 at 04:07:11PM +0100, Peter Zijlstra wrote: > On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: > > In the mean time, any feedback on the current state is appreciated. > > > > * Patches 1 to 18 adapts the current objtool code to make it easier to > > support new architectures. > > In the interrest of moving things along; I've looked through these > and 1-14,16 look good to me, 17,18 hurt my brain. > > Josh, what say you? Agreed. Julien, thanks a lot for splitting these up nicely. If you post 1-14 (updated based on the recent comments), we can look at merging those sooner. 15-18 also hurt my brain -- probably a symptom of the existing fragile mess -- so I'll need to spend more time staring at them.
On Tue, Jan 21, 2020 at 09:08:29AM -0800, Nick Desaulniers wrote: > On Tue, Jan 21, 2020 at 2:31 AM Will Deacon <will@kernel.org> wrote: > > > > On Mon, Jan 13, 2020 at 07:57:48AM +0000, Julien Thierry wrote: > > > On 1/12/20 8:42 AM, Nathan Chancellor wrote: > > > > The 0day bot reported a couple of issues with clang with this series; > > > > the full report is available here (clang reports are only sent to our > > > > mailing lists for manual triage for the time being): > > > > > > > > https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ > > > > > > > > > > Thanks, I'll have a look at those. > > > > > > > The first obvious issue is that this series appears to depend on a GCC > > > > plugin? I'll be quite honest, objtool and everything it does is rather > > > > over my head but I see this warning during configuration (allyesconfig): > > > > > > > > WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES > > > > Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] > > > > Selected by [y]: > > > > - ARM64 [=y] && STACK_VALIDATION [=y] > > > > > > > > Followed by the actual error: > > > > > > > > error: unable to load plugin > > > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': > > > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot > > > > open shared object file: No such file or directory' > > > > > > > > If this plugin is absolutely necessary and can't be implemented in > > > > another way so that clang can be used, seems like STACK_VALIDATION > > > > should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero. > > > > > > > > > > So currently the plugin is necessary for proper validation. One option can > > > be to just let objtool output false positives on files containing jump > > > tables when the plugin cannot be used. But overall I guess it makes more > > > sense to disable stack validation for non-gcc builds, for now. > > > > Alternatively, could we add '-fno-jump-tables' to the KBUILD_CFLAGS if > > STACK_VALIDATION is selected but we're not using GCC? Is that sufficient > > to prevent generation of these things? > > Surely we wouldn't want to replace jump tables with long chains of > comparisons just because objtool couldn't validate jump tables without > a GCC plugin for aarch64 for some reason, right? objtool validation > is valuable, but tying runtime performance to a GCC plugin used for > validation seems bad. I'm only suggesting it if STACK_VALIDATION is selected. It's off by default, and lives in Kconfig.debug. I'd prefer that to "cross your fingers are do nothing differently", which is what the other option seems to be. Will
On Tue, Jan 21, 2020 at 06:06:34PM +0000, Will Deacon wrote: > On Tue, Jan 21, 2020 at 09:08:29AM -0800, Nick Desaulniers wrote: > > On Tue, Jan 21, 2020 at 2:31 AM Will Deacon <will@kernel.org> wrote: > > > > > > On Mon, Jan 13, 2020 at 07:57:48AM +0000, Julien Thierry wrote: > > > > On 1/12/20 8:42 AM, Nathan Chancellor wrote: > > > > > The 0day bot reported a couple of issues with clang with this series; > > > > > the full report is available here (clang reports are only sent to our > > > > > mailing lists for manual triage for the time being): > > > > > > > > > > https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ > > > > > > > > > > > > > Thanks, I'll have a look at those. > > > > > > > > > The first obvious issue is that this series appears to depend on a GCC > > > > > plugin? I'll be quite honest, objtool and everything it does is rather > > > > > over my head but I see this warning during configuration (allyesconfig): > > > > > > > > > > WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES > > > > > Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] > > > > > Selected by [y]: > > > > > - ARM64 [=y] && STACK_VALIDATION [=y] > > > > > > > > > > Followed by the actual error: > > > > > > > > > > error: unable to load plugin > > > > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': > > > > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot > > > > > open shared object file: No such file or directory' > > > > > > > > > > If this plugin is absolutely necessary and can't be implemented in > > > > > another way so that clang can be used, seems like STACK_VALIDATION > > > > > should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero. > > > > > > > > > > > > > So currently the plugin is necessary for proper validation. One option can > > > > be to just let objtool output false positives on files containing jump > > > > tables when the plugin cannot be used. But overall I guess it makes more > > > > sense to disable stack validation for non-gcc builds, for now. > > > > > > Alternatively, could we add '-fno-jump-tables' to the KBUILD_CFLAGS if > > > STACK_VALIDATION is selected but we're not using GCC? Is that sufficient > > > to prevent generation of these things? > > > > Surely we wouldn't want to replace jump tables with long chains of > > comparisons just because objtool couldn't validate jump tables without > > a GCC plugin for aarch64 for some reason, right? objtool validation > > is valuable, but tying runtime performance to a GCC plugin used for > > validation seems bad. > > I'm only suggesting it if STACK_VALIDATION is selected. It's off by default, > and lives in Kconfig.debug. I'd prefer that to "cross your fingers are do > nothing differently", which is what the other option seems to be. I don't know what the right answer is here, but keep in mind that objtool is on by default for x86, so don't be surprised if that eventually happens to arch64 too. Short term it might be ok to disable jump tables with objtool enabled, or to disable objtool when clang is in use, but long term we'll need to figure out a better solution.
On Tue, Jan 21, 2020 at 12:30:09PM -0600, Josh Poimboeuf wrote: > On Tue, Jan 21, 2020 at 06:06:34PM +0000, Will Deacon wrote: > > On Tue, Jan 21, 2020 at 09:08:29AM -0800, Nick Desaulniers wrote: > > > On Tue, Jan 21, 2020 at 2:31 AM Will Deacon <will@kernel.org> wrote: > > > > > > > > On Mon, Jan 13, 2020 at 07:57:48AM +0000, Julien Thierry wrote: > > > > > On 1/12/20 8:42 AM, Nathan Chancellor wrote: > > > > > > The 0day bot reported a couple of issues with clang with this series; > > > > > > the full report is available here (clang reports are only sent to our > > > > > > mailing lists for manual triage for the time being): > > > > > > > > > > > > https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ > > > > > > > > > > > > > > > > Thanks, I'll have a look at those. > > > > > > > > > > > The first obvious issue is that this series appears to depend on a GCC > > > > > > plugin? I'll be quite honest, objtool and everything it does is rather > > > > > > over my head but I see this warning during configuration (allyesconfig): > > > > > > > > > > > > WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES > > > > > > Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] > > > > > > Selected by [y]: > > > > > > - ARM64 [=y] && STACK_VALIDATION [=y] > > > > > > > > > > > > Followed by the actual error: > > > > > > > > > > > > error: unable to load plugin > > > > > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': > > > > > > './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot > > > > > > open shared object file: No such file or directory' > > > > > > > > > > > > If this plugin is absolutely necessary and can't be implemented in > > > > > > another way so that clang can be used, seems like STACK_VALIDATION > > > > > > should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero. > > > > > > > > > > > > > > > > So currently the plugin is necessary for proper validation. One option can > > > > > be to just let objtool output false positives on files containing jump > > > > > tables when the plugin cannot be used. But overall I guess it makes more > > > > > sense to disable stack validation for non-gcc builds, for now. > > > > > > > > Alternatively, could we add '-fno-jump-tables' to the KBUILD_CFLAGS if > > > > STACK_VALIDATION is selected but we're not using GCC? Is that sufficient > > > > to prevent generation of these things? > > > > > > Surely we wouldn't want to replace jump tables with long chains of > > > comparisons just because objtool couldn't validate jump tables without > > > a GCC plugin for aarch64 for some reason, right? objtool validation > > > is valuable, but tying runtime performance to a GCC plugin used for > > > validation seems bad. > > > > I'm only suggesting it if STACK_VALIDATION is selected. It's off by default, > > and lives in Kconfig.debug. I'd prefer that to "cross your fingers are do > > nothing differently", which is what the other option seems to be. > > I don't know what the right answer is here, but keep in mind that > objtool is on by default for x86, so don't be surprised if that > eventually happens to arch64 too. > > Short term it might be ok to disable jump tables with objtool enabled, > or to disable objtool when clang is in use, but long term we'll need to > figure out a better solution. Oh, absolutely. No objection from me fixing this properly in the long term. I just don't want to be in a situation where STACK_VALIDATION is silently ignored in the meantime. Will
On 1/21/20 10:30 AM, Will Deacon wrote: > On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: >> This patch series is the continuation of Raphael's work [1]. All the >> patches can be retrieved from: >> git clone -b arm64-objtool-v5 https://github.com/julien-thierry/linux.git > > [...] > >> objtool: arm64: Decode unknown instructions >> objtool: arm64: Decode simple data processing instructions >> objtool: arm64: Decode add/sub immediate instructions >> objtool: arm64: Decode logical data processing instructions >> objtool: arm64: Decode system instructions not affecting the flow >> objtool: arm64: Decode calls to higher EL >> objtool: arm64: Decode brk instruction >> objtool: arm64: Decode instruction triggering context switch >> objtool: arm64: Decode branch instructions with PC relative immediates >> objtool: arm64: Decode branch to register instruction >> objtool: arm64: Decode basic load/stores >> objtool: arm64: Decode load/store with register offset >> objtool: arm64: Decode load/store register pair instructions >> objtool: arm64: Decode FP/SIMD load/store instructions >> objtool: arm64: Decode load/store exclusive >> objtool: arm64: Decode atomic load/store >> objtool: arm64: Decode pointer auth load instructions >> objtool: arm64: Decode load acquire/store release >> objtool: arm64: Decode load/store with memory tag >> objtool: arm64: Decode load literal >> objtool: arm64: Decode register data processing instructions >> objtool: arm64: Decode FP/SIMD data processing instructions >> objtool: arm64: Decode SVE instructions > > That's a lot of decoding logic which we already have in > arch/arm64/{kernel/insn.c,include/asm/insn.h}. I'd prefer to see this stuff > reused or generated from a single source, since it's really easy to get it > wrong, has a tendency to bitrot and is nasty to debug. > The thing is that the code in those files is mostly encoding logic (motivated by BPF) rather than decoding (except for the instruction that might be trapped, but these rarely overlap with instructions that objtools cares about). I agree that ideally the decoding/encoding should be under arch/arm64/lib, I was just a bit weary introducing a lot of decoding code under arch/arm64 that wouldn't even be used in kernel code. But I can make an attempt for the encode/decode lib and post it as part of the next version. Cheers,
On 1/21/20 5:50 PM, Josh Poimboeuf wrote: > On Mon, Jan 20, 2020 at 04:07:11PM +0100, Peter Zijlstra wrote: >> On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: >>> In the mean time, any feedback on the current state is appreciated. >>> >>> * Patches 1 to 18 adapts the current objtool code to make it easier to >>> support new architectures. >> >> In the interrest of moving things along; I've looked through these >> and 1-14,16 look good to me, 17,18 hurt my brain. >> >> Josh, what say you? > > Agreed. > > Julien, thanks a lot for splitting these up nicely. If you post 1-14 > (updated based on the recent comments), we can look at merging those > sooner. > Sure, I'll repost the refactoring patches separately once I've updated them. > 15-18 also hurt my brain -- probably a symptom of the existing fragile > mess -- so I'll need to spend more time staring at them. > Yes, the whole state update code hurt quite a lot as well. It took me a while to convince myself that my changes felt correct (to me at least, it might be that I got things wrong :) ). Thanks,
On Thu, Jan 23, 2020 at 01:52:17PM +0000, Julien Thierry wrote: > > > On 1/21/20 10:30 AM, Will Deacon wrote: > > On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: > > > This patch series is the continuation of Raphael's work [1]. All the > > > patches can be retrieved from: > > > git clone -b arm64-objtool-v5 https://github.com/julien-thierry/linux.git > > > > [...] > > > > > objtool: arm64: Decode unknown instructions > > > objtool: arm64: Decode simple data processing instructions > > > objtool: arm64: Decode add/sub immediate instructions > > > objtool: arm64: Decode logical data processing instructions > > > objtool: arm64: Decode system instructions not affecting the flow > > > objtool: arm64: Decode calls to higher EL > > > objtool: arm64: Decode brk instruction > > > objtool: arm64: Decode instruction triggering context switch > > > objtool: arm64: Decode branch instructions with PC relative immediates > > > objtool: arm64: Decode branch to register instruction > > > objtool: arm64: Decode basic load/stores > > > objtool: arm64: Decode load/store with register offset > > > objtool: arm64: Decode load/store register pair instructions > > > objtool: arm64: Decode FP/SIMD load/store instructions > > > objtool: arm64: Decode load/store exclusive > > > objtool: arm64: Decode atomic load/store > > > objtool: arm64: Decode pointer auth load instructions > > > objtool: arm64: Decode load acquire/store release > > > objtool: arm64: Decode load/store with memory tag > > > objtool: arm64: Decode load literal > > > objtool: arm64: Decode register data processing instructions > > > objtool: arm64: Decode FP/SIMD data processing instructions > > > objtool: arm64: Decode SVE instructions > > > > That's a lot of decoding logic which we already have in > > arch/arm64/{kernel/insn.c,include/asm/insn.h}. I'd prefer to see this stuff > > reused or generated from a single source, since it's really easy to get it > > wrong, has a tendency to bitrot and is nasty to debug. > > > > The thing is that the code in those files is mostly encoding logic > (motivated by BPF) rather than decoding (except for the instruction that > might be trapped, but these rarely overlap with instructions that objtools > cares about). I agree that ideally the decoding/encoding should be under > arch/arm64/lib, I was just a bit weary introducing a lot of decoding code > under arch/arm64 that wouldn't even be used in kernel code. Hmm, but kprobes decodes instructions somehow :p Not saying you have to refactor everything, but I'd hope you could reuse some of the aarch64_insn_is* and aarch64_insn_extract* functions at least. Will
On 1/23/20 2:35 PM, Will Deacon wrote: > On Thu, Jan 23, 2020 at 01:52:17PM +0000, Julien Thierry wrote: >> >> >> On 1/21/20 10:30 AM, Will Deacon wrote: >>> On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: >>>> This patch series is the continuation of Raphael's work [1]. All the >>>> patches can be retrieved from: >>>> git clone -b arm64-objtool-v5 https://github.com/julien-thierry/linux.git >>> >>> [...] >>> >>>> objtool: arm64: Decode unknown instructions >>>> objtool: arm64: Decode simple data processing instructions >>>> objtool: arm64: Decode add/sub immediate instructions >>>> objtool: arm64: Decode logical data processing instructions >>>> objtool: arm64: Decode system instructions not affecting the flow >>>> objtool: arm64: Decode calls to higher EL >>>> objtool: arm64: Decode brk instruction >>>> objtool: arm64: Decode instruction triggering context switch >>>> objtool: arm64: Decode branch instructions with PC relative immediates >>>> objtool: arm64: Decode branch to register instruction >>>> objtool: arm64: Decode basic load/stores >>>> objtool: arm64: Decode load/store with register offset >>>> objtool: arm64: Decode load/store register pair instructions >>>> objtool: arm64: Decode FP/SIMD load/store instructions >>>> objtool: arm64: Decode load/store exclusive >>>> objtool: arm64: Decode atomic load/store >>>> objtool: arm64: Decode pointer auth load instructions >>>> objtool: arm64: Decode load acquire/store release >>>> objtool: arm64: Decode load/store with memory tag >>>> objtool: arm64: Decode load literal >>>> objtool: arm64: Decode register data processing instructions >>>> objtool: arm64: Decode FP/SIMD data processing instructions >>>> objtool: arm64: Decode SVE instructions >>> >>> That's a lot of decoding logic which we already have in >>> arch/arm64/{kernel/insn.c,include/asm/insn.h}. I'd prefer to see this stuff >>> reused or generated from a single source, since it's really easy to get it >>> wrong, has a tendency to bitrot and is nasty to debug. >>> >> >> The thing is that the code in those files is mostly encoding logic >> (motivated by BPF) rather than decoding (except for the instruction that >> might be trapped, but these rarely overlap with instructions that objtools >> cares about). I agree that ideally the decoding/encoding should be under >> arch/arm64/lib, I was just a bit weary introducing a lot of decoding code >> under arch/arm64 that wouldn't even be used in kernel code. > > Hmm, but kprobes decodes instructions somehow :p > > Not saying you have to refactor everything, but I'd hope you could reuse > some of the aarch64_insn_is* and aarch64_insn_extract* functions at least. > Oh, you're right, there seem to be more than what I remembered. There is probably a bunch of things I can reuse (and I'll feel more at ease with that rather than introducing a whole bunch of new code :D ). Thanks,
Hi, This patch series is the continuation of Raphael's work [1]. All the patches can be retrieved from: git clone -b arm64-objtool-v5 https://github.com/julien-thierry/linux.git There still are some outstanding issues but the series is starting to get pretty big so it is probably good to start having some discussions on the current state of things. It felt necessary to split some of the patches (especially the arm64 decoder). In order to give Raphael credit for his work I used the "Suggested-by" tag. If this is not the right way to give credit or if it should be present on more patches do let me know. There still are some shortcomings. On defconfig here are the remaining warnings: * arch/arm64/crypto/crct10dif-ce-core.o: warning: objtool: crc_t10dif_pmull_p8()+0xf0: unsupported intra-function call * arch/arm64/kernel/cpu_errata.o: warning: objtool: qcom_link_stack_sanitization()+0x4: unsupported intra-function call Objtool currently does not support bl from a procedure to itself. This is also an issue with retpolines. I need to investigate more to figure out whether something can be done for this or if this file should not be validated by objtool. * arch/arm64/kernel/efi-entry.o: warning: objtool: entry()+0xb0: sibling call from callable instruction with modified stack frame The EFI entry jumps to code mapped by EFI. Objtool cannot know statically where the code flow is going. * arch/arm64/kernel/entry.o: warning: objtool: .entry.tramp.text+0x404: unsupported intra-function call Need to figure out what is needed to handle aarch64 trampolines. x86 explicitly annotates theirs with ANNOTATE_NOSPEC_ALTERNATIVE and patching them as alternatives. * arch/arm64/kernel/head.o: warning: objtool: .head.text+0x58: can't find jump dest instruction at .head.text+0x80884 This is actually a constant that turns out to be a valid branch opcode. A possible solution could be to introduce a marco that explicitly annotates constants placed in code sections. * arch/arm64/kernel/hibernate-asm.o: warning: objtool: el1_sync()+0x4: unsupported instruction in callable function Symbols el<x>_* shouldn't be considered as callable functions. Should we use SYM_CODE_END instead of PROC_END? * arch/arm64/kvm/hyp/hyp-entry.o: warning: objtool: .hyp.text: empty alternative at end of section This is due to the arm64 alternative_cb. Currently, the feature corresponding to the alternative_cb is defined as the current number of features supported by the kernel, meaning the identifier is not fixed accross kernel versions. This makes it a bit hard to detect these alternative_cb for external tools. Would it be acceptable to set a fixed identifier for alternative_cb? (probably 0xFF so it is always higher than the number of real features) * drivers/ata/libata-scsi.o: warning: objtool: ata_sas_queuecmd() falls through to next function ata_scsi_scan_host() This is due to a limitation in the switch table metadata interpretation. The compiler might create a table of unsigned offsets and then compute the final offset as follows: ldrb offset_reg, [<offset_table>, <offset_idx>, uxtw] adr base_reg, <base_addr> add res_addr, base_reg, offset_reg, sxtb #2 Effectively using the loaded offset as a signed value. I don't have a simple way to solve this at the moment, I'd like to avoid decoding the instructions to check which ones might sign extend the loaded offset. * kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x44: sibling call from callable instruction with modified stack frame This is because the function uses a C jump table which differ from basic jump tables. Also, the code generated for C jump tables on arm64 does not follow the same form as the one for x86. So the existing x86 objtool code handling C jump tables can't be used. I'll focus on understanding the arm64 pattern so objtool can handle them. In the mean time, any feedback on the current state is appreciated. * Patches 1 to 18 adapts the current objtool code to make it easier to support new architectures. * Patches 19 to 45 add the support for arm64 architecture to objtool. * Patches 46 to 57 fix warnings reported by objtool on the existing arm64 code. Changes since RFCv4[1]: * Rebase on v5.5-rc5 * Misc cleanup/bug fixes * Fix some new objtool warnings reported on arm64 objects * Make ORC subcommand optional since arm64 does not currently support it * Support branch instructions in alternative sections when they jump within the same set of alternative instructions * Replace the "extra" stack_op with a list of stack_op * Split the decoder into multiple patches to ease review * Mark constants generated by load literal instructions as bytes that should not be reached by execution flow * Rework the switch table handling [1] https://lkml.org/lkml/2019/8/16/400 Thanks, Julien --> Julien Thierry (43): objtool: check: Remove redundant checks on operand type objtool: check: Clean instruction state before each function validation objtool: check: Use arch specific values in restore_reg() objtool: check: Ignore empty alternative groups objtool: Give ORC functions consistent name objtool: Make ORC support optional objtool: Split generic and arch specific CFI definitions objtool: Abstract alternative special case handling objtool: check: Allow jumps from an alternative group to itself objtool: Do not look for STT_NOTYPE symbols objtool: Support addition to set frame pointer objtool: Support restoring BP from the stack without POP objtool: Make stack validation more generic objtool: Support multiple stack_op per instruction objtool: arm64: Decode unknown instructions objtool: arm64: Decode simple data processing instructions objtool: arm64: Decode add/sub immediate instructions objtool: arm64: Decode logical data processing instructions objtool: arm64: Decode system instructions not affecting the flow objtool: arm64: Decode calls to higher EL objtool: arm64: Decode brk instruction objtool: arm64: Decode instruction triggering context switch objtool: arm64: Decode branch instructions with PC relative immediates objtool: arm64: Decode branch to register instruction objtool: arm64: Decode basic load/stores objtool: arm64: Decode load/store with register offset objtool: arm64: Decode load/store register pair instructions objtool: arm64: Decode FP/SIMD load/store instructions objtool: arm64: Decode load/store exclusive objtool: arm64: Decode atomic load/store objtool: arm64: Decode pointer auth load instructions objtool: arm64: Decode load acquire/store release objtool: arm64: Decode load/store with memory tag objtool: arm64: Decode load literal objtool: arm64: Decode register data processing instructions objtool: arm64: Decode FP/SIMD data processing instructions objtool: arm64: Decode SVE instructions objtool: arm64: Implement functions to add switch tables alternatives arm64: Generate no-ops to pad executable section arm64: Move constant to rodata arm64: Mark sigreturn32.o as containing non standard code arm64: entry: Avoid empty alternatives entries arm64: crypto: Remove redundant branch Raphael Gault (14): objtool: Add abstraction for computation of symbols offsets objtool: orc: Refactor ORC API for other architectures to implement. objtool: Move registers and control flow to arch-dependent code objtool: Refactor switch-tables code to support other architectures objtool: arm64: Add required implementation for supporting the aarch64 architecture in objtool. gcc-plugins: objtool: Add plugin to detect switch table on arm64 objtool: arm64: Enable stack validation for arm64 arm64: alternative: Mark .altinstr_replacement as containing executable instructions arm64: assembler: Add macro to annotate asm function having non standard stack-frame. arm64: sleep: Prevent stack frame warnings from objtool arm64: kvm: Annotate non-standard stack frame functions arm64: kernel: Add exception on kuser32 to prevent stack analysis arm64: crypto: Add exceptions for crypto object to prevent stack analysis arm64: kernel: Annotate non-standard stack frame functions arch/arm64/Kconfig | 2 + arch/arm64/crypto/Makefile | 3 + arch/arm64/crypto/sha1-ce-core.S | 3 +- arch/arm64/crypto/sha2-ce-core.S | 3 +- arch/arm64/crypto/sha3-ce-core.S | 3 +- arch/arm64/crypto/sha512-ce-core.S | 3 +- arch/arm64/include/asm/alternative.h | 2 +- arch/arm64/kernel/Makefile | 4 + arch/arm64/kernel/entry.S | 4 +- arch/arm64/kernel/hyp-stub.S | 3 + arch/arm64/kernel/relocate_kernel.S | 5 + arch/arm64/kernel/sleep.S | 5 + arch/arm64/kvm/hyp-init.S | 3 + arch/arm64/kvm/hyp/entry.S | 3 + include/linux/frame.h | 19 +- scripts/Makefile.gcc-plugins | 2 + scripts/gcc-plugins/Kconfig | 4 + .../arm64_switch_table_detection_plugin.c | 94 + tools/objtool/Build | 4 +- tools/objtool/Makefile | 13 +- tools/objtool/arch.h | 14 +- tools/objtool/arch/arm64/Build | 5 + tools/objtool/arch/arm64/arch_special.c | 262 ++ tools/objtool/arch/arm64/bit_operations.c | 69 + tools/objtool/arch/arm64/decode.c | 2866 +++++++++++++++++ .../objtool/arch/arm64/include/arch_special.h | 23 + .../arch/arm64/include/bit_operations.h | 31 + tools/objtool/arch/arm64/include/cfi_regs.h | 44 + .../objtool/arch/arm64/include/insn_decode.h | 206 ++ tools/objtool/arch/x86/Build | 3 + tools/objtool/arch/x86/arch_special.c | 182 ++ tools/objtool/arch/x86/decode.c | 29 +- tools/objtool/arch/x86/include/arch_special.h | 28 + tools/objtool/arch/x86/include/cfi_regs.h | 25 + tools/objtool/{ => arch/x86}/orc_dump.c | 4 +- tools/objtool/{ => arch/x86}/orc_gen.c | 114 +- tools/objtool/cfi.h | 21 +- tools/objtool/check.c | 461 +-- tools/objtool/check.h | 13 +- tools/objtool/elf.c | 3 +- tools/objtool/objtool.c | 2 + tools/objtool/orc.h | 38 +- tools/objtool/special.c | 44 +- tools/objtool/special.h | 13 + 44 files changed, 4282 insertions(+), 400 deletions(-) create mode 100644 scripts/gcc-plugins/arm64_switch_table_detection_plugin.c create mode 100644 tools/objtool/arch/arm64/Build create mode 100644 tools/objtool/arch/arm64/arch_special.c create mode 100644 tools/objtool/arch/arm64/bit_operations.c create mode 100644 tools/objtool/arch/arm64/decode.c create mode 100644 tools/objtool/arch/arm64/include/arch_special.h create mode 100644 tools/objtool/arch/arm64/include/bit_operations.h create mode 100644 tools/objtool/arch/arm64/include/cfi_regs.h create mode 100644 tools/objtool/arch/arm64/include/insn_decode.h create mode 100644 tools/objtool/arch/x86/arch_special.c create mode 100644 tools/objtool/arch/x86/include/arch_special.h create mode 100644 tools/objtool/arch/x86/include/cfi_regs.h rename tools/objtool/{ => arch/x86}/orc_dump.c (98%) rename tools/objtool/{ => arch/x86}/orc_gen.c (62%) -- 2.21.0