linux-kernel - Re: [PATCH 1/2] arm64: Implement arch_stack_walk

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: <20250319183757.404779-1-wnliu@google.com>
Date: Wed, 19 Mar 2025 18:37:56 +0000
From: Weinan Liu <wnliu@...gle.com>
To: jpoimboe@...nel.org
Cc: indu.bhagat@...cle.com, irogers@...gle.com, joe.lawrence@...hat.com, 
	kernel-team@...a.com, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-toolchains@...r.kernel.org, 
	live-patching@...r.kernel.org, mark.rutland@....com, peterz@...radead.org, 
	puranjay@...nel.org, roman.gushchin@...ux.dev, rostedt@...dmis.org, 
	song@...nel.org, will@...nel.org, wnliu@...gle.com
Subject: Re: [PATCH 1/2] arm64: Implement arch_stack_walk_reliable

On Tue, Mar 18, 2025 at 10:39 PM Josh Poimboeuf <jpoimboe@...nel.org> wrote:
>
> On Tue, Mar 18, 2025 at 08:58:52PM -0700, Song Liu wrote:
> > On a closer look, I think we also need some logic in unwind_find_stack()
> > so that we can see when the unwinder hits the exception boundary. For
> > this reason, we may still need unwind_state.unreliable. I will look into
> > fixing this and send v2.
>
> Isn't that what FRAME_META_TYPE_PT_REGS is for?
>
> Maybe it can just tell kunwind_stack_walk() to set a bit in
> kunwind_state which tells kunwind_next_frame_record_meta() to quit the
> unwind early for the FRAME_META_TYPE_PT_REGS case.  That also has the
> benefit of stopping the unwind as soon as the exception is encounterd.
>

After reviewing the code flow, it seems like we should treat all -EINVALID
cases or `FRAME_META_TYPE_PT_REGS` cases as unreliable unwinds.

Would a simplification like the one below work?
Or we can return a special value for success cases in kunwind_next_regs_pc() 

```
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 69d0567a0c38..0eb69fa6161a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -296,7 +296,8 @@ do_kunwind(struct kunwind_state *state, kunwind_consume_fn consume_state,
 		if (!consume_state(state, cookie))
 			break;
 		ret = kunwind_next(state);
-		if (ret < 0)
+		if (ret < 0 || state->source == KUNWIND_SOURCE_REGS_PC)
+			state->common.unreliable = true;
 			break;
 	}
 }
```

--
Weinan