linux-kernel - Re: [PATCH v2 2/3] objtool: Handle secondary stack related instructions

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: <3bea8b7f-b783-ba85-1ed0-c6bd3f6af92b@loongson.cn>
Date: Mon, 5 Aug 2024 14:37:57 +0800
From: Tiezhu Yang <yangtiezhu@...ngson.cn>
To: 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 v2 2/3] objtool: Handle secondary stack related
 instructions

On 08/05/2024 11:26 AM, 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".

...

> 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) {

Here should be CFI_BP instead of CFI_FP which is only defined
for LoongArch.

> +					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 */

Oh, sorry, I forgot to test this change on x86.

Here is the test info on x86: the cfa->base is CFI_BP
or CFI_BP_INDIRECT and the cfa->offset is not 0
if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP),
thus it can check the following condition
if(cfa->base == CFI_BP && cfa->offset == 0)
to distinguish x86 and LoongArch.

So the correct change should be something like this
to make sure it works well for both x86 and LoongArch:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 01237d167223..0832d20c95d2 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2993,10 +2993,28 @@ static int update_cfi_state(struct instruction 
*insn,
                                 break;
                         }

-                       if (op->dest.reg == CFI_SP && op->src.reg == 
CFI_BP) {
+                       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;
+                       }

-                               /* lea disp(%rbp), %rsp */
-                               cfi->stack_size = -(op->src.offset + 
regs[CFI_BP].offset);
+                       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_BP && cfa->offset 
== 0) {
+                                       if (insn->sym->secondary_stack) {
+                                               cfa->base = CFI_SP;
+                                               cfa->offset = 
-op->src.offset;
+                                       }
+                               } else {
+                                       /* lea disp(%rbp), %rsp */
+                                       cfi->stack_size = 
-(op->src.offset + regs[CFI_BP].offset);
+                               }
                                 break;
                         }

I will wait for some days to get more review comments
and then send v3 later.

Thanks,
Tiezhu