linux-kernel - Re: [PATCH 1/5] namespaces: move user_ns into 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: <20160724055414.GA17270@outlook.office365.com>
Date:	Sat, 23 Jul 2016 22:54:15 -0700
From:	Andrew Vagin <avagin@...tuozzo.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
CC:	Andrey Vagin <avagin@...nvz.org>, <linux-kernel@...r.kernel.org>,
	"James Bottomley" <James.Bottomley@...senPartnership.com>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	<linux-api@...r.kernel.org>,
	<containers@...ts.linux-foundation.org>,
	Alexander Viro <viro@...iv.linux.org.uk>, <criu@...nvz.org>,
	<linux-fsdevel@...r.kernel.org>,
	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
Subject: Re: [PATCH 1/5] namespaces: move user_ns into ns_common

On Sun, Jul 24, 2016 at 12:00:13AM -0500, Eric W. Biederman wrote:
> Andrey Vagin <avagin@...nvz.org> writes:
> 
> > Every namespace has a pointer to an user namespace where is was created,
> > but they're all privately embedded in the individual namespace specific
> > structures.
> >
> > Now we are going to add an user-space interface to get an owning user
> > namespace, so it looks reasonable to move it into ns_common.
> >
> > Originally this idea was suggested by James Bottomley.
> 
> I skimmed through this and I really don't like move user_ns into
> ns_common.  If for no other reason that it seems to have guarantteed
> this patchset as written would not apply to my tree.

I am not insisting on this. In a second version, I will add the
get_owner operation to proc_ns_operations.

Thanks!

> 
> > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> > index 8297e5b..a941b44 100644
> > --- a/include/linux/user_namespace.h
> > +++ b/include/linux/user_namespace.h
> > @@ -27,11 +27,15 @@ struct user_namespace {
> >  	struct uid_gid_map	gid_map;
> >  	struct uid_gid_map	projid_map;
> >  	atomic_t		count;
> > -	struct user_namespace	*parent;
> >  	int			level;
> >  	kuid_t			owner;
> >  	kgid_t			group;
> > -	struct ns_common	ns;
> > +
> > +	/* ->ns.user_ns and ->parent are synonyms */
> > +	union {
> > +		struct user_namespace	*parent;
> > +		struct ns_common	ns;
> > +	};
> >  	unsigned long		flags;
> >  
> >  	/* Register of per-UID persistent keyrings for this namespace */
> 
> This union is unmaintainable.  It is very easy for someone to change
> ns_common and accidentially break this.  The C standard does not
> allow data to be accessed as either one union member or the other.
> Which means semantically this code relies on undefined behavior, and
> the compiler can do anything in this case and gcc has sometimes been
> known to use that allowance.
> 
> Eric