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]

Date:   Wed, 24 Aug 2016 01:37:54 +0900
From:   Lorenzo Colitti <lorenzo@...gle.com>
To:     David Ahern <dsa@...ulusnetworks.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 Wed, Aug 24, 2016 at 12:02 AM, David Ahern <dsa@...ulusnetworks.com> wrote:
> +int udp_abort(struct sock *sk, int err)
> +{
> +       lock_sock(sk);
> +
> +       sk->sk_err = err;
> +       sk->sk_error_report(sk);
> +       udp_disconnect(sk, 0);

I notice that udp_disconnect does "inet->inet_daddr = 0" but doesn't
touch sk_v6_daddr. I wonder if that's correct. Even if it isn't,
that's likely not specific to this patch, since udpv6_prot specifies
udp_disconnect as its disconnect function pointer.

> +       if (req->sdiag_family == AF_INET)
> +               sk = __udp4_lib_lookup(net,
> +                               req->id.idiag_dst[0], req->id.idiag_dport,
> +                               req->id.idiag_src[0], req->id.idiag_sport,
> +                               req->id.idiag_if, tbl, NULL);
> +#if IS_ENABLED(CONFIG_IPV6)
> +       else if (req->sdiag_family == AF_INET6)
> +               sk = __udp6_lib_lookup(net,

I think you need to check for mapped addresses like Eric added to
inet_diag_find_one_icsk in 7c1306723 .

> +                               (struct in6_addr *)req->id.idiag_dst,
> +                               req->id.idiag_dport,
> +                               (struct in6_addr *)req->id.idiag_src,
> +                               req->id.idiag_sport,
> +                               req->id.idiag_if, tbl, NULL);
> +#endif

If you want to be consistent with what the TCP code does, return
-EINVAL here instead of -ENOENT if an invalid address family was
passed in (e.g., if user passes in AF_INET6 and IPv6 is not compiled
in).

> +       if (sock_diag_check_cookie(sk, req->id.idiag_cookie)) {
> +               sock_put(sk);
> +               return -ENOENT;
> +       }
> +
> +       return sock_diag_destroy(sk, ECONNABORTED);

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.