| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 12 Jan 2020 01:42:58 -0700
From: Nathan Chancellor <natechancellor@...il.com>
To: Julien Thierry <jthierry@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
jpoimboe@...hat.com, peterz@...radead.org, raphael.gault@....com,
catalin.marinas@....com, will@...nel.org,
clang-built-linux@...glegroups.com
Subject: Re: [RFC v5 00/57] objtool: Add support for arm64
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;
Powered by blists - more mailing lists