linux-kernel - Re: [PATCH v5 04/17] arm64: stacktrace: Handle frame pointer from different address spaces

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]

Date:   Thu, 21 Jul 2022 10:57:26 +0100
From:   Fuad Tabba <tabba@...gle.com>
To:     Kalesh Singh <kaleshsingh@...gle.com>
Cc:     maz@...nel.org, mark.rutland@....com, broonie@...nel.org,
        madvenka@...ux.microsoft.com, will@...nel.org, qperret@...gle.com,
        james.morse@....com, alexandru.elisei@....com,
        suzuki.poulose@....com, catalin.marinas@....com,
        andreyknvl@...il.com, vincenzo.frascino@....com,
        mhiramat@...nel.org, ast@...nel.org, wangkefeng.wang@...wei.com,
        elver@...gle.com, keirf@...gle.com, yuzenghui@...wei.com,
        ardb@...nel.org, oupton@...gle.com,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH v5 04/17] arm64: stacktrace: Handle frame pointer from
 different address spaces

Hi Kalesh,


On Thu, Jul 21, 2022 at 6:57 AM Kalesh Singh <kaleshsingh@...gle.com> wrote:
>
> The unwinder code is made reusable so that it can be used to
> unwind various types of stacks. One usecase is unwinding the
> nVHE hyp stack from the host (EL1) in non-protected mode. This
> means that the unwinder must be able to translate HYP stack
> addresses to kernel addresses.
>
> Add a callback (stack_trace_translate_fp_fn) to allow specifying
> the translation function.
>
> Signed-off-by: Kalesh Singh <kaleshsingh@...gle.com>
> ---
> Changes in v5:
>   - Fix typo in commit text, per Fuad
>   - Update unwind_next_common() to not have side effects on failure, per Fuad
>   - Use regular comment instead of doc comments, per Fuad
>
>  arch/arm64/include/asm/stacktrace/common.h | 29 +++++++++++++++++++---
>  arch/arm64/kernel/stacktrace.c             |  2 +-
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 0c5cbfdb56b5..e89c8c39858d 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -124,11 +124,25 @@ static inline void unwind_init_common(struct unwind_state *state,
>         state->prev_type = STACK_TYPE_UNKNOWN;
>  }
>
> +/*
> + * stack_trace_translate_fp_fn() - Translates a non-kernel frame pointer to
> + * a kernel address.
> + *
> + * @fp:   the frame pointer to be updated to it's kernel address.

nit: it's -> its

Otherwise

Reviewed-by: Fuad Tabba <tabba@...gle.com>

Cheers,
/fuad





> + * @type: the stack type associated with frame pointer @fp
> + *
> + * Returns true and success and @fp is updated to the corresponding
> + * kernel virtual address; otherwise returns false.
> + */
> +typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp,
> +                                           enum stack_type type);
> +
>  static inline int unwind_next_common(struct unwind_state *state,
> -                                    struct stack_info *info)
> +                                    struct stack_info *info,
> +                                    stack_trace_translate_fp_fn translate_fp)
>  {
> +       unsigned long fp = state->fp, kern_fp = fp;
>         struct task_struct *tsk = state->task;
> -       unsigned long fp = state->fp;
>
>         if (fp & 0x7)
>                 return -EINVAL;
> @@ -139,6 +153,13 @@ static inline int unwind_next_common(struct unwind_state *state,
>         if (test_bit(info->type, state->stacks_done))
>                 return -EINVAL;
>
> +       /*
> +        * If fp is not from the current address space perform the necessary
> +        * translation before dereferencing it to get the next fp.
> +        */
> +       if (translate_fp && !translate_fp(&kern_fp, info->type))
> +               return -EINVAL;
> +
>         /*
>          * As stacks grow downward, any valid record on the same stack must be
>          * at a strictly higher address than the prior record.
> @@ -163,8 +184,8 @@ static inline int unwind_next_common(struct unwind_state *state,
>          * Record this frame record's values and location. The prev_fp and
>          * prev_type are only meaningful to the next unwind_next() invocation.
>          */
> -       state->fp = READ_ONCE(*(unsigned long *)(fp));
> -       state->pc = READ_ONCE(*(unsigned long *)(fp + 8));
> +       state->fp = READ_ONCE(*(unsigned long *)(kern_fp));
> +       state->pc = READ_ONCE(*(unsigned long *)(kern_fp + 8));
>         state->prev_fp = fp;
>         state->prev_type = info->type;
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 834851939364..eef3cf6bf2d7 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -87,7 +87,7 @@ static int notrace unwind_next(struct unwind_state *state)
>         if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
>                 return -ENOENT;
>
> -       err = unwind_next_common(state, &info);
> +       err = unwind_next_common(state, &info, NULL);
>         if (err)
>                 return err;
>
> --
> 2.37.0.170.g444d1eabd0-goog
>