Re: [PATCH v4 19/39] unwind_user/sframe: Add support for reading .sframe contents

[Andrii Nakryiko]

On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>
> In preparation for using sframe to unwind user space stacks, add an
> sframe_find() interface for finding the sframe information associated
> with a given text address.
>
> For performance, use user_read_access_begin() and the corresponding
> unsafe_*() accessors.  Note that use of pr_debug() in uaccess-enabled
> regions would break noinstr validation, so there aren't any debug
> messages yet.  That will be added in a subsequent commit.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  include/linux/sframe.h       |   5 +
>  kernel/unwind/sframe.c       | 295 ++++++++++++++++++++++++++++++++++-
>  kernel/unwind/sframe_debug.h |  35 +++++
>  3 files changed, 331 insertions(+), 4 deletions(-)
>  create mode 100644 kernel/unwind/sframe_debug.h
>

[...]

> +
> +static __always_inline int __read_fde(struct sframe_section *sec,
> +                                     unsigned int fde_num,
> +                                     struct sframe_fde *fde)
> +{
> +       unsigned long fde_addr, ip;
> +
> +       fde_addr = sec->fdes_start + (fde_num * sizeof(struct sframe_fde));
> +       unsafe_copy_from_user(fde, (void __user *)fde_addr,
> +                             sizeof(struct sframe_fde), Efault);
> +
> +       ip = sec->sframe_start + fde->start_addr;
> +       if (ip < sec->text_start || ip > sec->text_end)

ip >= sec->test_end ? ip == sec->test_end doesn't make sense, no?

> +               return -EINVAL;
> +
> +       return 0;
> +
> +Efault:
> +       return -EFAULT;
> +}
> +

[...]

> +static __always_inline int __read_fre(struct sframe_section *sec,
> +                                     struct sframe_fde *fde,
> +                                     unsigned long fre_addr,
> +                                     struct sframe_fre *fre)
> +{
> +       unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
> +       unsigned char fre_type = SFRAME_FUNC_FRE_TYPE(fde->info);
> +       unsigned char offset_count, offset_size;
> +       s32 ip_off, cfa_off, ra_off, fp_off;
> +       unsigned long cur = fre_addr;
> +       unsigned char addr_size;
> +       u8 info;
> +
> +       addr_size = fre_type_to_size(fre_type);
> +       if (!addr_size)
> +               return -EFAULT;
> +
> +       if (fre_addr + addr_size + 1 > sec->fres_end)

nit: isn't this the same as `fre_addr + addr_size >= sec->fres_end` ?

> +               return -EFAULT;
> +
> +       UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);
> +       if (fde_type == SFRAME_FDE_TYPE_PCINC && ip_off > fde->func_size)

is ip_off == fde->func_size allowable?

> +               return -EFAULT;
> +
> +       UNSAFE_GET_USER_INC(info, cur, 1, Efault);
> +       offset_count = SFRAME_FRE_OFFSET_COUNT(info);
> +       offset_size  = offset_size_enum_to_size(SFRAME_FRE_OFFSET_SIZE(info));
> +       if (!offset_count || !offset_size)
> +               return -EFAULT;
> +
> +       if (cur + (offset_count * offset_size) > sec->fres_end)

offset_count * offset_size done in u8 can overflow, no? maybe upcast
to unsigned long or use check_add_overflow?

> +               return -EFAULT;
> +
> +       fre->size = addr_size + 1 + (offset_count * offset_size);
> +
> +       UNSAFE_GET_USER_INC(cfa_off, cur, offset_size, Efault);
> +       offset_count--;
> +
> +       ra_off = sec->ra_off;
> +       if (!ra_off) {
> +               if (!offset_count--)
> +                       return -EFAULT;
> +
> +               UNSAFE_GET_USER_INC(ra_off, cur, offset_size, Efault);
> +       }
> +
> +       fp_off = sec->fp_off;
> +       if (!fp_off && offset_count) {
> +               offset_count--;
> +               UNSAFE_GET_USER_INC(fp_off, cur, offset_size, Efault);
> +       }
> +
> +       if (offset_count)
> +               return -EFAULT;
> +
> +       fre->ip_off             = ip_off;
> +       fre->cfa_off            = cfa_off;
> +       fre->ra_off             = ra_off;
> +       fre->fp_off             = fp_off;
> +       fre->info               = info;
> +
> +       return 0;
> +
> +Efault:
> +       return -EFAULT;
> +}
> +
> +static __always_inline int __find_fre(struct sframe_section *sec,
> +                                     struct sframe_fde *fde, unsigned long ip,
> +                                     struct unwind_user_frame *frame)
> +{
> +       unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
> +       struct sframe_fre *fre, *prev_fre = NULL;
> +       struct sframe_fre fres[2];

you only need prev_fre->ip_off, so why all this `which` and `fres[2]`
business if all you need is prev_fre_off and a bool whether you have
prev_fre at all?

> +       unsigned long fre_addr;
> +       bool which = false;
> +       unsigned int i;
> +       s32 ip_off;
> +
> +       ip_off = (s32)(ip - sec->sframe_start) - fde->start_addr;
> +
> +       if (fde_type == SFRAME_FDE_TYPE_PCMASK)
> +               ip_off %= fde->rep_size;

did you check that fde->rep_size is not zero?

> +
> +       fre_addr = sec->fres_start + fde->fres_off;
> +
> +       for (i = 0; i < fde->fres_num; i++) {

why not binary search? seem more logical to guard against cases with
lots of FREs and be pretty fast in common case anyways.


> +               int ret;
> +
> +               /*
> +                * Alternate between the two fre_addr[] entries for 'fre' and
> +                * 'prev_fre'.
> +                */
> +               fre = which ? fres : fres + 1;
> +               which = !which;
> +
> +               ret = __read_fre(sec, fde, fre_addr, fre);
> +               if (ret)
> +                       return ret;
> +
> +               fre_addr += fre->size;
> +
> +               if (prev_fre && fre->ip_off <= prev_fre->ip_off)
> +                       return -EFAULT;
> +
> +               if (fre->ip_off > ip_off)
> +                       break;
> +
> +               prev_fre = fre;
> +       }
> +
> +       if (!prev_fre)
> +               return -EINVAL;
> +       fre = prev_fre;
> +
> +       frame->cfa_off = fre->cfa_off;
> +       frame->ra_off  = fre->ra_off;
> +       frame->fp_off  = fre->fp_off;
> +       frame->use_fp  = SFRAME_FRE_CFA_BASE_REG_ID(fre->info) == SFRAME_BASE_REG_FP;
> +
> +       return 0;
> +}
> +

[...]