netdev - Re: [PATCH nf-next] netns: add and use net_ns

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: <20170602095342.GC13569@breakpoint.cc>
Date:   Fri, 2 Jun 2017 11:53:42 +0200
From:   Florian Westphal <fw@...len.de>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     'Florian Westphal' <fw@...len.de>,
        "netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [PATCH nf-next] netns: add and use net_ns_barrier

David Laight <David.Laight@...LAB.COM> wrote:
> From: Florian Westphal
> > Sent: 30 May 2017 10:38
> > 
> > Quoting Joe Stringer:
> >   If a user loads nf_conntrack_ftp, sends FTP traffic through a network
> >   namespace, destroys that namespace then unloads the FTP helper module,
> >   then the kernel will crash.
> > 
> > Events that lead to the crash:
> > 1. conntrack is created with ftp helper in netns x
> > 2. This netns is destroyed
> > 3. netns destruction is scheduled
> > 4. netns destruction wq starts, removes netns from global list
> > 5. ftp helper is unloaded, which resets all helpers of the conntracks
> > via for_each_net()
> > 
> > but because netns is already gone from list the for_each_net() loop
> > doesn't include it, therefore all of these conntracks are unaffected.
> > 
> > 6. helper module unload finishes
> > 7. netns wq invokes destructor for rmmod'ed helper
> > 
> ...
> >  void
> >  nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
> > @@ -1734,6 +1736,13 @@ nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
> >  	}
> >  	rtnl_unlock();
> > 
> > +	/* Need to wait for netns cleanup worker to finish, if its
> > +	 * running -- it might have deleted a net namespace from
> > +	 * the global list, so our __nf_ct_unconfirmed_destroy() might
> > +	 * not have affected all namespaces.
> > +	 */
> > +	net_ns_barrier();
> > +
> 
> A problem I see is that nothing obvious guarantees that the cleanup worker
> has actually started.

If it hasn't even started, the earlier for_each_net() has seen all
net namespaces and we managed to clear helper extensions of all conntracks.

Same in case it has finished already: netns cleanup work queue has
free'd all the affected conntracks we might have missed.

We only are in trouble if netns work queue is running concurrently:
netns cleanup first removes net namespaces from the global list,
so nf_ct_iterate_destroy might have missed these.