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] [day] [month] [year] [list]

Message-ID: <CAPhsuW7Zz9EaW3+gtp2Ut-8DVB4F=Dacr2btJ8gL0OA4T6bQzA@mail.gmail.com>
Date: Wed, 19 Mar 2025 12:09:17 -0700
From: Song Liu <song@...nel.org>
To: Weinan Liu <wnliu@...gle.com>
Cc: jpoimboe@...nel.org, 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, will@...nel.org
Subject: Re: [PATCH 1/2] arm64: Implement arch_stack_walk_reliable

On Wed, Mar 19, 2025 at 11:38 AM Weinan Liu <wnliu@...gle.com> wrote:
>
> 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.

Agreed with this: all -EINVALID cases or `FRAME_META_TYPE_PT_REGS`
should be unreliable, IIUC.

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

I am current leaning toward not using common.unreliable. It seems to add
unnecessary complexity here. But I may change my mind later on.

Thanks,
Song

>                         break;
>         }
>  }
> ```
>
> --
> Weinan