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: <tencent_AB92549E869AAA1F1E5EF653439554675109@qq.com>
Date: Mon, 23 Sep 2024 03:09:56 +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

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.

Best regards,
Jiawei Ye


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ