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 <[email protected]> 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 <[email protected]>
> ---
>  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
>