Re: [PATCH v3 1/4] objtool: Handle frame pointer related instructions
From: Jinyang He
Date: Sun Sep 08 2024 - 21:10:39 EST
On 2024-08-07 16:59, Tiezhu Yang wrote:
After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
support"), there are three new instructions "addi.d $fp, $sp, 32",
"sub.d $sp, $sp, $t0" and "addi.d $sp, $fp, -32" for the secondary
stack in do_syscall(), then there is a objtool warning "return with
modified stack frame" and no handle_syscall() which is the previous
frame of do_syscall() in the call trace when executing the command
"echo l > /proc/sysrq-trigger".
objdump shows something like this:
0000000000000000 <do_syscall>:
0: 02ff8063 addi.d $sp, $sp, -32
4: 29c04076 st.d $fp, $sp, 16
8: 29c02077 st.d $s0, $sp, 8
c: 29c06061 st.d $ra, $sp, 24
10: 02c08076 addi.d $fp, $sp, 32
...
74: 0011b063 sub.d $sp, $sp, $t0
...
a8: 4c000181 jirl $ra, $t0, 0
...
dc: 02ff82c3 addi.d $sp, $fp, -32
e0: 28c06061 ld.d $ra, $sp, 24
e4: 28c04076 ld.d $fp, $sp, 16
e8: 28c02077 ld.d $s0, $sp, 8
ec: 02c08063 addi.d $sp, $sp, 32
f0: 4c000020 jirl $zero, $ra, 0
The instruction "sub.d $sp, $sp, $t0" changes the stack bottom
and the new stack size is a random value, in order to find the
return address of do_syscall() which is stored in the original
stack frame after executing "jirl $ra, $t0, 0", it should use
fp which points to the original stack top.
At the beginning, the thought is tended to decode the secondary
stack instruction "sub.d $sp, $sp, $t0" and set it as a label,
then check this label for the two frame pointer instructions to
change the cfa base and cfa offset during the period of secondary
stack in update_cfi_state(). This is valid for GCC rather than
Clang due to there are different secondary stack instructions for
ClangBuiltLinux on LoongArch, something like this:
0000000000000000 <do_syscall>:
...
88: 00119064 sub.d $a0, $sp, $a0
8c: 00150083 or $sp, $a0, $zero
...
Actually, it equals to a single instruction "sub.d $sp, $sp, $a0",
but there is no proper condition to check it as a label like GCC,
so the beginning thought is not a good way.
Essentially, there are two special frame pointer instructions
"addi.d $fp, $sp, imm" and "addi.d $sp, $fp, imm", the first one
points fp to the original stack top and the second one restores
the original stack bottom from fp.
Based on the above analysis, in order to not add a arch specified
update_cfi_state(), just add a member "frame_pointer" in the struct
symbol as a label to avoid affecting the current normal case, then
set it as true only if there is "addi.d $sp, $fp, imm", the last is
to check this label for the two frame pointer instructions to change
the cfa base and cfa offset in update_cfi_state().
Tested with the following two configs:
(1) CONFIG_RANDOMIZE_KSTACK_OFFSET=y &&
CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT=n
(2) CONFIG_RANDOMIZE_KSTACK_OFFSET=y &&
CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT=y
By the way, there is no effect for x86 with this patch, tested on
the x86 machine with Fedora 40 system.
Cc: stable@xxxxxxxxxxxxxxx # 6.9+
Signed-off-by: Tiezhu Yang <yangtiezhu@xxxxxxxxxxx>
---
tools/objtool/arch/loongarch/decode.c | 11 ++++++++++-
tools/objtool/check.c | 23 ++++++++++++++++++++---
tools/objtool/include/objtool/elf.h | 1 +
3 files changed, 31 insertions(+), 4 deletions(-)
LGTM.
Thanks!
Jinyang
diff --git a/tools/objtool/arch/loongarch/decode.c b/tools/objtool/arch/loongarch/decode.c
index aee479d2191c..69b66994f2a1 100644
--- a/tools/objtool/arch/loongarch/decode.c
+++ b/tools/objtool/arch/loongarch/decode.c
@@ -122,7 +122,7 @@ static bool decode_insn_reg2i12_fomat(union loongarch_instruction inst,
switch (inst.reg2i12_format.opcode) {
case addid_op:
if ((inst.reg2i12_format.rd == CFI_SP) || (inst.reg2i12_format.rj == CFI_SP)) {
- /* addi.d sp,sp,si12 or addi.d fp,sp,si12 */
+ /* addi.d sp,sp,si12 or addi.d fp,sp,si12 or addi.d sp,fp,si12 */
insn->immediate = sign_extend64(inst.reg2i12_format.immediate, 11);
ADD_OP(op) {
op->src.type = OP_SRC_ADD;
@@ -132,6 +132,15 @@ static bool decode_insn_reg2i12_fomat(union loongarch_instruction inst,
op->dest.reg = inst.reg2i12_format.rd;
}
}
+ if ((inst.reg2i12_format.rd == CFI_SP) && (inst.reg2i12_format.rj == CFI_FP)) {
+ /* addi.d sp,fp,si12 */
+ struct symbol *func = find_func_containing(insn->sec, insn->offset);
+
+ if (!func)
+ return false;
+
+ func->frame_pointer = true;
+ }
break;
case ldd_op:
if (inst.reg2i12_format.rj == CFI_SP) {
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 01237d167223..af9cfed7f4ec 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2993,10 +2993,27 @@ static int update_cfi_state(struct instruction *insn,
break;
}
- if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) {
+ if (op->dest.reg == CFI_BP && op->src.reg == CFI_SP &&
+ insn->sym->frame_pointer) {
+ /* addi.d fp,sp,imm on LoongArch */
+ if (cfa->base == CFI_SP && cfa->offset == op->src.offset) {
+ cfa->base = CFI_BP;
+ cfa->offset = 0;
+ }
+ break;
+ }
- /* lea disp(%rbp), %rsp */
- cfi->stack_size = -(op->src.offset + regs[CFI_BP].offset);
+ if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) {
+ /* addi.d sp,fp,imm on LoongArch */
+ if (cfa->base == CFI_BP && cfa->offset == 0) {
+ if (insn->sym->frame_pointer) {
+ cfa->base = CFI_SP;
+ cfa->offset = -op->src.offset;
+ }
+ } else {
+ /* lea disp(%rbp), %rsp */
+ cfi->stack_size = -(op->src.offset + regs[CFI_BP].offset);
+ }
break;
}
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index 2b8a69de4db8..d7e815c2fd15 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -68,6 +68,7 @@ struct symbol {
u8 warned : 1;
u8 embedded_insn : 1;
u8 local_label : 1;
+ u8 frame_pointer : 1;
struct list_head pv_target;
struct reloc *relocs;
};