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

[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
>    ...
>    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 <lkp@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407201336.mW8dj1VB-lkp@xxxxxxxxx/
> Fixes: b2d23158e6c8 ("objtool/LoongArch: Implement instruction decoder")
> Signed-off-by: Tiezhu Yang <yangtiezhu@xxxxxxxxxxx>
> ---
>   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;