| 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
| ||
|
Message-ID: <20210318161723.GA10758@arm.com>
Date: Thu, 18 Mar 2021 16:17:24 +0000
From: Catalin Marinas <catalin.marinas@....com>
To: Mark Rutland <mark.rutland@....com>
Cc: Chen Jun <chenjun102@...wei.com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, akpm@...ux-foundation.org,
will@...nel.org, rui.xiang@...wei.com,
Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current
On Wed, Mar 17, 2021 at 07:34:16PM +0000, Mark Rutland wrote:
> On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote:
> > On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote:
> > > On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same
> > > stack:
> > > stack_trace_save+0x4c/0x78
> > > register_early_stack+0x34/0x70
> > > init_page_owner+0x34/0x230
> > > page_ext_init+0x1bc/0x1dc
> > >
> > > The reason is that:
> > > check_recursive_alloc always return 1 because that
> > > entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).
> > >
> > > The root cause is that:
> > > commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> > > make the save_trace save 2 more entries.
> > >
> > > Add skip in arch_stack_walk when task == current.
> > >
> > > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> > > Signed-off-by: Chen Jun <chenjun102@...wei.com>
> > > ---
> > > arch/arm64/kernel/stacktrace.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > > index ad20981..c26b0ac 100644
> > > --- a/arch/arm64/kernel/stacktrace.c
> > > +++ b/arch/arm64/kernel/stacktrace.c
> > > @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> > >
> > > if (regs)
> > > start_backtrace(&frame, regs->regs[29], regs->pc);
> > > - else if (task == current)
> > > + else if (task == current) {
> > > + ((struct stacktrace_cookie *)cookie)->skip += 2;
> > > start_backtrace(&frame,
> > > (unsigned long)__builtin_frame_address(0),
> > > (unsigned long)arch_stack_walk);
> > > - else
> > > + } else
> > > start_backtrace(&frame, thread_saved_fp(task),
> > > thread_saved_pc(task));
> >
> > I don't like abusing the cookie here. It's void * as it's meant to be an
> > opaque type. I'd rather skip the first two frames in walk_stackframe()
> > instead before invoking fn().
>
> I agree that we shouldn't touch cookie here.
>
> I don't think that it's right to bodge this inside walk_stackframe(),
> since that'll add bogus skipping for the case starting with regs in the
> current task. If we need a bodge, it has to live in arch_stack_walk()
> where we set up the initial unwinding state.
Good point. However, instead of relying on __builtin_frame_address(1),
can we add a 'skip' value to struct stackframe via arch_stack_walk() ->
start_backtrace() that is consumed by walk_stackframe()?
> In another thread, we came to the conclusion that arch_stack_walk()
> should start at its parent, and its parent should add any skipping it
> requires.
This makes sense.
> Currently, arch_stack_walk() is off-by-one, and we can bodge that by
> using __builtin_frame_address(1), though I'm waiting for some compiler
> folk to confirm that's sound. Otherwise we need to add an assembly
> trampoline to snapshot the FP, which is unfortunastely convoluted.
>
> This report suggests that a caller of arch_stack_walk() is off-by-one
> too, which suggests a larger cross-architecture semantic issue. I'll try
> to take a look tomorrow.
I don't think the caller is off by one, at least not by the final skip
value. __set_page_owner() wants the trace to start at its caller. The
callee save_stack() in the same file adds a skip of 2.
save_stack_trace() increments the skip before invoking
arch_stack_walk(). So far, this assumes that arch_stack_walk() starts at
its parent, i.e. save_stack_trace().
So save_stack_trace() only need to skip 1 and I think that's in line
with the original report where the entries[0] is __set_page_owner(). We
only need to skip one. Another untested quick hack (we should probably
add the skip argument to start_backtrace()):
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index eb29b1fe8255..0d32d932ac89 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -56,6 +56,7 @@ struct stackframe {
DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
unsigned long prev_fp;
enum stack_type prev_type;
+ int skip;
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
int graph;
#endif
@@ -153,6 +154,7 @@ static inline void start_backtrace(struct stackframe *frame,
{
frame->fp = fp;
frame->pc = pc;
+ frame->skip = 0;
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
frame->graph = 0;
#endif
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ad20981dfda4..a89b2ecbf3de 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -118,7 +118,9 @@ void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
while (1) {
int ret;
- if (!fn(data, frame->pc))
+ if (frame->skip > 0)
+ frame->skip--;
+ else if (!fn(data, frame->pc))
break;
ret = unwind_frame(tsk, frame);
if (ret < 0)
@@ -201,11 +203,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
if (regs)
start_backtrace(&frame, regs->regs[29], regs->pc);
- else if (task == current)
+ else if (task == current) {
start_backtrace(&frame,
(unsigned long)__builtin_frame_address(0),
(unsigned long)arch_stack_walk);
- else
+ frame.skip = 1;
+ } else
start_backtrace(&frame, thread_saved_fp(task),
thread_saved_pc(task));
--
Catalin
Powered by blists - more mailing lists