linux-kernel - Re: [PATCH 0/8] unwind, arm64: add sframe unwinder for kernel

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: <20250206150212.2485132-1-wnliu@google.com>
Date: Thu,  6 Feb 2025 15:02:12 +0000
From: Weinan Liu <wnliu@...gle.com>
To: puranjay@...nel.org
Cc: indu.bhagat@...cle.com, irogers@...gle.com, joe.lawrence@...hat.com, 
	jpoimboe@...nel.org, 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, 
	roman.gushchin@...ux.dev, rostedt@...dmis.org, will@...nel.org, 
	wnliu@...gle.com
Subject: Re: [PATCH 0/8] unwind, arm64: add sframe unwinder for kernel

> After some debugging this is what I found:
> 
> devtmpfsd() calls devtmpfs_work_loop() which is marked '__noreturn' and has an
> infinite loop. The compiler puts the `bl` to devtmpfs_work_loop() as the the
> last instruction in devtmpfsd() and therefore on entry to devtmpfs_work_loop(),
> LR points to an instruction beyond devtmpfsd() and this consfuses the unwinder.
> 
> ffff800080d9a070 <devtmpfsd>:
> ffff800080d9a070:       d503201f        nop
> ffff800080d9a074:       d503201f        nop
> ffff800080d9a078:       d503233f        paciasp
> ffff800080d9a07c:       a9be7bfd        stp     x29, x30, [sp, #-32]!
> ffff800080d9a080:       910003fd        mov     x29, sp
> ffff800080d9a084:       f9000bf3        str     x19, [sp, #16]
> ffff800080d9a088:       943378e8        bl      ffff800081a78428 <devtmpfs_setup>
> ffff800080d9a08c:       90006ca1        adrp    x1, ffff800081b2e000 <unique_processor_ids+0x3758>
> ffff800080d9a090:       2a0003f3        mov     w19, w0
> ffff800080d9a094:       912de021        add     x1, x1, #0xb78
> ffff800080d9a098:       91002020        add     x0, x1, #0x8
> ffff800080d9a09c:       97cd2a43        bl      ffff8000800e49a8 <complete>
> ffff800080d9a0a0:       340000d3        cbz     w19, ffff800080d9a0b8 <devtmpfsd+0x48>
> ffff800080d9a0a4:       2a1303e0        mov     w0, w19
> ffff800080d9a0a8:       f9400bf3        ldr     x19, [sp, #16]
> ffff800080d9a0ac:       a8c27bfd        ldp     x29, x30, [sp], #32
> ffff800080d9a0b0:       d50323bf        autiasp
> ffff800080d9a0b4:       d65f03c0        ret
> ffff800080d9a0b8:       97f06526        bl      ffff8000809b3550 <devtmpfs_work_loop>
> ffff800080d9a0bc:       00000000        udf     #0
> ffff800080d9a0c0:       d503201f        nop
> ffff800080d9a0c4:       d503201f        nop
> 
> find_fde() got pc=0xffff800080d9a0bc which is not in [sfde_func_start_address, sfde_func_size)
> 
> output for readelf --sframe for devtmpfsd()
> 
> func idx [51825]: pc = 0xffff800080d9a070, size = 76 bytes
>     STARTPC           CFA       FP        RA
>     ffff800080d9a070  sp+0      u         u
>     ffff800080d9a07c  sp+0      u         u[s]
>     ffff800080d9a080  sp+32     c-32      c-24[s]
>     ffff800080d9a0b0  sp+0      u         u[s]
>     ffff800080d9a0b4  sp+0      u         u
>     ffff800080d9a0b8  sp+32     c-32      c-24[s]
> 
> The unwinder and all the related infra is assuming that the return address
> will be part of a valid function which is not the case here.
> 
> I am not sure which component needs to be fixed here, but the following
> patch(which is a hack) fixes the issue by considering the return address as
> part of the function descriptor entry.
> 
> -- 8< --
> 
> diff --git a/kernel/sframe_lookup.c b/kernel/sframe_lookup.c
> index 846f1da95..28bec5064 100644
> --- a/kernel/sframe_lookup.c
> +++ b/kernel/sframe_lookup.c
> @@ -82,7 +82,7 @@ static struct sframe_fde *find_fde(const struct sframe_table *tbl, unsigned long
>         if (f >= tbl->sfhdr_p->num_fdes || f < 0)
>                 return NULL;
>         fdep = tbl->fde_p + f;
> -       if (ip < fdep->start_addr || ip >= fdep->start_addr + fdep->size)
> +       if (ip < fdep->start_addr || ip > fdep->start_addr + fdep->size)
>                 return NULL;
> 
>         return fdep;
> @@ -106,7 +106,7 @@ static int find_fre(const struct sframe_table *tbl, unsigned long pc,
>         else
>                 ip_off = (int32_t)(pc - (unsigned long)tbl->sfhdr_p) - fdep->start_addr;
> 
> -       if (ip_off < 0 || ip_off >= fdep->size)
> +       if (ip_off < 0 || ip_off > fdep->size)
>                 return -EINVAL;
> 
>         /*
> 
> -- >8 --
> 
> Thanks,
> Puranjay

Thank you for reporting this issue.
I just found out that Josh also intentionally uses '>' instead of '>=' for the same reason
https://lore.kernel.org/lkml/20250122225257.h64ftfnorofe7cb4@jpoimboe/T/#m6d70a20ed9f5b3bbe5b24b24b8c5dcc603a79101

QQ, do we need to care the stacktrace after '__noreturn' function?