netdev - Re: [PATCH v3 net-next] net: diag: support SOCK

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: <e0d15dd5-28d1-6df5-c62c-1cc272a6a164@cumulusnetworks.com>
Date:   Tue, 23 Aug 2016 11:16:57 -0600
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Lorenzo Colitti <lorenzo@...gle.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH v3 net-next] net: diag: support SOCK_DESTROY for UDP
 sockets

On 8/23/16 11:13 AM, Lorenzo Colitti wrote:
> On Wed, Aug 24, 2016 at 2:02 AM, David Ahern <dsa@...ulusnetworks.com> wrote:
>>> Looking at the code again, it seems that there's a bug in
>>> sock_diag_destroy. If the destroy operation does not occur (e.g., if
>>> sock_diag_destroy returns EPERM, or the protocol doesn't support
>>> destroy), then it doesn't release the refcount. This affects the TCP
>>> code as well and as such is my fault, not yours. The most obvious way
>>> to fix this might be to call sock_gen_put in sock_diag_destroy.
>>
>> sock_gen_put seems specific to tcp which is why I used sock_put here.
> 
> I thought sock_gen_put can be used on UDP as well, but it does seem a
> bit counterintuitive.
> 
>> Perhaps better to have the callers of sock_diag_destroy handle the refcnt? This
>> function took it; it should release it success or fail. Same for tcp.
> 
> So you'd remove the sock_put and sock_gen_put calls from tcp_abort and
> just add one sock_gen_put in tcp_diag_destroy? That does seem simpler.
> 

untested and mangled on a copy and paste but the intent is:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index f1a9a0a8a1f3..54c5d0a8f6e9 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3176,7 +3176,6 @@ int tcp_abort(struct sock *sk, int err)
                        local_bh_enable();
                        return 0;
                }
-               sock_gen_put(sk);
                return -EOPNOTSUPP;
        }

@@ -3205,7 +3204,6 @@ int tcp_abort(struct sock *sk, int err)
        bh_unlock_sock(sk);
        local_bh_enable();
        release_sock(sk);
-       sock_put(sk);
        return 0;
 }
 EXPORT_SYMBOL_GPL(tcp_abort);
diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index 4d610934fb39..a748c74aa8b7 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -54,11 +54,16 @@ static int tcp_diag_destroy(struct sk_buff *in_skb,
 {
        struct net *net = sock_net(in_skb->sk);
        struct sock *sk = inet_diag_find_one_icsk(net, &tcp_hashinfo, req);
+       int err;

        if (IS_ERR(sk))
                return PTR_ERR(sk);

-       return sock_diag_destroy(sk, ECONNABORTED);
+       err = sock_diag_destroy(sk, ECONNABORTED);
+
+       sock_gen_put(sk);     <---- if (!sk_fullsock(sk)) needed here? seems like this works for all TCP
+
+       return err;
 }
 #endif