| 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
| ||
|
Date: Fri, 16 Dec 2022 10:14:40 +0800
From: Qing Zhang <zhangqing@...ngson.cn>
To: Jinyang He <hejinyang@...ngson.cn>,
Huacai Chen <chenhuacai@...nel.org>,
WANG Xuerui <kernel@...0n.name>
Cc: loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org,
Steven Rostedt <rostedt@...dmis.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from
prologue_unwinder
Hi, Jinyang
On 2022/12/16 上午9:40, Jinyang He wrote:
> On 2022-12-15 17:15, Qing Zhang wrote:
>
>> Hi, Jinyang
>>
>> On 2022/12/15 下午12:01, Jinyang He wrote:
>>> The prolugue unwinder rely on symbol info. When PC is not in kernel
>>> text address, it cannot find relative symbol info and it will be broken.
>>> The guess unwinder will be used in this case. And the guess unwinder
>>> codes in prolugue unwinder is redundant. Strip it out and set the
>>> unwinder info in unwind_state.
>>>
>>> Signed-off-by: Jinyang He <hejinyang@...ngson.cn>
>>> ---
>>> arch/loongarch/include/asm/unwind.h | 22 ++++
>>> arch/loongarch/kernel/Makefile | 3 +-
>>> arch/loongarch/kernel/unwind.c | 52 +++++++++
>>> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
>>> arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
>>> 5 files changed, 135 insertions(+), 118 deletions(-)
>>> create mode 100644 arch/loongarch/kernel/unwind.c
>>>
>>> diff --git a/arch/loongarch/include/asm/unwind.h
>>> b/arch/loongarch/include/asm/unwind.h
>>> index 6ece48f0ff77..a16aff1d086a 100644
>>> --- a/arch/loongarch/include/asm/unwind.h
>>> +++ b/arch/loongarch/include/asm/unwind.h
>>> @@ -18,6 +18,8 @@ enum unwinder_type {
>>> UNWINDER_PROLOGUE,
>>> };
>>> +struct unwinder_ops;
>>> +
>>> struct unwind_state {
>>> char type; /* UNWINDER_XXX */
>>> struct stack_info stack_info;
>>> @@ -25,8 +27,22 @@ struct unwind_state {
>>> bool first, error, is_ftrace;
>>> int graph_idx;
>>> unsigned long sp, pc, ra;
>>> + const struct unwinder_ops *ops;
>>> +};
>>> +
>>> +struct unwinder_ops {
>>> + void (*unwind_start)(struct unwind_state *state,
>>> + struct task_struct *task, struct pt_regs *regs);
>>> + bool (*unwind_next_frame)(struct unwind_state *state);
>>> + unsigned long (*unwind_get_return_address)(struct unwind_state
>>> *state);
>>> };
>>> +extern const struct unwinder_ops *default_unwinder;
>>> +extern const struct unwinder_ops unwinder_guess;
>>> +#ifdef CONFIG_UNWINDER_PROLOGUE
>>> +extern const struct unwinder_ops unwinder_prologue;
>>> +#endif
>>> +
>>> void unwind_start(struct unwind_state *state,
>>> struct task_struct *task, struct pt_regs *regs);
>>> bool unwind_next_frame(struct unwind_state *state);
>>> @@ -49,4 +65,10 @@ static inline unsigned long
>>> unwind_graph_addr(struct unwind_state *state,
>>> return ftrace_graph_ret_addr(state->task, &state->graph_idx,
>>> pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
>>> }
>>> +
>>> +static inline void unwind_register_unwinder(struct unwind_state *state,
>>> + const struct unwinder_ops *unwinder)
>>> +{
>>> + state->ops = unwinder;
>>> +}
>>> #endif /* _ASM_UNWIND_H */
>>> diff --git a/arch/loongarch/kernel/Makefile
>>> b/arch/loongarch/kernel/Makefile
>>> index 7ca65195f7f8..cb6029ea3ea9 100644
>>> --- a/arch/loongarch/kernel/Makefile
>>> +++ b/arch/loongarch/kernel/Makefile
>>> @@ -8,7 +8,7 @@ extra-y := vmlinux.lds
>>> obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o
>>> entry.o genex.o \
>>> traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o
>>> switch.o \
>>> elf.o syscall.o signal.o time.o topology.o inst.o
>>> ptrace.o vdso.o \
>>> - alternative.o unaligned.o
>>> + alternative.o unaligned.o unwind.o unwind_guess.o
>>> obj-$(CONFIG_ACPI) += acpi.o
>>> obj-$(CONFIG_EFI) += efi.o
>>> @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
>>> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
>>> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>>> -obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>>> obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
>>> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
>>> diff --git a/arch/loongarch/kernel/unwind.c
>>> b/arch/loongarch/kernel/unwind.c
>>> new file mode 100644
>>> index 000000000000..568c6fe707d1
>>> --- /dev/null
>>> +++ b/arch/loongarch/kernel/unwind.c
>>> @@ -0,0 +1,52 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2022 Loongson Technology Corporation Limited
>>> + */
>>> +#include <asm/unwind.h>
>>> +
>>> +#if defined(CONFIG_UNWINDER_GUESS)
>>> +const struct unwinder_ops *default_unwinder = &unwinder_guess;
>>> +#elif defined(CONFIG_UNWINDER_PROLOGUE)
>>> +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
>>> +#endif
>>> +
>>> +unsigned long unwind_get_return_address(struct unwind_state *state)
>>> +{
>>> + if (!state->ops || unwind_done(state))
>>> + return 0;
>>> + return state->ops->unwind_get_return_address(state);
>>> +}
>>> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>> +
>>> +void unwind_start(struct unwind_state *state, struct task_struct *task,
>>> + struct pt_regs *regs)
>>> +{
>>> + memset(state, 0, sizeof(*state));
>>> + unwind_register_unwinder(state, default_unwinder);
>>> + if (regs) {
>>> + state->sp = regs->regs[3];
>>> + state->pc = regs->csr_era;
>>> + state->ra = regs->regs[1];
>>> + } else if (task == current) {
>>> + state->sp = (unsigned long)__builtin_frame_address(0);
>>> + state->pc = (unsigned long)__builtin_return_address(0);
>>> + state->ra = 0;
>>> + } else {
>>> + state->sp = thread_saved_fp(task);
>>> + state->pc = thread_saved_ra(task);
>>> + state->ra = 0;
>>> + }
>>> + state->task = task;
>>> + get_stack_info(state->sp, state->task, &state->stack_info);
>>> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
here. :)
>>> + state->ops->unwind_start(state, task, regs);
>>> +}
>>> +EXPORT_SYMBOL_GPL(unwind_start);
>>> +
>>> +bool unwind_next_frame(struct unwind_state *state)
>>> +{
>>> + if (!state->ops || unwind_done(state))
>>> + return false;
>>> + return state->ops->unwind_next_frame(state);
>>> +}
>>> +EXPORT_SYMBOL_GPL(unwind_next_frame);
>>> diff --git a/arch/loongarch/kernel/unwind_guess.c
>>> b/arch/loongarch/kernel/unwind_guess.c
>>> index 8ce32c37c587..b7ca2b88ac63 100644
>>> --- a/arch/loongarch/kernel/unwind_guess.c
>>> +++ b/arch/loongarch/kernel/unwind_guess.c
>>> @@ -7,51 +7,23 @@
>>> #include <asm/unwind.h>
>>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>>> +static unsigned long get_return_address(struct unwind_state *state)
>>> {
>>> - if (unwind_done(state))
>>> - return 0;
>>> return state->pc;
>>> }
>>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>> -void unwind_start(struct unwind_state *state, struct task_struct
>>> *task,
>>> +static void start(struct unwind_state *state, struct task_struct *task,
>>> struct pt_regs *regs)
>>> {
>>> - memset(state, 0, sizeof(*state));
>>> -
>>> - if (regs) {
>>> - state->sp = regs->regs[3];
>>> - state->pc = regs->csr_era;
>>> - } else if (task == current) {
>>> - state->sp = (unsigned long)__builtin_frame_address(0);
>>> - state->pc = (unsigned long)__builtin_return_address(0);
>>> - } else {
>>> - state->sp = thread_saved_fp(task);
>>> - state->pc = thread_saved_ra(task);
>>> - }
>>> -
>>> - state->task = task;
>>> - state->first = true;
>>> - state->pc = unwind_graph_add(state, state->pc, state->sp);
>>
>> Do we need to unwind_graph_add again here? unwinder_guess and
>> unwind_prologue have already been done.
>
> Hi, Qing
>
>
> Sorry I don't what meanning here, as here is a delete line.
>
Sorry, It's the unwind_start up here.
>
>>
>>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>> -
>>> if (!unwind_done(state) && !__kernel_text_address(state->pc))
>>> unwind_next_frame(state);
>>> }
>>> -EXPORT_SYMBOL_GPL(unwind_start);
>>> -bool unwind_next_frame(struct unwind_state *state)
>>> +static bool next_frame(struct unwind_state *state)
>>> {
>>> struct stack_info *info = &state->stack_info;
>>> unsigned long addr;
>>> - if (unwind_done(state))
>>> - return false;
>>> -
>>> - if (state->first)
>>> - state->first = false;
>>> -
>>> do {
>>> for (state->sp += sizeof(unsigned long);
>>> state->sp < info->end;
>>> @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
>>> return false;
>>> }
>>> -EXPORT_SYMBOL_GPL(unwind_next_frame);
>>> +
>>> +const struct unwinder_ops unwinder_guess = {
>>> + .unwind_start = start,
>>> + .unwind_next_frame = next_frame,
>>> + .unwind_get_return_address = get_return_address,
>>> +};
>>> diff --git a/arch/loongarch/kernel/unwind_prologue.c
>>> b/arch/loongarch/kernel/unwind_prologue.c
>>> index d464c533c64f..9677e13c4b4c 100644
>>> --- a/arch/loongarch/kernel/unwind_prologue.c
>>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>>> @@ -9,6 +9,8 @@
>>> #include <asm/ptrace.h>
>>> #include <asm/unwind.h>
>>> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>>> +
>>> static inline void unwind_state_fixup(struct unwind_state *state)
>>> {
>>> #ifdef CONFIG_DYNAMIC_FTRACE
>>> @@ -19,31 +21,19 @@ static inline void unwind_state_fixup(struct
>>> unwind_state *state)
>>> #endif
>>> }
>>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>>> +static unsigned long get_return_address(struct unwind_state *state)
>>> {
>>> - if (unwind_done(state))
>>> - return 0;
>>> return state->pc;
>>> }
>>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>> -
>>> -static bool unwind_by_guess(struct unwind_state *state)
>>> -{
>>> - struct stack_info *info = &state->stack_info;
>>> - unsigned long addr;
>>> -
>>> - for (state->sp += sizeof(unsigned long);
>>> - state->sp < info->end;
>>> - state->sp += sizeof(unsigned long)) {
>>> - addr = *(unsigned long *)(state->sp);
>>> - state->pc = unwind_graph_addr(state, addr, state->sp + 8);
>>> - if (__kernel_text_address(state->pc))
>>> - return true;
>>> - }
>>> -
>>> - return false;
>>> -}
>>> +/*
>>> + * LoongArch function prologue like follows,
>>> + * [others instructions not use stack var]
>>> + * addi.d sp, sp, -imm
>>> + * st.d xx, sp, offset <- save callee saved regs and
>>> + * st.d yy, sp, offset save ra if function is nest.
>>> + * [others instructions]
>>> + */
>>> static bool unwind_by_prologue(struct unwind_state *state)
>>> {
>>> long frame_ra = -1;
>>> @@ -89,6 +79,10 @@ static bool unwind_by_prologue(struct unwind_state
>>> *state)
>>> ip++;
>>> }
>>> + /*
>>> + * Not find stack alloc action, PC may be in a leaf function.
>>> Only the
>>> + * first being true is reasonable, otherwise indicate analysis
>>> is broken.
>>> + */
>>> if (!frame_size) {
>>> if (state->first)
>>> goto first;
>>> @@ -106,6 +100,7 @@ static bool unwind_by_prologue(struct
>>> unwind_state *state)
>>> ip++;
>>> }
>>> + /* Not find save $ra action, PC may be in a leaf function,
>>> too. */
>>> if (frame_ra < 0) {
>>> if (state->first) {
>>> state->sp = state->sp + frame_size;
>>> @@ -114,96 +109,63 @@ static bool unwind_by_prologue(struct
>>> unwind_state *state)
>>> return false;
>>> }
>>> - if (state->first)
>>> - state->first = false;
>>> -
>>> state->pc = *(unsigned long *)(state->sp + frame_ra);
>>> state->sp = state->sp + frame_size;
>>> goto out;
>>> first:
>>> - state->first = false;
>>> - if (state->pc == state->ra)
>>> - return false;
>>> -
>>> state->pc = state->ra;
>>> out:
>>> + state->first = false;
>>> unwind_state_fixup(state);
>>> return !!__kernel_text_address(state->pc);
>>> }
>>> -void unwind_start(struct unwind_state *state, struct task_struct
>>> *task,
>>> +static void start(struct unwind_state *state, struct task_struct *task,
>>> struct pt_regs *regs)
>>> {
>>> - memset(state, 0, sizeof(*state));
>>> - state->type = UNWINDER_PROLOGUE;
>>> -
>>> - if (regs) {
>>> - state->sp = regs->regs[3];
>>> - state->pc = regs->csr_era;
>>> - state->ra = regs->regs[1];
>>> - if (!__kernel_text_address(state->pc))
>>> - state->type = UNWINDER_GUESS;
>>> - } else if (task == current) {
>>> - state->sp = (unsigned long)__builtin_frame_address(0);
>>> - state->pc = (unsigned long)__builtin_return_address(0);
>>> - state->ra = 0;
>>> - } else {
>>> - state->sp = thread_saved_fp(task);
>>> - state->pc = thread_saved_ra(task);
>>> - state->ra = 0;
>>> - }
>>> -
>>> - state->task = task;
>>> state->first = true;
>>> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
>>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>> - if (!unwind_done(state) && !__kernel_text_address(state->pc))
>>> - unwind_next_frame(state);
>>> + /*
>>> + * The current PC is not kernel text address, we cannot find its
>>> + * relative symbol. Thus, prologue analysis will be broken. Luckly,
>>> + * we can use the guard unwinder.
>>> + */
>>> + if (!__kernel_text_address(state->pc)) {
>>> + unwind_register_unwinder(state, guard_unwinder);
>>
>> Just add a comment here. Instead of using guard_unwinder, can we still
>> use guess_unwinder?
>
> Yes, and I'll use '&guess_unwinder' in next version. And I'll
> drop unwind type in next version, too.
>
>
> Thanks,
>
> Jinyang
>
Powered by blists - more mailing lists