| 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: <CAEf4BzbQGipBq-Ls1=n49nyNoX941Ka13HgXAXLTzGAo9wckAA@mail.gmail.com>
Date: Fri, 24 Jan 2025 10:00:52 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>, Indu Bhagat <indu.bhagat@...cle.com>
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 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...
> + shdr.auxhdr_len) {
> + dbg("bad/unsupported sframe header\n");
> + return -EINVAL;
> + }
> +
> + if (!shdr.num_fdes || !shdr.num_fres) {
given SFRAME_F_FRAME_POINTER in the header, is it really that
nonsensical and illegal to have zero FDEs/FREs? Maybe we should allow
that?
> + dbg("no fde/fre entries\n");
> + return -EINVAL;
> + }
> +
> + header_end = sec->sframe_start + SFRAME_HEADER_SIZE(shdr);
> + if (header_end >= sec->sframe_end) {
if we allow zero FDEs/FREs, header_end == sec->sframe_end is legal, right?
> + dbg("header doesn't fit in section\n");
> + return -EINVAL;
> + }
> +
> + num_fdes = shdr.num_fdes;
> + fdes_start = header_end + shdr.fdes_off;
> + fdes_end = fdes_start + (num_fdes * sizeof(struct sframe_fde));
> +
> + fres_start = header_end + shdr.fres_off;
> + fres_end = fres_start + shdr.fre_len;
> +
maybe use check_add_overflow() in all the above calculation, at least
on 32-bit arches this all can overflow and it's not clear if below
sanity check detects all possible overflows
> + if (fres_start < fdes_end || fres_end > sec->sframe_end) {
> + dbg("inconsistent fde/fre offsets\n");
> + return -EINVAL;
> + }
> +
> + sec->num_fdes = num_fdes;
> + sec->fdes_start = fdes_start;
> + sec->fres_start = fres_start;
> + sec->fres_end = fres_end;
> +
> + sec->ra_off = shdr.cfa_fixed_ra_offset;
> + sec->fp_off = shdr.cfa_fixed_fp_offset;
> +
> + return 0;
> +}
> +
[...]
> diff --git a/kernel/unwind/sframe.h b/kernel/unwind/sframe.h
> new file mode 100644
> index 000000000000..e9bfccfaf5b4
> --- /dev/null
> +++ b/kernel/unwind/sframe.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * From https://www.sourceware.org/binutils/docs/sframe-spec.html
> + */
> +#ifndef _SFRAME_H
> +#define _SFRAME_H
> +
> +#include <linux/types.h>
> +
> +#define SFRAME_VERSION_1 1
> +#define SFRAME_VERSION_2 2
> +#define SFRAME_MAGIC 0xdee2
> +
> +#define SFRAME_F_FDE_SORTED 0x1
> +#define SFRAME_F_FRAME_POINTER 0x2
> +
> +#define SFRAME_ABI_AARCH64_ENDIAN_BIG 1
> +#define SFRAME_ABI_AARCH64_ENDIAN_LITTLE 2
> +#define SFRAME_ABI_AMD64_ENDIAN_LITTLE 3
> +
> +#define SFRAME_FDE_TYPE_PCINC 0
> +#define SFRAME_FDE_TYPE_PCMASK 1
> +
> +struct sframe_preamble {
> + u16 magic;
> + u8 version;
> + u8 flags;
> +} __packed;
> +
> +struct sframe_header {
> + struct sframe_preamble preamble;
> + u8 abi_arch;
> + s8 cfa_fixed_fp_offset;
> + s8 cfa_fixed_ra_offset;
> + u8 auxhdr_len;
> + u32 num_fdes;
> + u32 num_fres;
> + u32 fre_len;
> + u32 fdes_off;
> + u32 fres_off;
> +} __packed;
> +
> +#define SFRAME_HEADER_SIZE(header) \
> + ((sizeof(struct sframe_header) + header.auxhdr_len))
> +
> +#define SFRAME_AARCH64_PAUTH_KEY_A 0
> +#define SFRAME_AARCH64_PAUTH_KEY_B 1
> +
> +struct sframe_fde {
> + s32 start_addr;
> + u32 func_size;
> + u32 fres_off;
> + u32 fres_num;
> + u8 info;
> + u8 rep_size;
> + u16 padding;
> +} __packed;
I couldn't understand from SFrame itself, but why do sframe_header,
sframe_preamble, and sframe_fde have to be marked __packed, if it's
all naturally aligned (intentionally and by design)?..
> +
> +#define SFRAME_FUNC_FRE_TYPE(data) (data & 0xf)
> +#define SFRAME_FUNC_FDE_TYPE(data) ((data >> 4) & 0x1)
> +#define SFRAME_FUNC_PAUTH_KEY(data) ((data >> 5) & 0x1)
> +
> +#define SFRAME_BASE_REG_FP 0
> +#define SFRAME_BASE_REG_SP 1
> +
> +#define SFRAME_FRE_CFA_BASE_REG_ID(data) (data & 0x1)
> +#define SFRAME_FRE_OFFSET_COUNT(data) ((data >> 1) & 0xf)
> +#define SFRAME_FRE_OFFSET_SIZE(data) ((data >> 5) & 0x3)
> +#define SFRAME_FRE_MANGLED_RA_P(data) ((data >> 7) & 0x1)
> +
> +#endif /* _SFRAME_H */
> --
> 2.48.1
>
Powered by blists - more mailing lists