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: <CAPhsuW4BEDU=ZJavnofZtygGcQ9AJ5F2jJiuV6-nsnbZD+Gg-Q@mail.gmail.com>
Date: Tue, 18 Mar 2025 16:38:20 -0700
From: Song Liu <song@...nel.org>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-toolchains@...r.kernel.org, live-patching@...r.kernel.org, 
	indu.bhagat@...cle.com, puranjay@...nel.org, wnliu@...gle.com, 
	irogers@...gle.com, joe.lawrence@...hat.com, mark.rutland@....com, 
	peterz@...radead.org, roman.gushchin@...ux.dev, rostedt@...dmis.org, 
	will@...nel.org, kernel-team@...a.com
Subject: Re: [PATCH 1/2] arm64: Implement arch_stack_walk_reliable

On Tue, Mar 18, 2025 at 4:00 PM Josh Poimboeuf <jpoimboe@...nel.org> wrote:
>
> On Tue, Mar 18, 2025 at 01:14:40PM -0700, Song Liu wrote:
> > >
> > > See for example all the error conditions in the x86 version of
> > > arch_stack_walk_reliable() and in arch/x86/kernel/unwind_frame.c.
> >
> > I guess I missed this part:
> >
> > diff --git i/arch/arm64/kernel/stacktrace.c w/arch/arm64/kernel/stacktrace.c
> > index 69d0567a0c38..3bb8e3ea4c4b 100644
> > --- i/arch/arm64/kernel/stacktrace.c
> > +++ w/arch/arm64/kernel/stacktrace.c
> > @@ -268,6 +268,8 @@ kunwind_next(struct kunwind_state *state)
> >         case KUNWIND_SOURCE_TASK:
> >         case KUNWIND_SOURCE_REGS_PC:
> >                 err = kunwind_next_frame_record(state);
> > +               if (err && err != -ENOENT)
> > +                       state->common.unreliable = true;
> >                 break;
> >         default:
> >                 err = -EINVAL;
>
> I still see some issues:
>
>   - do_kunwind() -> kunwind_recover_return_address() can fail
>
>   - do_kunwind() -> consume_state() can fail

Great points! I have got the following:

diff --git i/arch/arm64/kernel/stacktrace.c w/arch/arm64/kernel/stacktrace.c
index 69d0567a0c38..852e4b322209 100644
--- i/arch/arm64/kernel/stacktrace.c
+++ w/arch/arm64/kernel/stacktrace.c
@@ -139,6 +139,7 @@ kunwind_recover_return_address(struct kunwind_state *state)
                                                (void *)state->common.fp);
                if (state->common.pc == orig_pc) {
                        WARN_ON_ONCE(state->task == current);
+                       state->common.unreliable = true;
                        return -EINVAL;
                }
                state->common.pc = orig_pc;
@@ -268,6 +269,8 @@ kunwind_next(struct kunwind_state *state)
        case KUNWIND_SOURCE_TASK:
        case KUNWIND_SOURCE_REGS_PC:
                err = kunwind_next_frame_record(state);
+               if (err && err != -ENOENT)
+                       state->common.unreliable = true;
                break;
        default:
                err = -EINVAL;
@@ -404,12 +407,16 @@ static __always_inline bool
 arch_kunwind_reliable_consume_entry(const struct kunwind_state
*state, void *cookie)
 {
        struct kunwind_reliable_consume_entry_data *data = cookie;
+       bool ret;

        if (state->common.unreliable) {
                data->unreliable = true;
                return false;
        }
-       return data->consume_entry(data->cookie, state->common.pc);
+       ret = data->consume_entry(data->cookie, state->common.pc);
+       if (!ret)
+               data->unreliable = true;
+       return ret;
 }

 noinline noinstr int arch_stack_walk_reliable(stack_trace_consume_fn
consume_entry,


>   - even in the -ENOENT case the unreliable bit has already been set
>     right before the call to kunwind_next_frame_record_meta().

For this one, do you mean we set state->common.unreliable, but
failed to propagate it to data.unreliable?

Thanks,
Song