netdev - Re: [PATCH net] sock_diag: invert socket destroy broadcast check

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: <8737o2721q.fsf@x220.int.ebiederm.org>
Date:	Fri, 24 Jun 2016 15:41:05 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, kraig@...gle.com,
	eric.dumazet@...il.com, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] sock_diag: invert socket destroy broadcast check

Willem de Bruijn <willemdebruijn.kernel@...il.com> writes:

> From: Willem de Bruijn <willemb@...gle.com>
>
> Socket destruction is only broadcast for a socket sk if a diag
> listener is registered and sk is not a kernel socket.
>
> Invert the test to not even check for listeners for kernel sockets.
>
> The sock_diag_has_destroy_listeners invocation dereferences
> sock_net(sk), which for kernel sockets can be invalid as they do not
> take a reference on the network namespace.

No.  That isn't so.  A kernel socket for a network namespace must be
destroyed in the network namespace teardown.

Therefore sock_net(sk) should always be valid.  If sock_net(sk) is not
valid something very broken is going on.

Eric

> Fixes: b922622ec6ef ("sock_diag: don't broadcast kernel sockets")
> Fixes: 26abe14379f8 ("net: Modify sk_alloc to not reference count the
>                       netns of kernel sockets.")
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
>
> ---
>
> This patch fixes this immediate codepath. A broader issue of live
> kernel sockets pointing to deleted namespaces may persist.
>
> I observed skbs queued on a device queue in another namespace from
> a kernel socket in SOCK_DEAD state with dangling sock_net(sk). Socket
> refcnt is zero, but sk_wmem_alloc is not. (This was on an older
> kernel, have not yet tried to reproduce on net).
>
> It seems that we may need to reintroduce namespace reference counting
> for kernel sockets (with two-stage deletion to avoid the circular
> reference), scrub packets between namespaces, or reparent kernel
> sockets to init_net on namespace destruction.
> ---
>  net/core/sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 08bf97e..ba082b4 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1473,7 +1473,7 @@ void sk_destruct(struct sock *sk)
>  
>  static void __sk_free(struct sock *sk)
>  {
> -	if (unlikely(sock_diag_has_destroy_listeners(sk) && sk->sk_net_refcnt))
> +	if (unlikely(sk->sk_net_refcnt && sock_diag_has_destroy_listeners(sk)))
>  		sock_diag_broadcast_destroy(sk);
>  	else
>  		sk_destruct(sk);