2022-04-18 11:23:39

by Dmitry Monakhov

Subject: [PATCH] x86/unwind/orc: recheck address range after stack info was updated V2

get_stack_info() detects stack type only by begin address, so we must
check that address range in question is fully covered by detected stack

Otherwise following crash is possible:
-> unwind_next_frame
case ORC_TYPE_REGS:
if (!deref_stack_regs(state, sp, &state->ip, &state->sp))
-> deref_stack_regs
-> stack_access_ok <- addr is ok, but addr+len-1 is not, exit with success
*ip = READ_ONCE_NOCHECK(regs->ip); <- Here we hit stack guard fault

Original OOPS log:
BUG: stack guard page was hit at 000000000dd984a2 (stack is 00000000d1caafca..00000000613712f0)
kernel stack overflow (page fault): 0000 [#1] SMP NOPTI
CPU: 93 PID: 23787 Comm: context_switch1 Not tainted 5.4.145 #1
RIP: 0010:unwind_next_frame
Call Trace:
<NMI>
perf_callchain_kernel
..
get_perf_callchain
perf_callchain
perf_prepare_sample
perf_event_output_forward
...
__perf_event_overflow
perf_ibs_handle_irq
....
perf_ibs_nmi_handler
nmi_handle
default_do_nmi
do_nmi
end_repeat_nmi

Changes since v1:
- Do not call on_stack() twice for valid range.

Signed-off-by: Dmitry Monakhov <[email protected]>

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 794fdef2501a..38185aedf7d1 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -339,11 +339,11 @@ static bool stack_access_ok(struct unwind_state *state, unsigned long _addr,
struct stack_info *info = &state->stack_info;
void *addr = (void *)_addr;

- if (!on_stack(info, addr, len) &&
- (get_stack_info(addr, state->task, info, &state->stack_mask)))
- return false;
+ if (on_stack(info, addr, len))
+ return true;

- return true;
+ return !get_stack_info(addr, state->task, info, &state->stack_mask) &&
+ on_stack(info, addr, len);
}

static bool deref_stack_reg(struct unwind_state *state, unsigned long addr,
--
2.7.4

2022-04-19 14:20:03

by Josh Poimboeuf

Subject: Re: [PATCH] x86/unwind/orc: recheck address range after stack info was updated V2

In the subject, the patch version should be in the [PATCH] field which
gets discarded by git. Also "recheck" should be capitalized:

[PATCH v2] x86/unwind/orc: Recheck address range after stack info was updated

> get_stack_info() detects stack type only by begin address, so we must
> check that address range in question is fully covered by detected stack

Try to use good grammar/punctuation. This sentence doesn't even have a
period.

And, it should mention the fact that the root cause actually seems to be
a bug in the IBS handling code which is passing invalid regs to the
unwinder.

>
> Otherwise following crash is possible:
> -> unwind_next_frame
> case ORC_TYPE_REGS:
> if (!deref_stack_regs(state, sp, &state->ip, &state->sp))
> -> deref_stack_regs
> -> stack_access_ok <- addr is ok, but addr+len-1 is not, exit with success
> *ip = READ_ONCE_NOCHECK(regs->ip); <- Here we hit stack guard fault
>
> Original OOPS log:
> BUG: stack guard page was hit at 000000000dd984a2 (stack is 00000000d1caafca..00000000613712f0)
> kernel stack overflow (page fault): 0000 [#1] SMP NOPTI
> CPU: 93 PID: 23787 Comm: context_switch1 Not tainted 5.4.145 #1
> RIP: 0010:unwind_next_frame
> Call Trace:
> <NMI>
> perf_callchain_kernel
> ..
> get_perf_callchain
> perf_callchain
> perf_prepare_sample
> perf_event_output_forward
> ...
> __perf_event_overflow
> perf_ibs_handle_irq
> ....
> perf_ibs_nmi_handler

This is definitely more readable than before, though I think all the
'...' can be removed, since you only trimmed the '?' entries which are
just bread crumbs on the stack and not actually part of the call path.

> nmi_handle
> default_do_nmi
> do_nmi
> end_repeat_nmi
>
> Changes since v1:
> - Do not call on_stack() twice for valid range.

This changelog shouldn't be a part of the patch description itself.
Instead it should be below the "---" line.

Also, a diffstat would be helpful.

See "The canonical patch format" in
Documentation/process/submitting-patches.rst.

> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 794fdef2501a..38185aedf7d1 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -339,11 +339,11 @@ static bool stack_access_ok(struct unwind_state *state, unsigned long _addr,
> struct stack_info *info = &state->stack_info;
> void *addr = (void *)_addr;
>
> - if (!on_stack(info, addr, len) &&
> - (get_stack_info(addr, state->task, info, &state->stack_mask)))
> - return false;
> + if (on_stack(info, addr, len))
> + return true;
>
> - return true;
> + return !get_stack_info(addr, state->task, info, &state->stack_mask) &&
> + on_stack(info, addr, len);

The change itself looks ok.

--
Josh