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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Jun 2017 22:42:13 +0200
From:   Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:     Flavio Leitner <fbl@...close.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net] netlink: don't send unknown nsid

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.

> 
> 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.


Nicolas

ps: I won't be able to read my emails before monday ;-)