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

Message-ID: <20221229122731.1603031-1-zhengyejian1@huawei.com>
Date:   Thu, 29 Dec 2022 12:27:31 +0000
From:   Zheng Yejian <zhengyejian1@...wei.com>
To:     <frederic@...nel.org>
CC:     <jiangshanlai@...il.com>, <joel@...lfernandes.org>,
        <josh@...htriplett.org>, <linux-kernel@...r.kernel.org>,
        <mathieu.desnoyers@...icios.com>, <paulmck@...nel.org>,
        <quic_neeraju@...cinc.com>, <rcu@...r.kernel.org>,
        <rostedt@...dmis.org>, <zhengyejian1@...wei.com>,
        <mhiramat@...nel.org>
Subject: Re: [PATCH] rcu: Fix kernel stack overflow caused by kprobe on rcu_irq_enter_check_tick()

On Mon, 5 Dec 2022 14:23:53 +0100, Frederic Weisbecker wrote:
> On Mon, Nov 21, 2022 at 11:57:03AM -0800, Paul E. McKenney wrote:
> > 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?
> 
> Tagging as noinstr doesn't look right as there are functions in
> __rcu_irq_enter_check_tick() that can be traced anyway. Also that
> function has the constraint that it can't be called while RCU is idle
> so it's up to the caller to call instrumentation_begin()/end().

This problem is due to __rcu_irq_enter_check_tick() being kprobe-ed, so
how about adding __rcu_irq_enter_check_tick() into kprobe blacklist by
tagging with NOKPROBE_SYMBOL:

  diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
  index cf34a961821a..41606d3ed083 100644
  --- a/kernel/rcu/tree.c
  +++ b/kernel/rcu/tree.c
  @@ -659,6 +659,7 @@ void __rcu_irq_enter_check_tick(void)
          }
          raw_spin_unlock_rcu_node(rdp->mynode);
   }
  +NOKPROBE_SYMBOL(__rcu_irq_enter_check_tick);
   #endif /* CONFIG_NO_HZ_FULL */

+Cc: <mhiramat@...nel.org>

--
Best regards,
Zheng Yejian