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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAhV-H5qoaBwJe929+GnYzbrbmcMgTXy5zL8smXDmOSCtAQntw@mail.gmail.com>
Date: Tue, 30 Jul 2024 16:59:25 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc: Josh Poimboeuf <jpoimboe@...nel.org>, Peter Zijlstra <peterz@...radead.org>, loongarch@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

Hi, Tiezhu,

Can this patch also fix the warnings of ClangBuiltLinux?

Huacai

On Tue, Jul 30, 2024 at 2:19 PM Tiezhu Yang <yangtiezhu@...ngson.cn> 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
>   ...
>   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@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407201336.mW8dj1VB-lkp@intel.com/
> Fixes: b2d23158e6c8 ("objtool/LoongArch: Implement instruction decoder")
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.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
>
>