Messages in this thread |  | | Subject | Re: [PATCH v2 2/3] objtool: Handle secondary stack related instructions | | From | Jinyang He <> | | Date | Mon, 5 Aug 2024 18:15:01 +0800 |
| |
On 2024-08-05 11:26, Tiezhu Yang wrote:
> After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET > support"), there is a new instruction "sub.d $sp, $sp, $t0" for the > secondary stack in do_syscall(), then there exists a objtool warning > "do_syscall+0x11c: return with modified stack frame" and there is 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. > > This is a rare case, add a member "secondary_stack" in the struct > symbol as a label to avoid affecting the current normal case, then > set it as true only if there exists the secondary stack instruction > "sub.d $sp, $sp, $t0", at last check this label for the other special > secondary stack instructions about fp to change the cfa base and cfa > offset during the period of secondary stack 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 > > Cc: stable@vger.kernel.org # 6.9+ > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > tools/objtool/arch/loongarch/decode.c | 8 +++++++- > tools/objtool/check.c | 22 ++++++++++++++++++++++ > tools/objtool/include/objtool/elf.h | 1 + > 3 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/tools/objtool/arch/loongarch/decode.c b/tools/objtool/arch/loongarch/decode.c > index db4dd05cdb49..4085714ffd18 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; > @@ -275,6 +275,8 @@ static bool decode_insn_reg2i16_fomat(union loongarch_instruction inst, > static bool decode_insn_reg3_fomat(union loongarch_instruction inst, > struct instruction *insn) > { > + struct symbol *func; > + > switch (inst.reg3_format.opcode) { > case subd_op: > if ((inst.reg3_format.rd == CFI_SP) && (inst.reg3_format.rj == CFI_SP)) { > @@ -282,6 +284,10 @@ static bool decode_insn_reg3_fomat(union loongarch_instruction inst, > * sub.d sp,sp,t0 > * this is a rare case for the secondary stack. > */ > + func = find_func_containing(insn->sec, insn->offset); > + if (!func) > + return false; > + func->secondary_stack = true; > } > break; > default: > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 01237d167223..c7b9942fee29 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -2993,6 +2993,28 @@ static int update_cfi_state(struct instruction *insn, > break; > } > > + if (op->dest.reg == CFI_BP && op->src.reg == CFI_SP) { > + /* addi.d fp,sp,imm for the secondary stack on LoongArch */ > + if (cfa->base == CFI_SP && cfa->offset == op->src.offset) { > + if (insn->sym->secondary_stack) { > + cfa->base = CFI_BP; > + cfa->offset = 0; > + } > + } > + break; > + } > + > + if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) { > + /* addi.d sp,fp,imm for the secondary stack on LoongArch */ > + if (cfa->base == CFI_FP && cfa->offset == 0) { > + if (insn->sym->secondary_stack) { > + cfa->base = CFI_SP; > + cfa->offset = -op->src.offset; > + } > + } > + break; > + } > + > if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) { > > /* lea disp(%rbp), %rsp */ > diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h > index 2b8a69de4db8..586916e0d441 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 secondary_stack : 1; > struct list_head pv_target; > struct reloc *relocs; > };
LGTM although I think we should make update_cfi_state() arch-specific. It also fix many warning when use Clang, thanks.
Jinyang
|  |