netdev - Re: [PATCH net-next 1/5] net: add TIME_WAIT logic to sk_to_full

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: <CANn89i+PxDFAkc_O9VdP3KgdBsRtpgaTCuYnH11ccLZAzpKMpA@mail.gmail.com>
Date: Fri, 4 Oct 2024 23:56:26 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Martin KaFai Lau <martin.lau@...ux.dev>
Cc: netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next 1/5] net: add TIME_WAIT logic to sk_to_full_sk()

On Fri, Oct 4, 2024 at 9:16 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> TCP will soon attach TIME_WAIT sockets to some ACK and RST.
>
> Make sure sk_to_full_sk() detects this and does not return
> a non full socket.
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
>  include/net/inet_sock.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
> index 394c3b66065e20d34594d6e2a2010c55bb457810..cec093b78151b9a3b95ad4b3672a72b0aa9a8305 100644
> --- a/include/net/inet_sock.h
> +++ b/include/net/inet_sock.h
> @@ -319,8 +319,10 @@ static inline unsigned long inet_cmsg_flags(const struct inet_sock *inet)
>  static inline struct sock *sk_to_full_sk(struct sock *sk)
>  {
>  #ifdef CONFIG_INET
> -       if (sk && sk->sk_state == TCP_NEW_SYN_RECV)
> +       if (sk && READ_ONCE(sk->sk_state) == TCP_NEW_SYN_RECV)
>                 sk = inet_reqsk(sk)->rsk_listener;
> +       if (sk && READ_ONCE(sk->sk_state) == TCP_TIME_WAIT)
> +               sk = NULL;
>  #endif
>         return sk;
>  }

It appears some callers do not check if the return value could be NULL.
I will have to add in v2 :

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index ce91d9b2acb9f8991150ceead4475b130bead438..c3ffb45489a6924c1bc80355e862e243ec195b01
100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -209,7 +209,7 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
        int __ret = 0;                                                         \
        if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {                    \
                typeof(sk) __sk = sk_to_full_sk(sk);                           \
-               if (sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) &&        \
+               if (__sk && sk_fullsock(__sk) && __sk ==
skb_to_full_sk(skb) &&        \
                    cgroup_bpf_sock_enabled(__sk, CGROUP_INET_EGRESS))         \
                        __ret = __cgroup_bpf_run_filter_skb(__sk, skb,         \
                                                      CGROUP_INET_EGRESS); \
diff --git a/net/core/filter.c b/net/core/filter.c
index bd0d08bf76bb8de39ca2ca89cda99a97c9b0a034..533025618b2c06efa31548708f21d9e0ccbdc68d
100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6778,7 +6778,7 @@ __bpf_sk_lookup(struct sk_buff *skb, struct
bpf_sock_tuple *tuple, u32 len,
                /* sk_to_full_sk() may return (sk)->rsk_listener, so
make sure the original sk
                 * sock refcnt is decremented to prevent a request_sock leak.
                 */
-               if (!sk_fullsock(sk2))
+               if (sk2 && !sk_fullsock(sk2))
                        sk2 = NULL;
                if (sk2 != sk) {
                        sock_gen_put(sk);
@@ -6826,7 +6826,7 @@ bpf_sk_lookup(struct sk_buff *skb, struct
bpf_sock_tuple *tuple, u32 len,
                /* sk_to_full_sk() may return (sk)->rsk_listener, so
make sure the original sk
                 * sock refcnt is decremented to prevent a request_sock leak.
                 */
-               if (!sk_fullsock(sk2))
+               if (sk2 && !sk_fullsock(sk2))
                        sk2 = NULL;
                if (sk2 != sk) {
                        sock_gen_put(sk);
@@ -7276,7 +7276,7 @@ BPF_CALL_1(bpf_get_listener_sock, struct sock *, sk)
 {
        sk = sk_to_full_sk(sk);

-       if (sk->sk_state == TCP_LISTEN && sock_flag(sk, SOCK_RCU_FREE))
+       if (sk && sk->sk_state == TCP_LISTEN && sock_flag(sk, SOCK_RCU_FREE))
                return (unsigned long)sk;

        return (unsigned long)NULL;