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: <4ac60afc-de6b-acf6-c9e6-1f45c0680dbe@loongson.cn>
Date: Tue, 30 Jul 2024 17:28:48 +0800
From: Jinyang He <hejinyang@...ngson.cn>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>, Josh Poimboeuf
 <jpoimboe@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
 Huacai Chen <chenhuacai@...nel.org>
Cc: loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] objtool/LoongArch: Restrict stack operation
 instruction

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@...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;