netdev - Re: [PATCH net] netlink: don't send unknown nsid

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: <20170601224454.GG2673@x240.lan>
Date:   Thu, 1 Jun 2017 19:44:54 -0300
From:   Flavio Leitner <fbl@...close.org>
To:     Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net] netlink: don't send unknown nsid

On Thu, Jun 01, 2017 at 10:42:13PM +0200, Nicolas Dichtel wrote:
> Le 01/06/2017 à 19:02, Flavio Leitner a écrit :
> > On Thu, Jun 01, 2017 at 10:00:07AM +0200, Nicolas Dichtel wrote:
> >> The NETLINK_F_LISTEN_ALL_NSID otion enables to listen all netns that have a
> >> nsid assigned into the netns where the netlink socket is opened.
> >> The nsid is sent as metadata to userland, but the existence of this nsid is
> >> checked only for netns that are different from the socket netns. Thus, if
> >> no nsid is assigned to the socket netns, NETNSA_NSID_NOT_ASSIGNED is
> >> reported to the userland. This value is confusing and useless.
> >> After this patch, only valid nsid are sent to userland.
> >>
> >> Reported-by: Flavio Leitner <fbl@...close.org>
> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@...nd.com>
> >> ---
> >>  net/netlink/af_netlink.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> >> index ee841f00a6ec..7586d446d7dc 100644
> >> --- a/net/netlink/af_netlink.c
> >> +++ b/net/netlink/af_netlink.c
> >> @@ -62,6 +62,7 @@
> >>  #include <asm/cacheflush.h>
> >>  #include <linux/hash.h>
> >>  #include <linux/genetlink.h>
> >> +#include <linux/net_namespace.h>
> >>  
> >>  #include <net/net_namespace.h>
> >>  #include <net/sock.h>
> >> @@ -1415,7 +1416,8 @@ static void do_one_broadcast(struct sock *sk,
> >>  		goto out;
> >>  	}
> >>  	NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net);
> >> -	NETLINK_CB(p->skb2).nsid_is_set = true;
> >> +	if (NETLINK_CB(p->skb2).nsid != NETNSA_NSID_NOT_ASSIGNED)
> >> +		NETLINK_CB(p->skb2).nsid_is_set = true;
> >>  	val = netlink_broadcast_deliver(sk, p->skb2);
> >>  	if (val < 0) {
> >>  		netlink_overrun(sk);
> > 
> > If the assumption is that nsid allocation can never fail or that if it
> > does, we can't report to userspace, then the patch is good, but it
> > doesn't sound like a good long term solution.
> > 
> > Let's consider that the allocation of an id fails for whatever reason.
> > I think that should be reported to userspace to allow it to retry, or
> > do something else to handle this situation properly.  Not sending
> > anything means that it's in the same netns as the old kernels did,
> > which is incorrect.
> This is correct, because if nsid allocation fails, no netlink messages from this
> netns are sent to userspace (the check is done at the beginning of
> do_one_broadcast). The only netns allowed to send netlink messages to userspace
> without nsid is the netns of the socket.

I say it's incorrect because of the explanation below.

> > On the other hand, with the original patch, if the socket and the
> > device are in the same netns, we don't need to report any ID.  Previous
> > kernels did that, so we are not breaking anything.  When the netns
> > differs, then we either should report the real ID or an error.
> > 
> I don't understand. With or without my last patch, the kernel sends netlink
> messages of other netns than the netns where the socket is opened, only if an
> nsid is assigned.

"only if an nsid is assigned" that's the issue.

Let me ask this instead: How do you think userspace should behave when
netnsid allocation fails?

-- 
Flavio