linux-kernel - Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

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]

Date:   Mon, 11 Oct 2021 11:45:22 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        "Naveen N . Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        Ananth N Mavinakayanahalli <ananth@...ux.ibm.com>,
        Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
        Sven Schnelle <svens@...ux.ibm.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Nathan Chancellor <nathan@...nel.org>,
        linux-arm-kernel@...ts.infradead.org,
        Nathan Huckleberry <nhuck@...gle.com>
Subject: Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

On Fri, Oct 8, 2021 at 5:29 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> Currently the stacktrace on clang compiled arm kernel uses the 'lr'
> register to find the first frame address from pt_regs. However, that
> is wrong after calling another function, because the 'lr' register
> is used by 'bl' instruction and never be recovered.
>
> As same as gcc arm kernel, directly use the frame pointer (x11) of
> the pt_regs to find the first frame address.

Hi Masami,
Thanks for the patch. Testing with ARCH=arm defconfig (multi_v7_defconfig)

Before this patch:

$ mount -t proc /proc
$ echo 0 > /proc/sys/kernel/kptr_restrict
$ cat /proc/self/stack
[<0>] proc_single_show+0x4c/0xb8
[<0>] seq_read_iter+0x174/0x4d8
[<0>] seq_read+0x134/0x158
[<0>] vfs_read+0xcc/0x2f8
[<0>] ksys_read+0x74/0xd0
[<0>] __entry_text_start+0x14/0x14
[<0>] 0xbea38cc0

After this patch:
$ mount -t proc /proc
$ echo 0 > /proc/sys/kernel/kptr_restrict
$ cat /proc/self/stack
[<0>] proc_single_show+0x4c/0xb8
[<0>] seq_read_iter+0x174/0x4d8
[<0>] seq_read+0x134/0x158
[<0>] vfs_read+0xcc/0x2f8
[<0>] ksys_read+0x74/0xd0
[<0>] __entry_text_start+0x14/0x14
[<0>] 0xbeb55cc0

Is there a different way to test/verify this patch? (I'm pretty sure
we had verified the WARN_ONCE functionality with this, too.)

If I change from CONFIG_UNWINDER_ARM=y to
CONFIG_UNWINDER_FRAME_POINTER=y, before:

# cat /proc/self/stack
[<0>] stack_trace_save_tsk+0x50/0x6c
[<0>] proc_pid_stack+0xa0/0xf8
[<0>] proc_single_show+0x50/0xbc
[<0>] seq_read_iter+0x178/0x4ec
[<0>] seq_read+0x138/0x15c
[<0>] vfs_read+0xd0/0x304
[<0>] ksys_read+0x78/0xd4
[<0>] sys_read+0xc/0x10

after:
# cat /proc/self/stack
[<0>] proc_pid_stack+0xa0/0xf8
[<0>] proc_single_show+0x50/0xbc
[<0>] seq_read_iter+0x178/0x4ec
[<0>] seq_read+0x138/0x15c
[<0>] vfs_read+0xd0/0x304
[<0>] ksys_read+0x78/0xd4
[<0>] sys_read+0xc/0x10
[<0>] __entry_text_start+0x14/0x14
[<0>] 0xffffffff

So I guess this helps the CONFIG_UNWINDER_FRAME_POINTER=y case? (That
final frame address looks wrong, but is potentially yet another bug;
perhaps for clang we need to manually store the previous frame's pc at
a different offset before jumping to __entry_text_start).

Also, I'm curious about CONFIG_THUMB2_KERNEL (forces CONFIG_UNWINDER_ARM=y).

before:
# cat /proc/self/stack
[<0>] proc_single_show+0x31/0x86
[<0>] seq_read_iter+0xff/0x326
[<0>] seq_read+0xd7/0xf2
[<0>] vfs_read+0x93/0x20e
[<0>] ksys_read+0x53/0x92
[<0>] ret_fast_syscall+0x1/0x52
[<0>] 0xbe9a9cc0

after:
# cat /proc/self/stack
[<0>] proc_single_show+0x31/0x86
[<0>] seq_read_iter+0xff/0x326
[<0>] seq_read+0xd7/0xf2
[<0>] vfs_read+0x93/0x20e
[<0>] ksys_read+0x53/0x92
[<0>] ret_fast_syscall+0x1/0x52
[<0>] 0xbec08cc0

Tested-by: Nick Desaulniers <ndesaulniers@...gle.com>

so likely this fixes/improves CONFIG_UNWINDER_FRAME_POINTER=y? Is that correct?

>
> Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> ---
>  arch/arm/kernel/stacktrace.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> index 76ea4178a55c..db798eac7431 100644
> --- a/arch/arm/kernel/stacktrace.c
> +++ b/arch/arm/kernel/stacktrace.c
> @@ -54,8 +54,7 @@ int notrace unwind_frame(struct stackframe *frame)
>
>         frame->sp = frame->fp;
>         frame->fp = *(unsigned long *)(fp);
> -       frame->pc = frame->lr;
> -       frame->lr = *(unsigned long *)(fp + 4);
> +       frame->pc = *(unsigned long *)(fp + 4);
>  #else
>         /* check current frame pointer is within bounds */
>         if (fp < low + 12 || fp > high - 4)
>

-- 
Thanks,
~Nick Desaulniers