linux-kernel - Re: [PATCH v4 17/39] unwind_user/sframe: Add support for reading .sframe headers

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: <5cda6984-800c-45c6-867a-8fc0bb267c8a@oracle.com>
Date: Fri, 24 Jan 2025 12:31:59 -0800
From: Indu Bhagat <indu.bhagat@...cle.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>, Ingo Molnar <mingo@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        linux-kernel@...r.kernel.org, Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-perf-users@...r.kernel.org, Mark Brown <broonie@...nel.org>,
        linux-toolchains@...r.kernel.org, Jordan Rome <jordalgo@...a.com>,
        Sam James <sam@...too.org>, linux-trace-kernel@...r.kernel.org,
        Jens Remus <jremus@...ux.ibm.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Florian Weimer <fweimer@...hat.com>, Andy Lutomirski <luto@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>, Weinan Liu <wnliu@...gle.com>
Subject: Re: [PATCH v4 17/39] unwind_user/sframe: Add support for reading
 .sframe headers

On 1/24/25 10:00 AM, Andrii Nakryiko wrote:
> On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf<jpoimboe@...nel.org> wrote:
>> In preparation for unwinding user space stacks with sframe, add basic
>> sframe compile infrastructure and support for reading the .sframe
>> section header.
>>
>> sframe_add_section() reads the header and unconditionally returns an
>> error, so it's not very useful yet.  A subsequent patch will improve
>> that.
>>
>> Signed-off-by: Josh Poimboeuf<jpoimboe@...nel.org>
>> ---
>>   arch/Kconfig           |   3 +
>>   include/linux/sframe.h |  36 +++++++++++
>>   kernel/unwind/Makefile |   3 +-
>>   kernel/unwind/sframe.c | 136 +++++++++++++++++++++++++++++++++++++++++
>>   kernel/unwind/sframe.h |  71 +++++++++++++++++++++
>>   5 files changed, 248 insertions(+), 1 deletion(-)
>>   create mode 100644 include/linux/sframe.h
>>   create mode 100644 kernel/unwind/sframe.c
>>   create mode 100644 kernel/unwind/sframe.h
>>
> [...]
> 
>> +
>> +extern int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
>> +                             unsigned long text_start, unsigned long text_end);
>> +extern int sframe_remove_section(unsigned long sframe_addr);
>> +
>> +#else /* !CONFIG_HAVE_UNWIND_USER_SFRAME */
>> +
>> +static inline int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end, unsigned long text_start, unsigned long text_end) { return -ENOSYS; }
> nit: very-very long, wrap it?
> 
>> +static inline int sframe_remove_section(unsigned long sframe_addr) { return -ENOSYS; }
>> +
>> +#endif /* CONFIG_HAVE_UNWIND_USER_SFRAME */
>> +
>> +#endif /* _LINUX_SFRAME_H */
> [...]
> 
>> +static int sframe_read_header(struct sframe_section *sec)
>> +{
>> +       unsigned long header_end, fdes_start, fdes_end, fres_start, fres_end;
>> +       struct sframe_header shdr;
>> +       unsigned int num_fdes;
>> +
>> +       if (copy_from_user(&shdr, (void __user *)sec->sframe_start, sizeof(shdr))) {
>> +               dbg("header usercopy failed\n");
>> +               return -EFAULT;
>> +       }
>> +
>> +       if (shdr.preamble.magic != SFRAME_MAGIC ||
>> +           shdr.preamble.version != SFRAME_VERSION_2 ||
>> +           !(shdr.preamble.flags & SFRAME_F_FDE_SORTED) ||
> probably more a question to Indu, but why is this sorting not
> mandatory and part of SFrame "standard"? How realistically non-sorted
> FDEs would work in practice? Ain't nobody got time to sort them just
> to unwind the stack...

It is not worthwhile for the assembler (even wasteful as it adds to 
build time for nothing) to generate an .sframe section with FDEs in 
sorted order of start PC of function.  This is because the final order 
is decided by the linker as it merges all input sections.

Thats one reason why it is already necessary that the specification 
allows SFRAME_F_FDE_SORTED not set in the section. I can also see how 
not making the sorting mandatory may also be necessary for JIT use-case..

FWIW, for non-JIT environments, non-sorted FDEs are not expected in 
linked binaries; such a thing does not seem to be useful in practice.

Hope that helps
Indu