[PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

[Tiezhu Yang]

After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
support"), the code flow of do_syscall() was changed when compiled
with GCC due to the secondary stack of add_random_kstack_offset(),
something like this:

addi.d $sp, $sp, -32
st.d $fp, $sp, 16
st.d $ra, $sp, 24
addi.d $fp, $sp, 32
...
sub.d $sp, $sp, $t1
...
addi.d $sp, $fp, -32
ld.d $ra, $sp, 24
ld.d $fp, $sp, 16
addi.d $sp, $sp, 32

fp points to the stack top, it is only used to save and restore the
original sp and is not used as cfa base for arch_callee_saved_reg().
In the case OP_SRC_ADD of update_cfi_state(), the above rare case is
not handled so that lead to a wrong stack size, then there exists a
objtool warning "do_syscall+0x11c: return with modified stack frame".

Because the fp related instructions do not modify the stack frame,
no need to decode them, just restrict stack operation instruction
only with the single case "addi.d sp,sp,si12".

By the way, if fp is used as cfa base for arch_callee_saved_reg()
(there is no this behavior on LoongArch at present), then it needs
to decode the related instructions and modify update_cfi_state().

Reported-by: kernel test robot <l...@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202407201336...@intel.com/
Fixes: b2d23158e6c8 ("objtool/LoongArch: Implement instruction decoder")
Signed-off-by: Tiezhu Yang <yangt...@loongson.cn>
---
tools/objtool/arch/loongarch/decode.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/arch/loongarch/decode.c b/tools/objtool/arch/loongarch/decode.c
index aee479d2191c..6a34af675cee 100644
--- a/tools/objtool/arch/loongarch/decode.c
+++ b/tools/objtool/arch/loongarch/decode.c
@@ -121,8 +121,8 @@ 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 */
+ if ((inst.reg2i12_format.rd == CFI_SP) && (inst.reg2i12_format.rj == CFI_SP)) {
+ /* addi.d sp,sp,si12 */
insn->immediate = sign_extend64(inst.reg2i12_format.immediate, 11);
ADD_OP(op) {
op->src.type = OP_SRC_ADD;
--
2.42.0

[Huacai Chen]

Hi, Tiezhu,

Can this patch also fix the warnings of ClangBuiltLinux?

Huacai

[Jinyang He]

On 2024-07-30 14:19, Tiezhu Yang wrote:

> After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
> support"), the code flow of do_syscall() was changed when compiled
> with GCC due to the secondary stack of add_random_kstack_offset(),
> something like this:
>
> addi.d $sp, $sp, -32
> st.d $fp, $sp, 16
> st.d $ra, $sp, 24
> addi.d $fp, $sp, 32
> ...
> sub.d $sp, $sp, $t1

Have you checked the ORC info whether is right or tried backtrace which
passed do_syscall? The "sub.d $sp, $sp, $t1" has modified the $sp so the
$sp cannot express CFA here. This patch just clear the warning but ignore
the validity of ORC info. The wrong ORC info may cause illegally access
memory when backtrace.


Thanks,
Jinyang

[Tiezhu Yang]

On 07/30/2024 05:28 PM, Jinyang He wrote:
> On 2024-07-30 14:19, Tiezhu Yang wrote:
>
>> After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
>> support"), the code flow of do_syscall() was changed when compiled
>> with GCC due to the secondary stack of add_random_kstack_offset(),
>> something like this:
>>
>> addi.d $sp, $sp, -32
>> st.d $fp, $sp, 16
>> st.d $ra, $sp, 24
>> addi.d $fp, $sp, 32
>> ...
>> sub.d $sp, $sp, $t1
> Have you checked the ORC info whether is right or tried backtrace which
> passed do_syscall? The "sub.d $sp, $sp, $t1" has modified the $sp so the
> $sp cannot express CFA here. This patch just clear the warning but ignore
> the validity of ORC info. The wrong ORC info may cause illegally access
> memory when backtrace.

I did testing many times before submitting, the call trace is
expected when testing "echo l > /proc/sysrq-trigger".

Thanks,
Tiezhu

[Jinyang He]

Make sure the RANDOMIZE_KSTACK_OFFSET is enable. I tested it by
CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT=y on qemu and it did
not show the frame about handle_syscall.

On the other hand,
$ ./tools/objtool/objtool --dump arch/loongarch/kernel/syscall.o
.noinstr.text+0:type:call sp:sp +   0 fp: (und)  ra: (und)  signal:0
.noinstr.text+4:type:call sp:sp +  32 fp: (und)  ra: (und)  signal:0
.noinstr.text+8:type:call sp:sp +  32 fp:prevsp + -16 ra: (und) signal:0
.noinstr.text+c:type:call sp:sp +  32 fp:prevsp + -16 ra:prevsp + -8
signal:0
.noinstr.text+e4:type:call sp:sp +  32 fp:prevsp + -16 ra: (und) signal:0
.noinstr.text+e8:type:call sp:sp +  32 fp: (und)  ra: (und) signal:0
.noinstr.text+ec:type:call sp:sp +   0 fp: (und)  ra: (und) signal:0
.noinstr.text+f0:type:? sp: (und)  fp: (und)  ra: (und)  signal:0
.noinstr.text+100:type:call sp:sp +  32 fp:prevsp + -16 ra:prevsp + -8
signal:0
.noinstr.text+118:type:? sp: (und)  fp: (und)  ra: (und)  signal:0
$ objdump -d arch/loongarch/kernel/syscall.o | grep "sub.*sp.*sp"
  70:    0011b463     sub.d           $sp, $sp, $t1
Obviously the ORC info is wrong.
(Assume the backtrace PC is 0x74, how to find the previous frame?)