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

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] [day] [month] [year] [list]

Message-ID: <20220418163733.hmee24hzlt4zyixm@treble>
Date:   Mon, 18 Apr 2022 09:37:33 -0700
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Dmitry Monakhov <dmtrmonakhov@...dex-team.ru>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, mingo@...hat.com,
        Kim Phillips <kim.phillips@....com>,
        Peter Zijlstra <peterz@...radead.org>
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