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]

Date:	Sun, 24 Jul 2016 00:00:13 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Andrey Vagin <avagin@...nvz.org>
Cc:	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

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.

> 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