netdev - Re: [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion

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: <tencent_A01E0A5EA0520995F067A792F49F5C9B1609@qq.com>
Date: Tue, 24 Sep 2024 03:57:10 +0000
From: Jiawei Ye <jiawei.ye@...mail.com>
To: edumazet@...gle.com
Cc: davem@...emloft.net,
	dsahern@...nel.org,
	fw@...len.de,
	jiawei.ye@...mail.com,
	kuba@...nel.org,
	linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org,
	pabeni@...hat.com
Subject: Re: [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control

On 9/23/24 15:36, Eric Dumazet wrote:
> On Mon, Sep 23, 2024 at 5:16 AM Jiawei Ye <jiawei.ye@...mail.com> wrote:
> >
> > Thank you very much for your feedback, Florian Westphal and Eric Dumazet.
> >
> > On 9/20/24 22:11, Eric Dumazet wrote
> > >
> > > On Fri, Sep 20, 2024 at 11:35 AM Florian Westphal <fw@...len.de> wrote:
> > > >
> > > > Jiawei Ye <jiawei.ye@...mail.com> wrote:
> > > > > In the `tcp_assign_congestion_control` function, the `ca->flags` is
> > > > > accessed after the RCU read-side critical section is unlocked. According
> > > > > to RCU usage rules, this is illegal. Reusing this pointer can lead to
> > > > > unpredictable behavior, including accessing memory that has been updated
> > > > > or causing use-after-free issues.
> > > > >
> > > > > This possible bug was identified using a static analysis tool developed
> > > > > by myself, specifically designed to detect RCU-related issues.
> > > > >
> > > > > To resolve this issue, the `rcu_read_unlock` call has been moved to the
> > > > > end of the function.
> > > > >
> > > > > Signed-off-by: Jiawei Ye <jiawei.ye@...mail.com>
> > > > > ---
> > > > > In another part of the file, `tcp_set_congestion_control` calls
> > > > > `tcp_reinit_congestion_control`, ensuring that the congestion control
> > > > > reinitialization process is protected by RCU. The
> > > > > `tcp_reinit_congestion_control` function contains operations almost
> > > > > identical to those in `tcp_assign_congestion_control`, but the former
> > > > > operates under full RCU protection, whereas the latter is only partially
> > > > > protected. The differing protection strategies between the two may
> > > > > warrant further unification.
> > > > > ---
> > > > >  net/ipv4/tcp_cong.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> > > > > index 0306d257fa64..356a59d316e3 100644
> > > > > --- a/net/ipv4/tcp_cong.c
> > > > > +++ b/net/ipv4/tcp_cong.c
> > > > > @@ -223,13 +223,13 @@ void tcp_assign_congestion_control(struct sock *sk)
> > > > >       if (unlikely(!bpf_try_module_get(ca, ca->owner)))
> > > > >               ca = &tcp_reno;
> > > >
> > > > After this, ca either has module refcount incremented, so it can't
> > > > go away anymore, or reno fallback was enabled (always bultin).
> > > >
> > > > >       icsk->icsk_ca_ops = ca;
> > > > > -     rcu_read_unlock();
> > > >
> > > > Therefore its ok to rcu unlock here.
> > >
> > > I agree, there is no bug here.
> > >
> > > Jiawei Ye, I guess your static analysis tool is not ready yet.
> >
> > Yes, the static analysis tool is still under development and debugging.
> >
> > While I've collected and analyzed some relevant RCU commits from
> > associated repositories, designing an effective static detection tool
> > remains challenging.
> >
> > It's quite difficult without the assistance of experienced developers. If
> > you have any suggestions or examples, I would greatly appreciate your
> > help.
> >
> 
> This case is explained in Documentation/RCU/rcuref.rst
> 
> line 61 : search_and_reference()
> 
> For congestion control modules, we call try_module_get() which calls
> atomic_inc_not_zero(&module->refcnt)

Thank you, Eric Dumazet. I will further explore the details in
Documentation/RCU/rcuref.rst and related documents.