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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANn89iLvFuuihCtt9PME2uS1WJATnf5fKjDToa1WzVnRzHnPfg@mail.gmail.com>
Date: Fri, 8 Mar 2024 17:35:37 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Allison Henderson <allison.henderson@...cle.com>, 
	Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org, linux-rdma@...r.kernel.org, 
	rds-devel@....oracle.com
Subject: Re: [PATCH v4 net 1/2] tcp: Fix use-after-free in inet_twsk_purge().

On Fri, Mar 8, 2024 at 12:22 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> Commit 740ea3c4a0b2 ("tcp: Clean up kernel listener's reqsk in
> inet_twsk_purge()") added changes in inet_twsk_purge() to purge
> reqsk in per-netns ehash during netns dismantle.
>
> inet_csk_reqsk_queue_drop_and_put() will remove reqsk from per-netns
> ehash, but the iteration uses sk_nulls_for_each_rcu(), which is not
> safe.  After removing reqsk, we need to restart iteration.
>
> Also, we need to use refcount_inc_not_zero() to check if reqsk is
> freed by its timer.
>
> Fixes: 740ea3c4a0b2 ("tcp: Clean up kernel listener's reqsk in inet_twsk_purge()")
> Reported-by: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
>  net/ipv4/inet_timewait_sock.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> index 5befa4de5b24..c81f83893fc7 100644
> --- a/net/ipv4/inet_timewait_sock.c
> +++ b/net/ipv4/inet_timewait_sock.c
> @@ -278,18 +278,32 @@ void inet_twsk_purge(struct inet_hashinfo *hashinfo, int family)
>  restart:
>                 sk_nulls_for_each_rcu(sk, node, &head->chain) {
>                         if (sk->sk_state != TCP_TIME_WAIT) {
> +                               struct request_sock *req;
> +
> +                               if (likely(sk->sk_state != TCP_NEW_SYN_RECV))
> +                                       continue;
> +
>                                 /* A kernel listener socket might not hold refcnt for net,
>                                  * so reqsk_timer_handler() could be fired after net is
>                                  * freed.  Userspace listener and reqsk never exist here.
>                                  */
> -                               if (unlikely(sk->sk_state == TCP_NEW_SYN_RECV &&
> -                                            hashinfo->pernet)) {
> -                                       struct request_sock *req = inet_reqsk(sk);
>
> -                                       inet_csk_reqsk_queue_drop_and_put(req->rsk_listener, req);
> +                               if (sk->sk_family != family ||
> +                                   refcount_read(&sock_net(sk)->ns.count))
> +                                       continue;
> +
> +                               req = inet_reqsk(sk);
> +                               if (unlikely(!refcount_inc_not_zero(&req->rsk_refcnt)))
> +                                       continue;
> +
> +                               if (unlikely(sk->sk_family != family ||
> +                                            refcount_read(&sock_net(sk)->ns.count))) {
> +                                       reqsk_put(req);
> +                                       continue;
>                                 }
>
> -                               continue;
> +                               inet_csk_reqsk_queue_drop_and_put(req->rsk_listener, req);

> +                               goto restart;
>                         }
>
>                         tw = inet_twsk(sk);
> --
> 2.30.2
>

Unfortunately  inet_csk_reqsk_queue_drop_and_put() also needs the
local_bh_disable() part.
Presumably this code never met LOCKDEP :/

What about factoring the code like this ?
(Untested patch, only compiled)

diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index 5befa4de5b2416281ad2795713a70d0fd847b0b2..21a9b267fc8b843f8320767fb705de32f2c7b7b0
100644
--- a/net/ipv4/inet_timewait_sock.c
+++ b/net/ipv4/inet_timewait_sock.c
@@ -265,10 +265,9 @@ EXPORT_SYMBOL_GPL(__inet_twsk_schedule);

 void inet_twsk_purge(struct inet_hashinfo *hashinfo, int family)
 {
-       struct inet_timewait_sock *tw;
-       struct sock *sk;
        struct hlist_nulls_node *node;
        unsigned int slot;
+       struct sock *sk;

        for (slot = 0; slot <= hashinfo->ehash_mask; slot++) {
                struct inet_ehash_bucket *head = &hashinfo->ehash[slot];
@@ -277,38 +276,35 @@ void inet_twsk_purge(struct inet_hashinfo
*hashinfo, int family)
                rcu_read_lock();
 restart:
                sk_nulls_for_each_rcu(sk, node, &head->chain) {
-                       if (sk->sk_state != TCP_TIME_WAIT) {
-                               /* A kernel listener socket might not
hold refcnt for net,
-                                * so reqsk_timer_handler() could be
fired after net is
-                                * freed.  Userspace listener and
reqsk never exist here.
-                                */
-                               if (unlikely(sk->sk_state == TCP_NEW_SYN_RECV &&
-                                            hashinfo->pernet)) {
-                                       struct request_sock *req =
inet_reqsk(sk);
-
-
inet_csk_reqsk_queue_drop_and_put(req->rsk_listener, req);
-                               }
+                       int state = inet_sk_state_load(sk);

+                       if ((1 << state) & ~(TCPF_TIME_WAIT |
+                                            TCPF_NEW_SYN_RECV))
                                continue;
-                       }

-                       tw = inet_twsk(sk);
-                       if ((tw->tw_family != family) ||
-                               refcount_read(&twsk_net(tw)->ns.count))
+                       if ((sk->sk_family != family) ||
+                               refcount_read(&sock_net(sk)->ns.count))
                                continue;

-                       if (unlikely(!refcount_inc_not_zero(&tw->tw_refcnt)))
+                       if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
                                continue;

-                       if (unlikely((tw->tw_family != family) ||
-                                    refcount_read(&twsk_net(tw)->ns.count))) {
-                               inet_twsk_put(tw);
+                       if (unlikely((sk->sk_family != family) ||
+                                    refcount_read(&sock_net(sk)->ns.count))) {
+                               sock_gen_put(sk);
                                goto restart;
                        }

                        rcu_read_unlock();
                        local_bh_disable();
-                       inet_twsk_deschedule_put(tw);
+                       if (state == TCP_TIME_WAIT) {
+                               inet_twsk_deschedule_put(inet_twsk(sk));
+                       } else {
+                               struct request_sock *req = inet_reqsk(sk);
+
+
inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
+                                                                 req);
+                       }
                        local_bh_enable();
                        goto restart_rcu;
                }

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ