linux-kernel - Re: [PATCH] rcu: Fix kernel stack overflow caused by kprobe on rcu_irq_enter_check

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]

Message-ID: <20221121195703.GO4001@paulmck-ThinkPad-P17-Gen-1>
Date:   Mon, 21 Nov 2022 11:57:03 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Zheng Yejian <zhengyejian1@...wei.com>
Cc:     frederic@...nel.org, quic_neeraju@...cinc.com,
        josh@...htriplett.org, rostedt@...dmis.org,
        mathieu.desnoyers@...icios.com, jiangshanlai@...il.com,
        joel@...lfernandes.org, rcu@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rcu: Fix kernel stack overflow caused by kprobe on
 rcu_irq_enter_check_tick()

On Sat, Nov 19, 2022 at 12:00:49PM +0800, Zheng Yejian wrote:
> Register kprobe on __rcu_irq_enter_check_tick() can cause kernel stack
> overflow [1]. This issue is first found in v5.10 and can be reproduced
> by enabling CONFIG_NO_HZ_FULL and doing like:
>   # cd /sys/kernel/debug/tracing/
>   # echo 'p:mp1 __rcu_irq_enter_check_tick' >> kprobe_events
>   # echo 1 > events/kprobes/enable
> 
> So __rcu_irq_enter_check_tick() should not be kprobed, mark it as noinstr.

Good catch!

I am inclined to queue this, but noticed that one of its callers need
it to be noinstr but that the others do not.

Need noinstr:

o	enter_from_kernel_mode() -> __enter_from_kernel_mode() ->
	rcu_irq_enter_check_tick() -> __rcu_irq_enter_check_tick()

Doesn't need noinstr:

o	ct_nmi_enter() -> rcu_irq_enter_check_tick() ->
	__rcu_irq_enter_check_tick(), courtesy of the call to
	instrumentation_begin() in ct_nmi_enter() that precedes the call
	to rcu_irq_enter_check_tick().

o	irqentry_enter() -> rcu_irq_enter_check_tick() ->
	__rcu_irq_enter_check_tick(), courtesy of the call to
	instrumentation_begin() in irqentry_enter() that precedes the
	call to rcu_irq_enter_check_tick().

Is tagging __rcu_irq_enter_check_tick() with noinstr as
proposed in this patch the right thing to do, or should there
be calls to instrumentation_begin() and instrumentation_end() in
enter_from_kernel_mode()?  Or something else entirely?

Thoughts?

							Thanx, Paul

> [1]
> Insufficient stack space to handle exception!
> Insufficient stack space to handle exception!
> [...]
> Kernel panic - not syncing: kernel stack overflow
> CPU: 3 PID: 34 Comm: migration/3 Not tainted
> 6.1.0-rc5-00884-g84368d882b96 #2
> Hardware name: linux,dummy-virt (DT)
> Stopper: multi_cpu_stop+0x0/0x228 <- __stop_cpus.constprop.0+0xa4/0x100
> Call trace:
>  dump_backtrace+0xf8/0x108
>  show_stack+0x20/0x48
>  dump_stack_lvl+0x68/0x84
>  dump_stack+0x1c/0x38
>  panic+0x214/0x404
>  add_taint+0x0/0xf0
>  panic_bad_stack+0x144/0x160
>  handle_bad_stack+0x38/0x58
>  __bad_stack+0x78/0x7c
>  el1h_64_sync_handler+0x4/0xe8
>  __rcu_irq_enter_check_tick+0x0/0x1b8
>  arm64_enter_el1_dbg.isra.0+0x14/0x20
>  el1_dbg+0x2c/0x90
>  el1h_64_sync_handler+0xcc/0xe8
>  el1h_64_sync+0x64/0x68
>  __rcu_irq_enter_check_tick+0x0/0x1b8
>  arm64_enter_el1_dbg.isra.0+0x14/0x20
>  el1_dbg+0x2c/0x90
>  el1h_64_sync_handler+0xcc/0xe8
>  el1h_64_sync+0x64/0x68
>  __rcu_irq_enter_check_tick+0x0/0x1b8
>  arm64_enter_el1_dbg.isra.0+0x14/0x20
>  el1_dbg+0x2c/0x90
>  el1h_64_sync_handler+0xcc/0xe8
>  el1h_64_sync+0x64/0x68
>  __rcu_irq_enter_check_tick+0x0/0x1b8
>  arm64_enter_el1_dbg.isra.0+0x14/0x20
>  el1_dbg+0x2c/0x90
>  el1h_64_sync_handler+0xcc/0xe8
>  el1h_64_sync+0x64/0x68
>  __rcu_irq_enter_check_tick+0x0/0x1b8
>  arm64_enter_el1_dbg.isra.0+0x14/0x20
>  el1_dbg+0x2c/0x90
>  el1h_64_sync_handler+0xcc/0xe8
>  el1h_64_sync+0x64/0x68
>  __rcu_irq_enter_check_tick+0x0/0x1b8
>  [...]
> 
> Cc: stable@...r.kernel.org
> Fixes: aaf2bc50df1f ("rcu: Abstract out rcu_irq_enter_check_tick() from rcu_nmi_enter()")
> Signed-off-by: Zheng Yejian <zhengyejian1@...wei.com>
> ---
>  kernel/rcu/tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 93416afebd59..68230f02cfb7 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -631,7 +631,7 @@ void rcu_irq_exit_check_preempt(void)
>   * controlled environments, this function allows RCU to get what it
>   * needs without creating otherwise useless interruptions.
>   */
> -void __rcu_irq_enter_check_tick(void)
> +noinstr void __rcu_irq_enter_check_tick(void)
>  {
>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>  
> -- 
> 2.25.1
>