Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430065 > unrolled thread

Re: [PATCH] namespaces: add transparent user namespaces

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] namespaces: add transparent user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-06-23 21:10 +0200
    Re: [PATCH] namespaces: add transparent user namespaces Jann Horn <jannh@google.com> - 2016-06-23 22:10 +0200
      Re: [PATCH] namespaces: add transparent user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-06-23 22:20 +0200
      [PATCH v2 1/2] namespaces: don't use from_k*id_munged() with init_user_ns Jann Horn <jannh@google.com> - 2016-06-25 02:30 +0200
        Re: [PATCH v2 2/2] namespaces: add transparent user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-06-27 17:30 +0200

#1430065 — Re: [PATCH] namespaces: add transparent user namespaces

Jann Horn <jannh@google.com> writes:

> This allows the admin of a user namespace to mark the namespace as
> transparent. All other namespaces, by default, are opaque.
>
> While the current behavior of user namespaces is appropriate for use in
> containers, there are many programs that only use user namespaces because
> doing so enables them to do other things (e.g. unsharing the mount or
> network namespace) that require namespaced capabilities. For them, the
> inability to see the real UIDs and GIDs of things from inside the user
> namespace can be very annoying.
>
> In a transparent namespace, all UIDs and GIDs that are mapped into its
> first opaque ancestor are visible and are not remapped. This means that if
> a process e.g. stat()s the real root directory in a namespace, it will
> still see it as owned by UID 0.
>
> Traditionally, any UID or GID that was visible in a user namespace was also
> mapped into the namespace, giving the namespace admin full access to it.
> This patch introduces a distinction: In a transparent namespace, UIDs and
> GIDs can be visible without being mapped. Non-mapped, visible UIDs can be
> passed from the kernel to userspace, but userspace can't send them back to
> the kernel. In order to be able to fully use specific UIDs/GIDs and gain
> privileges over them, mappings need to be set up in the usual way -
> however, to avoid aliasing problems, only identity mappings are permitted.
>
> I have gone through all callers of from_kuid() and from_kgid(), and as far
> as I can tell, kuid_has_mapping() and kgid_has_mapping() were the only
> functions that used them for privilege checks. (The keys subsystem uses
> them in an insecure way, and that issue has been known for a while, but my
> patch doesn't make that any more vulnerable than it already is.

Perhaps it has been known for a while but no one has stopped and
mentioned it to me.  What questionable thing is the keys subsystem
doing?

>)

This is a bigish change in semantics and I am going to have to digest
this before I can give this an ok.

Quite frankly at the base it scares me.


If this is just about presentation and allowing some information from
the parent user namespace I would be much happier if it was not
from_kuid but that you modified, but if you instead you had a function
say from_kuid_transparent, that performed the transformation you need
and was only used in those places it is safe.

I think I could reason about that.

As your patchset sits I can not reason about the change in semantics,
because without a large grep of the source I don't know what you are
changing.

And you are dramatically changing the semantics the semantics of
from_kuid to the point I do believe we need to inspepect all of the call
sites.  As such I really don't think it makes sense to reuse the
existing name for your new semantics.

Eric

> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  fs/proc/base.c                 |  28 ++++++--
>  include/linux/uidgid.h         |  16 ++++-
>  include/linux/user_namespace.h |   4 ++
>  kernel/user.c                  |   1 +
>  kernel/user_namespace.c        | 152 +++++++++++++++++++++++++++++++++++++++--
>  5 files changed, 191 insertions(+), 10 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..c521c51 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2744,7 +2744,8 @@ static const struct file_operations proc_projid_map_operations = {
>  	.release	= proc_id_map_release,
>  };
>  
> -static int proc_setgroups_open(struct inode *inode, struct file *file)
> +static int proc_nsadmin_open(struct inode *inode, struct file *file,
> +	int (*show)(struct seq_file *, void *))
>  {
>  	struct user_namespace *ns = NULL;
>  	struct task_struct *task;
> @@ -2767,7 +2768,7 @@ static int proc_setgroups_open(struct inode *inode, struct file *file)
>  			goto err_put_ns;
>  	}
>  
> -	ret = single_open(file, &proc_setgroups_show, ns);
> +	ret = single_open(file, show, ns);
>  	if (ret)
>  		goto err_put_ns;
>  
> @@ -2778,7 +2779,7 @@ err:
>  	return ret;
>  }
>  
> -static int proc_setgroups_release(struct inode *inode, struct file *file)
> +static int proc_nsadmin_release(struct inode *inode, struct file *file)
>  {
>  	struct seq_file *seq = file->private_data;
>  	struct user_namespace *ns = seq->private;
> @@ -2787,12 +2788,30 @@ static int proc_setgroups_release(struct inode *inode, struct file *file)
>  	return ret;
>  }
>  
> +static int proc_setgroups_open(struct inode *inode, struct file *file)
> +{
> +	return proc_nsadmin_open(inode, file, &proc_setgroups_show);
> +}
> +
>  static const struct file_operations proc_setgroups_operations = {
>  	.open		= proc_setgroups_open,
>  	.write		= proc_setgroups_write,
>  	.read		= seq_read,
>  	.llseek		= seq_lseek,
> -	.release	= proc_setgroups_release,
> +	.release	= proc_nsadmin_release,
> +};
> +
> +static int proc_transparent_open(struct inode *inode, struct file *file)
> +{
> +	return proc_nsadmin_open(inode, file, &proc_transparent_show);
> +}
> +
> +static const struct file_operations proc_transparent_operations = {
> +	.open		= proc_transparent_open,
> +	.write		= proc_transparent_write,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= proc_nsadmin_release,
>  };
>  #endif /* CONFIG_USER_NS */
>  
> @@ -2901,6 +2920,7 @@ static const struct pid_entry tgid_base_stuff[] = {
>  	REG("gid_map",    S_IRUGO|S_IWUSR, proc_gid_map_operations),
>  	REG("projid_map", S_IRUGO|S_IWUSR, proc_projid_map_operations),
>  	REG("setgroups",  S_IRUGO|S_IWUSR, proc_setgroups_operations),
> +	REG("transparent", S_IRUGO|S_IWUSR, proc_transparent_operations),
>  #endif
>  #ifdef CONFIG_CHECKPOINT_RESTORE
>  	REG("timers",	  S_IRUGO, proc_timers_operations),
> diff --git a/include/linux/uidgid.h b/include/linux/uidgid.h
> index 0383552..2908d40 100644
> --- a/include/linux/uidgid.h
> +++ b/include/linux/uidgid.h
> @@ -124,17 +124,19 @@ extern kgid_t make_kgid(struct user_namespace *from, gid_t gid);
>  
>  extern uid_t from_kuid(struct user_namespace *to, kuid_t uid);
>  extern gid_t from_kgid(struct user_namespace *to, kgid_t gid);
> +extern uid_t from_kuid_opaque(struct user_namespace *to, kuid_t uid);
> +extern gid_t from_kgid_opaque(struct user_namespace *to, kgid_t gid);
>  extern uid_t from_kuid_munged(struct user_namespace *to, kuid_t uid);
>  extern gid_t from_kgid_munged(struct user_namespace *to, kgid_t gid);
>  
>  static inline bool kuid_has_mapping(struct user_namespace *ns, kuid_t uid)
>  {
> -	return from_kuid(ns, uid) != (uid_t) -1;
> +	return from_kuid_opaque(ns, uid) != (uid_t) -1;
>  }
>  
>  static inline bool kgid_has_mapping(struct user_namespace *ns, kgid_t gid)
>  {
> -	return from_kgid(ns, gid) != (gid_t) -1;
> +	return from_kgid_opaque(ns, gid) != (gid_t) -1;
>  }
>  
>  #else
> @@ -159,6 +161,16 @@ static inline gid_t from_kgid(struct user_namespace *to, kgid_t kgid)
>  	return __kgid_val(kgid);
>  }
>  
> +static inline uid_t from_kuid_opaque(struct user_namespace *to, kuid_t kuid)
> +{
> +	return __kuid_val(kuid);
> +}
> +
> +static inline gid_t from_kgid_opaque(struct user_namespace *to, kgid_t kgid)
> +{
> +	return __kgid_val(kgid);
> +}
> +
>  static inline uid_t from_kuid_munged(struct user_namespace *to, kuid_t kuid)
>  {
>  	uid_t uid = from_kuid(to, kuid);
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index 8297e5b..18291ac 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -28,6 +28,8 @@ struct user_namespace {
>  	struct uid_gid_map	projid_map;
>  	atomic_t		count;
>  	struct user_namespace	*parent;
> +	/* self for normal ns; first opaque parent for transparent ns */
> +	struct user_namespace	*opaque;
>  	int			level;
>  	kuid_t			owner;
>  	kgid_t			group;
> @@ -71,6 +73,8 @@ extern ssize_t proc_gid_map_write(struct file *, const char __user *, size_t, lo
>  extern ssize_t proc_projid_map_write(struct file *, const char __user *, size_t, loff_t *);
>  extern ssize_t proc_setgroups_write(struct file *, const char __user *, size_t, loff_t *);
>  extern int proc_setgroups_show(struct seq_file *m, void *v);
> +extern ssize_t proc_transparent_write(struct file *, const char __user *, size_t, loff_t *);
> +extern int proc_transparent_show(struct seq_file *m, void *v);
>  extern bool userns_may_setgroups(const struct user_namespace *ns);
>  #else
>  
> diff --git a/kernel/user.c b/kernel/user.c
> index b069ccb..e1fd9e5 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -48,6 +48,7 @@ struct user_namespace init_user_ns = {
>  		},
>  	},
>  	.count = ATOMIC_INIT(3),
> +	.opaque = &init_user_ns,
>  	.owner = GLOBAL_ROOT_UID,
>  	.group = GLOBAL_ROOT_GID,
>  	.ns.inum = PROC_USER_INIT_INO,
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 9bafc21..da329a1 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -98,6 +98,7 @@ int create_user_ns(struct cred *new)
>  	atomic_set(&ns->count, 1);
>  	/* Leave the new->user_ns reference with the new user namespace. */
>  	ns->parent = parent_ns;
> +	ns->opaque = ns;
>  	ns->level = parent_ns->level + 1;
>  	ns->owner = owner;
>  	ns->group = group;
> @@ -251,18 +252,46 @@ EXPORT_SYMBOL(make_kuid);
>   *	Map @kuid into the user-namespace specified by @targ and
>   *	return the resulting uid.
>   *
> + *	This function is *not* appropriate for security checks because
> + *	if @targ is transparent, the mappings of an ancestor namespace
> + *	are used. If @targ isn't &init_user_ns and you intend to do
> + *	anything with the result apart from returning it to a process
> + *	in @targ, you might want to use from_kuid_opaque() instead.
> + *
>   *	There is always a mapping into the initial user_namespace.
>   *
> - *	If @kuid has no mapping in @targ (uid_t)-1 is returned.
> + *	If @kuid is not visible in @targ (uid_t)-1 is returned.
>   */
>  uid_t from_kuid(struct user_namespace *targ, kuid_t kuid)
>  {
>  	/* Map the uid from a global kernel uid */
> -	return map_id_up(&targ->uid_map, __kuid_val(kuid));
> +	struct user_namespace *opaque = READ_ONCE(targ->opaque);
> +
> +	return map_id_up(&opaque->uid_map, __kuid_val(kuid));
>  }
>  EXPORT_SYMBOL(from_kuid);
>  
>  /**
> + *	from_kuid_opaque - Create a uid from a kuid user-namespace pair.
> + *	@targ: The user namespace we want a uid in.
> + *	@kuid: The kernel internal uid to start with.
> + *
> + *	Map @kuid into the user-namespace specified by @targ and
> + *	return the resulting uid. This ignores transparent user
> + *	namespaces and is therefore appropriate for security checks.
> + *
> + *	There is always a mapping into the initial user_namespace.
> + *
> + *	If @kuid has no mapping in @targ (uid_t)-1 is returned.
> + */
> +uid_t from_kuid_opaque(struct user_namespace *targ, kuid_t kuid)
> +{
> +	/* Map the uid from a global kernel uid */
> +	return map_id_up(&targ->uid_map, __kuid_val(kuid));
> +}
> +EXPORT_SYMBOL(from_kuid_opaque);
> +
> +/**
>   *	from_kuid_munged - Create a uid from a kuid user-namespace pair.
>   *	@targ: The user namespace we want a uid in.
>   *	@kuid: The kernel internal uid to start with.
> @@ -319,18 +348,46 @@ EXPORT_SYMBOL(make_kgid);
>   *	Map @kgid into the user-namespace specified by @targ and
>   *	return the resulting gid.
>   *
> + *	This function is *not* appropriate for security checks because
> + *	if @targ is transparent, the mappings of an ancestor namespace
> + *	are used. If @targ isn't &init_user_ns and you intend to do
> + *	anything with the result apart from returning it to a process
> + *	in @targ, you might want to use from_kgid_opaque() instead.
> + *
>   *	There is always a mapping into the initial user_namespace.
>   *
> - *	If @kgid has no mapping in @targ (gid_t)-1 is returned.
> + *	If @kgid is not visible in @targ (gid_t)-1 is returned.
>   */
>  gid_t from_kgid(struct user_namespace *targ, kgid_t kgid)
>  {
>  	/* Map the gid from a global kernel gid */
> -	return map_id_up(&targ->gid_map, __kgid_val(kgid));
> +	struct user_namespace *opaque = READ_ONCE(targ->opaque);
> +
> +	return map_id_up(&opaque->gid_map, __kgid_val(kgid));
>  }
>  EXPORT_SYMBOL(from_kgid);
>  
>  /**
> + *	from_kgid_opaque - Create a gid from a kgid user-namespace pair.
> + *	@targ: The user namespace we want a gid in.
> + *	@kgid: The kernel internal gid to start with.
> + *
> + *	Map @kgid into the user-namespace specified by @targ and
> + *	return the resulting gid. This ignores transparent user
> + *	namespaces and is therefore appropriate for security checks.
> + *
> + *	There is always a mapping into the initial user_namespace.
> + *
> + *	If @kgid has no mapping in @targ (gid_t)-1 is returned.
> + */
> +gid_t from_kgid_opaque(struct user_namespace *targ, kgid_t kgid)
> +{
> +	/* Map the gid from a global kernel gid */
> +	return map_id_up(&targ->gid_map, __kgid_val(kgid));
> +}
> +EXPORT_SYMBOL(from_kgid_opaque);
> +
> +/**
>   *	from_kgid_munged - Create a gid from a kgid user-namespace pair.
>   *	@targ: The user namespace we want a gid in.
>   *	@kgid: The kernel internal gid to start with.
> @@ -811,6 +868,18 @@ static bool new_idmap_permitted(const struct file *file,
>  				struct uid_gid_map *new_map)
>  {
>  	const struct cred *cred = file->f_cred;
> +	unsigned int idx;
> +
> +	/* Don't allow non-identity mappings in transparent namespaces. */
> +	if (ns != ns->opaque) {
> +		for (idx = 0; idx < new_map->nr_extents; idx++) {
> +			struct uid_gid_extent *ext = &new_map->extent[idx];
> +
> +			if (ext->first != ext->lower_first)
> +				return false;
> +		}
> +	}
> +
>  	/* Don't allow mappings that would allow anything that wouldn't
>  	 * be allowed without the establishment of unprivileged mappings.
>  	 */
> @@ -922,6 +991,81 @@ out_unlock:
>  	goto out;
>  }
>  
> +int proc_transparent_show(struct seq_file *seq, void *v)
> +{
> +	struct user_namespace *ns = seq->private;
> +	struct user_namespace *opaque = READ_ONCE(ns->opaque);
> +
> +	seq_printf(seq, "%d\n", (ns == opaque) ? 0 : 1);
> +	return 0;
> +}
> +
> +ssize_t proc_transparent_write(struct file *file, const char __user *buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct seq_file *seq = file->private_data;
> +	struct user_namespace *ns = seq->private;
> +	char kbuf[8], *pos;
> +	bool transparent;
> +	ssize_t ret;
> +
> +	/* Only allow a very narrow range of strings to be written */
> +	ret = -EINVAL;
> +	if ((*ppos != 0) || (count >= sizeof(kbuf)))
> +		goto out;
> +
> +	/* What was written? */
> +	ret = -EFAULT;
> +	if (copy_from_user(kbuf, buf, count))
> +		goto out;
> +	kbuf[count] = '\0';
> +	pos = kbuf;
> +
> +	/* What is being requested? */
> +	ret = -EINVAL;
> +	if (pos[0] == '1') {
> +		pos += 1;
> +		transparent = true;
> +	} else if (pos[0] == '0') {
> +		pos += 1;
> +		transparent = false;
> +	} else
> +		goto out;
> +
> +	/* Verify there is not trailing junk on the line */
> +	pos = skip_spaces(pos);
> +	if (*pos != '\0')
> +		goto out;
> +
> +	ret = -EPERM;
> +	mutex_lock(&userns_state_mutex);
> +	/* Is the requested state different from the current one? */
> +	if (transparent != (ns->opaque != ns)) {
> +		/* You can't turn off transparent mode. */
> +		if (!transparent)
> +			goto out_unlock;
> +		/* If there are existing mappings, they might be
> +		 * non-identity mappings. Therefore, block transparent
> +		 * mode. This also prevents making the init namespace
> +		 * transparent (which wouldn't work).
> +		 */
> +		if (ns->uid_map.nr_extents != 0 || ns->gid_map.nr_extents != 0)
> +			goto out_unlock;
> +		/* Okay! Make the namespace transparent. */
> +		ns->opaque = ns->parent->opaque;
> +	}
> +	mutex_unlock(&userns_state_mutex);
> +
> +	/* Report a successful write */
> +	*ppos = count;
> +	ret = count;
> +out:
> +	return ret;
> +out_unlock:
> +	mutex_unlock(&userns_state_mutex);
> +	goto out;
> +}
> +
>  bool userns_may_setgroups(const struct user_namespace *ns)
>  {
>  	bool allowed;

[toc] | [next] | [standalone]

#1430124

On Thu, Jun 23, 2016 at 8:50 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Jann Horn <jannh@google.com> writes:
>
>> This allows the admin of a user namespace to mark the namespace as
>> transparent. All other namespaces, by default, are opaque.
>>
>> While the current behavior of user namespaces is appropriate for use in
>> containers, there are many programs that only use user namespaces because
>> doing so enables them to do other things (e.g. unsharing the mount or
>> network namespace) that require namespaced capabilities. For them, the
>> inability to see the real UIDs and GIDs of things from inside the user
>> namespace can be very annoying.
>>
>> In a transparent namespace, all UIDs and GIDs that are mapped into its
>> first opaque ancestor are visible and are not remapped. This means that if
>> a process e.g. stat()s the real root directory in a namespace, it will
>> still see it as owned by UID 0.
>>
>> Traditionally, any UID or GID that was visible in a user namespace was also
>> mapped into the namespace, giving the namespace admin full access to it.
>> This patch introduces a distinction: In a transparent namespace, UIDs and
>> GIDs can be visible without being mapped. Non-mapped, visible UIDs can be
>> passed from the kernel to userspace, but userspace can't send them back to
>> the kernel. In order to be able to fully use specific UIDs/GIDs and gain
>> privileges over them, mappings need to be set up in the usual way -
>> however, to avoid aliasing problems, only identity mappings are permitted.
>>
>> I have gone through all callers of from_kuid() and from_kgid(), and as far
>> as I can tell, kuid_has_mapping() and kgid_has_mapping() were the only
>> functions that used them for privilege checks. (The keys subsystem uses
>> them in an insecure way, and that issue has been known for a while, but my
>> patch doesn't make that any more vulnerable than it already is.
>
> Perhaps it has been known for a while but no one has stopped and
> mentioned it to me.  What questionable thing is the keys subsystem
> doing?

Not just questionable, completely wrong. The gist is that there is a
*global* name -> key mapping for accessing keys by name, and user
keyrings are stored in there under the name "_uid.%u", where %u
refers to the *namespaced* UID. (See install_user_keyrings().)
The result is that, if e.g. the user with UID 1000 has no running
processes, a local attacker can enter a new user namespace, map UID
1000 in the namespace to some KUID he controls, do
setresuid(1000, 1000, 1000), and now he owns user 1000's keyring.
This ends up permitting the attacker to dump the contents of KUID
1000's keys after KUID 1000 signs in. I discovered this while going
through the kuid->uid conversions when I thought about writing this
feature the first time.

(I think you have security list access, right? If so, you can see my PoC
and the discussion in the "namespace handling in security/keys is
broken" thread from 5. January 2016.)

> This is a bigish change in semantics and I am going to have to digest
> this before I can give this an ok.
>
> Quite frankly at the base it scares me.
>
> If this is just about presentation and allowing some information from
> the parent user namespace

Yes, that's exactly my intent.

> I would be much happier if it was not
> from_kuid but that you modified, but if you instead you had a function
> say from_kuid_transparent, that performed the transformation you need
> and was only used in those places it is safe.
>
> I think I could reason about that.
>
> As your patchset sits I can not reason about the change in semantics,
> because without a large grep of the source I don't know what you are
> changing.
>
> And you are dramatically changing the semantics the semantics of
> from_kuid to the point I do believe we need to inspepect all of the call
> sites.  As such I really don't think it makes sense to reuse the
> existing name for your new semantics.

Sure, that makes sense. I'll make a v2 with from_*uid_transparent() or
so.

[toc] | [prev] | [next] | [standalone]

#1430128

Jann Horn <jannh@google.com> writes:

> On Thu, Jun 23, 2016 at 8:50 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Jann Horn <jannh@google.com> writes:
>>
>>> This allows the admin of a user namespace to mark the namespace as
>>> transparent. All other namespaces, by default, are opaque.
>>>
>>> While the current behavior of user namespaces is appropriate for use in
>>> containers, there are many programs that only use user namespaces because
>>> doing so enables them to do other things (e.g. unsharing the mount or
>>> network namespace) that require namespaced capabilities. For them, the
>>> inability to see the real UIDs and GIDs of things from inside the user
>>> namespace can be very annoying.
>>>
>>> In a transparent namespace, all UIDs and GIDs that are mapped into its
>>> first opaque ancestor are visible and are not remapped. This means that if
>>> a process e.g. stat()s the real root directory in a namespace, it will
>>> still see it as owned by UID 0.
>>>
>>> Traditionally, any UID or GID that was visible in a user namespace was also
>>> mapped into the namespace, giving the namespace admin full access to it.
>>> This patch introduces a distinction: In a transparent namespace, UIDs and
>>> GIDs can be visible without being mapped. Non-mapped, visible UIDs can be
>>> passed from the kernel to userspace, but userspace can't send them back to
>>> the kernel. In order to be able to fully use specific UIDs/GIDs and gain
>>> privileges over them, mappings need to be set up in the usual way -
>>> however, to avoid aliasing problems, only identity mappings are permitted.
>>>
>>> I have gone through all callers of from_kuid() and from_kgid(), and as far
>>> as I can tell, kuid_has_mapping() and kgid_has_mapping() were the only
>>> functions that used them for privilege checks. (The keys subsystem uses
>>> them in an insecure way, and that issue has been known for a while, but my
>>> patch doesn't make that any more vulnerable than it already is.
>>
>> Perhaps it has been known for a while but no one has stopped and
>> mentioned it to me.  What questionable thing is the keys subsystem
>> doing?
>
> Not just questionable, completely wrong. The gist is that there is a
> *global* name -> key mapping for accessing keys by name, and user
> keyrings are stored in there under the name "_uid.%u", where %u
> refers to the *namespaced* UID. (See install_user_keyrings().)
> The result is that, if e.g. the user with UID 1000 has no running
> processes, a local attacker can enter a new user namespace, map UID
> 1000 in the namespace to some KUID he controls, do
> setresuid(1000, 1000, 1000), and now he owns user 1000's keyring.
> This ends up permitting the attacker to dump the contents of KUID
> 1000's keys after KUID 1000 signs in. I discovered this while going
> through the kuid->uid conversions when I thought about writing this
> feature the first time.

Ugh.  That definitely sounds like something worth addressing, and is
unfortunately how these kinds of things are found.

> (I think you have security list access, right? If so, you can see my PoC
> and the discussion in the "namespace handling in security/keys is
> broken" thread from 5. January 2016.)

I actually don't.  I have been Cc'd frequently so it would probably
smart if I did subscribe to the security list.  The rules of how a
person subscribes to that list have changed since last I understood what
was going on.  It used to be a vger list with a two week delay for
ordinary mortals to see the emails.

Currently I don't know what the rules for the list are, or how to
subscribe.  Any chance you might put me in contact with right people.
Not having been notified of that issue in January rather scares me.

>> This is a bigish change in semantics and I am going to have to digest
>> this before I can give this an ok.
>>
>> Quite frankly at the base it scares me.
>>
>> If this is just about presentation and allowing some information from
>> the parent user namespace
>
> Yes, that's exactly my intent.
>
>> I would be much happier if it was not
>> from_kuid but that you modified, but if you instead you had a function
>> say from_kuid_transparent, that performed the transformation you need
>> and was only used in those places it is safe.
>>
>> I think I could reason about that.
>>
>> As your patchset sits I can not reason about the change in semantics,
>> because without a large grep of the source I don't know what you are
>> changing.
>>
>> And you are dramatically changing the semantics the semantics of
>> from_kuid to the point I do believe we need to inspepect all of the call
>> sites.  As such I really don't think it makes sense to reuse the
>> existing name for your new semantics.
>
> Sure, that makes sense. I'll make a v2 with from_*uid_transparent() or
> so.

Thanks.  

Eric

[toc] | [prev] | [next] | [standalone]

#1431025 — [PATCH v2 1/2] namespaces: don't use from_k*id_munged() with init_user_ns

For init_user_ns, from_kuid_munged() and from_kgid_munged() are
guaranteed to be identical to from_kuid() and from_kgid(). In
preparation for another patch, change all usages of the _munged
variants that explicitly use init_user_ns to the non-munged ones.

This patch by itself should not have any effect.

Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/s390/hypfs/inode.c         |  4 ++--
 arch/x86/kernel/vm86_32.c       |  2 +-
 drivers/connector/cn_proc.c     |  8 ++++----
 fs/adfs/super.c                 |  4 ++--
 fs/autofs4/inode.c              |  4 ++--
 fs/cifs/cifs_spnego.c           |  4 ++--
 fs/cifs/cifsfs.c                |  8 ++++----
 fs/debugfs/inode.c              |  4 ++--
 fs/devpts/inode.c               |  4 ++--
 fs/ext2/super.c                 |  4 ++--
 fs/ext4/super.c                 |  4 ++--
 fs/fat/inode.c                  |  4 ++--
 fs/fuse/dev.c                   |  4 ++--
 fs/fuse/inode.c                 |  4 ++--
 fs/hfs/super.c                  |  4 ++--
 fs/hfsplus/options.c            |  4 ++--
 fs/hpfs/super.c                 |  4 ++--
 fs/ncpfs/inode.c                |  6 +++---
 fs/ntfs/inode.c                 |  4 ++--
 fs/proc/inode.c                 |  2 +-
 fs/quota/netlink.c              |  2 +-
 fs/tracefs/inode.c              |  4 ++--
 kernel/cred.c                   | 16 ++++++++--------
 mm/shmem.c                      |  4 ++--
 net/netfilter/nf_log_common.c   |  4 ++--
 net/netfilter/nfnetlink_queue.c |  4 ++--
 net/netfilter/nft_meta.c        |  4 ++--
 27 files changed, 62 insertions(+), 62 deletions(-)

diff --git a/arch/s390/hypfs/inode.c b/arch/s390/hypfs/inode.c
index 255c7ee..df0e2f3 100644
--- a/arch/s390/hypfs/inode.c
+++ b/arch/s390/hypfs/inode.c
@@ -259,8 +259,8 @@ static int hypfs_show_options(struct seq_file *s, struct dentry *root)
 {
 	struct hypfs_sb_info *hypfs_info = root->d_sb->s_fs_info;
 
-	seq_printf(s, ",uid=%u", from_kuid_munged(&init_user_ns, hypfs_info->uid));
-	seq_printf(s, ",gid=%u", from_kgid_munged(&init_user_ns, hypfs_info->gid));
+	seq_printf(s, ",uid=%u", from_kuid(&init_user_ns, hypfs_info->uid));
+	seq_printf(s, ",gid=%u", from_kgid(&init_user_ns, hypfs_info->gid));
 	return 0;
 }
 
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index 3dce1ca..23d8541 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -259,7 +259,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
 		 */
 		pr_info_once("Denied a call to vm86(old) from %s[%d] (uid: %d).  Set the vm.mmap_min_addr sysctl to 0 and/or adjust LSM mmap_min_addr policy to enable vm86 if you are using a vm86-based DOS emulator.\n",
 			     current->comm, task_pid_nr(current),
-			     from_kuid_munged(&init_user_ns, current_uid()));
+			     from_kuid(&init_user_ns, current_uid()));
 		return -EPERM;
 	}
 
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 15d06fc..89c3b6f 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -140,11 +140,11 @@ void proc_id_connector(struct task_struct *task, int which_id)
 	rcu_read_lock();
 	cred = __task_cred(task);
 	if (which_id == PROC_EVENT_UID) {
-		ev->event_data.id.r.ruid = from_kuid_munged(&init_user_ns, cred->uid);
-		ev->event_data.id.e.euid = from_kuid_munged(&init_user_ns, cred->euid);
+		ev->event_data.id.r.ruid = from_kuid(&init_user_ns, cred->uid);
+		ev->event_data.id.e.euid = from_kuid(&init_user_ns, cred->euid);
 	} else if (which_id == PROC_EVENT_GID) {
-		ev->event_data.id.r.rgid = from_kgid_munged(&init_user_ns, cred->gid);
-		ev->event_data.id.e.egid = from_kgid_munged(&init_user_ns, cred->egid);
+		ev->event_data.id.r.rgid = from_kgid(&init_user_ns, cred->gid);
+		ev->event_data.id.e.egid = from_kgid(&init_user_ns, cred->egid);
 	} else {
 		rcu_read_unlock();
 		return;
diff --git a/fs/adfs/super.c b/fs/adfs/super.c
index c9fdfb1..48b9195 100644
--- a/fs/adfs/super.c
+++ b/fs/adfs/super.c
@@ -131,9 +131,9 @@ static int adfs_show_options(struct seq_file *seq, struct dentry *root)
 	struct adfs_sb_info *asb = ADFS_SB(root->d_sb);
 
 	if (!uid_eq(asb->s_uid, GLOBAL_ROOT_UID))
-		seq_printf(seq, ",uid=%u", from_kuid_munged(&init_user_ns, asb->s_uid));
+		seq_printf(seq, ",uid=%u", from_kuid(&init_user_ns, asb->s_uid));
 	if (!gid_eq(asb->s_gid, GLOBAL_ROOT_GID))
-		seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, asb->s_gid));
+		seq_printf(seq, ",gid=%u", from_kgid(&init_user_ns, asb->s_gid));
 	if (asb->s_owner_mask != ADFS_DEFAULT_OWNER_MASK)
 		seq_printf(seq, ",ownmask=%o", asb->s_owner_mask);
 	if (asb->s_other_mask != ADFS_DEFAULT_OTHER_MASK)
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 61b2105..85a306f 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -77,10 +77,10 @@ static int autofs4_show_options(struct seq_file *m, struct dentry *root)
 	seq_printf(m, ",fd=%d", sbi->pipefd);
 	if (!uid_eq(root_inode->i_uid, GLOBAL_ROOT_UID))
 		seq_printf(m, ",uid=%u",
-			from_kuid_munged(&init_user_ns, root_inode->i_uid));
+			from_kuid(&init_user_ns, root_inode->i_uid));
 	if (!gid_eq(root_inode->i_gid, GLOBAL_ROOT_GID))
 		seq_printf(m, ",gid=%u",
-			from_kgid_munged(&init_user_ns, root_inode->i_gid));
+			from_kgid(&init_user_ns, root_inode->i_gid));
 	seq_printf(m, ",pgrp=%d", pid_vnr(sbi->oz_pgrp));
 	seq_printf(m, ",timeout=%lu", sbi->exp_timeout/HZ);
 	seq_printf(m, ",minproto=%d", sbi->min_proto);
diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c
index b611fc2..3784b8f 100644
--- a/fs/cifs/cifs_spnego.c
+++ b/fs/cifs/cifs_spnego.c
@@ -152,11 +152,11 @@ cifs_get_spnego_key(struct cifs_ses *sesInfo)
 
 	dp = description + strlen(description);
 	sprintf(dp, ";uid=0x%x",
-		from_kuid_munged(&init_user_ns, sesInfo->linux_uid));
+		from_kuid(&init_user_ns, sesInfo->linux_uid));
 
 	dp = description + strlen(description);
 	sprintf(dp, ";creduid=0x%x",
-		from_kuid_munged(&init_user_ns, sesInfo->cred_uid));
+		from_kuid(&init_user_ns, sesInfo->cred_uid));
 
 	if (sesInfo->user_name) {
 		dp = description + strlen(description);
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5d8b7ed..36af22a 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -428,14 +428,14 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 	}
 
 	seq_printf(s, ",uid=%u",
-		   from_kuid_munged(&init_user_ns, cifs_sb->mnt_uid));
+		   from_kuid(&init_user_ns, cifs_sb->mnt_uid));
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID)
 		seq_puts(s, ",forceuid");
 	else
 		seq_puts(s, ",noforceuid");
 
 	seq_printf(s, ",gid=%u",
-		   from_kgid_munged(&init_user_ns, cifs_sb->mnt_gid));
+		   from_kgid(&init_user_ns, cifs_sb->mnt_gid));
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_GID)
 		seq_puts(s, ",forcegid");
 	else
@@ -500,11 +500,11 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 		seq_puts(s, ",noperm");
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_BACKUPUID)
 		seq_printf(s, ",backupuid=%u",
-			   from_kuid_munged(&init_user_ns,
+			   from_kuid(&init_user_ns,
 					    cifs_sb->mnt_backupuid));
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_BACKUPGID)
 		seq_printf(s, ",backupgid=%u",
-			   from_kgid_munged(&init_user_ns,
+			   from_kgid(&init_user_ns,
 					    cifs_sb->mnt_backupgid));
 
 	seq_printf(s, ",rsize=%u", cifs_sb->rsize);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 4bc1f68..ff4cffe 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -160,10 +160,10 @@ static int debugfs_show_options(struct seq_file *m, struct dentry *root)
 
 	if (!uid_eq(opts->uid, GLOBAL_ROOT_UID))
 		seq_printf(m, ",uid=%u",
-			   from_kuid_munged(&init_user_ns, opts->uid));
+			   from_kuid(&init_user_ns, opts->uid));
 	if (!gid_eq(opts->gid, GLOBAL_ROOT_GID))
 		seq_printf(m, ",gid=%u",
-			   from_kgid_munged(&init_user_ns, opts->gid));
+			   from_kgid(&init_user_ns, opts->gid));
 	if (opts->mode != DEBUGFS_DEFAULT_MODE)
 		seq_printf(m, ",mode=%o", opts->mode);
 
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 37c134a..2809cb9 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -357,10 +357,10 @@ static int devpts_show_options(struct seq_file *seq, struct dentry *root)
 
 	if (opts->setuid)
 		seq_printf(seq, ",uid=%u",
-			   from_kuid_munged(&init_user_ns, opts->uid));
+			   from_kuid(&init_user_ns, opts->uid));
 	if (opts->setgid)
 		seq_printf(seq, ",gid=%u",
-			   from_kgid_munged(&init_user_ns, opts->gid));
+			   from_kgid(&init_user_ns, opts->gid));
 	seq_printf(seq, ",mode=%03o", opts->mode);
 	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
 	if (opts->max < NR_UNIX98_PTY_MAX)
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 1d93795..0d66d5d 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -244,12 +244,12 @@ static int ext2_show_options(struct seq_file *seq, struct dentry *root)
 	if (!uid_eq(sbi->s_resuid, make_kuid(&init_user_ns, EXT2_DEF_RESUID)) ||
 	    le16_to_cpu(es->s_def_resuid) != EXT2_DEF_RESUID) {
 		seq_printf(seq, ",resuid=%u",
-				from_kuid_munged(&init_user_ns, sbi->s_resuid));
+				from_kuid(&init_user_ns, sbi->s_resuid));
 	}
 	if (!gid_eq(sbi->s_resgid, make_kgid(&init_user_ns, EXT2_DEF_RESGID)) ||
 	    le16_to_cpu(es->s_def_resgid) != EXT2_DEF_RESGID) {
 		seq_printf(seq, ",resgid=%u",
-				from_kgid_munged(&init_user_ns, sbi->s_resgid));
+				from_kgid(&init_user_ns, sbi->s_resgid));
 	}
 	if (test_opt(sb, ERRORS_RO)) {
 		int def_errors = le16_to_cpu(es->s_errors);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3822a5a..4c5664b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1879,11 +1879,11 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
 	if (nodefs || !uid_eq(sbi->s_resuid, make_kuid(&init_user_ns, EXT4_DEF_RESUID)) ||
 	    le16_to_cpu(es->s_def_resuid) != EXT4_DEF_RESUID)
 		SEQ_OPTS_PRINT("resuid=%u",
-				from_kuid_munged(&init_user_ns, sbi->s_resuid));
+				from_kuid(&init_user_ns, sbi->s_resuid));
 	if (nodefs || !gid_eq(sbi->s_resgid, make_kgid(&init_user_ns, EXT4_DEF_RESGID)) ||
 	    le16_to_cpu(es->s_def_resgid) != EXT4_DEF_RESGID)
 		SEQ_OPTS_PRINT("resgid=%u",
-				from_kgid_munged(&init_user_ns, sbi->s_resgid));
+				from_kgid(&init_user_ns, sbi->s_resgid));
 	def_errors = nodefs ? -1 : le16_to_cpu(es->s_errors);
 	if (test_opt(sb, ERRORS_RO) && def_errors != EXT4_ERRORS_RO)
 		SEQ_OPTS_PUTS("errors=remount-ro");
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 3bcf579..e56472e 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -923,10 +923,10 @@ static int fat_show_options(struct seq_file *m, struct dentry *root)
 
 	if (!uid_eq(opts->fs_uid, GLOBAL_ROOT_UID))
 		seq_printf(m, ",uid=%u",
-				from_kuid_munged(&init_user_ns, opts->fs_uid));
+				from_kuid(&init_user_ns, opts->fs_uid));
 	if (!gid_eq(opts->fs_gid, GLOBAL_ROOT_GID))
 		seq_printf(m, ",gid=%u",
-				from_kgid_munged(&init_user_ns, opts->fs_gid));
+				from_kgid(&init_user_ns, opts->fs_gid));
 	seq_printf(m, ",fmask=%04o", opts->fs_fmask);
 	seq_printf(m, ",dmask=%04o", opts->fs_dmask);
 	if (opts->allow_utime)
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index cbece12..811c96c 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -126,8 +126,8 @@ static void __fuse_put_request(struct fuse_req *req)
 
 static void fuse_req_init_context(struct fuse_req *req)
 {
-	req->in.h.uid = from_kuid_munged(&init_user_ns, current_fsuid());
-	req->in.h.gid = from_kgid_munged(&init_user_ns, current_fsgid());
+	req->in.h.uid = from_kuid(&init_user_ns, current_fsuid());
+	req->in.h.gid = from_kgid(&init_user_ns, current_fsgid());
 	req->in.h.pid = current->pid;
 }
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 1ce6766..fb5b216 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -554,8 +554,8 @@ static int fuse_show_options(struct seq_file *m, struct dentry *root)
 	struct super_block *sb = root->d_sb;
 	struct fuse_conn *fc = get_fuse_conn_super(sb);
 
-	seq_printf(m, ",user_id=%u", from_kuid_munged(&init_user_ns, fc->user_id));
-	seq_printf(m, ",group_id=%u", from_kgid_munged(&init_user_ns, fc->group_id));
+	seq_printf(m, ",user_id=%u", from_kuid(&init_user_ns, fc->user_id));
+	seq_printf(m, ",group_id=%u", from_kgid(&init_user_ns, fc->group_id));
 	if (fc->flags & FUSE_DEFAULT_PERMISSIONS)
 		seq_puts(m, ",default_permissions");
 	if (fc->flags & FUSE_ALLOW_OTHER)
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 1ca95c2..30c952a 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -140,8 +140,8 @@ static int hfs_show_options(struct seq_file *seq, struct dentry *root)
 	if (sbi->s_type != cpu_to_be32(0x3f3f3f3f))
 		seq_show_option_n(seq, "type", (char *)&sbi->s_type, 4);
 	seq_printf(seq, ",uid=%u,gid=%u",
-			from_kuid_munged(&init_user_ns, sbi->s_uid),
-			from_kgid_munged(&init_user_ns, sbi->s_gid));
+			from_kuid(&init_user_ns, sbi->s_uid),
+			from_kgid(&init_user_ns, sbi->s_gid));
 	if (sbi->s_file_umask != 0133)
 		seq_printf(seq, ",file_umask=%o", sbi->s_file_umask);
 	if (sbi->s_dir_umask != 0022)
diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c
index bb806e5..6897f9f 100644
--- a/fs/hfsplus/options.c
+++ b/fs/hfsplus/options.c
@@ -222,8 +222,8 @@ int hfsplus_show_options(struct seq_file *seq, struct dentry *root)
 	if (sbi->type != HFSPLUS_DEF_CR_TYPE)
 		seq_show_option_n(seq, "type", (char *)&sbi->type, 4);
 	seq_printf(seq, ",umask=%o,uid=%u,gid=%u", sbi->umask,
-			from_kuid_munged(&init_user_ns, sbi->uid),
-			from_kgid_munged(&init_user_ns, sbi->gid));
+			from_kuid(&init_user_ns, sbi->uid),
+			from_kgid(&init_user_ns, sbi->gid));
 	if (sbi->part >= 0)
 		seq_printf(seq, ",part=%u", sbi->part);
 	if (sbi->session >= 0)
diff --git a/fs/hpfs/super.c b/fs/hpfs/super.c
index 82067ca..98d632b 100644
--- a/fs/hpfs/super.c
+++ b/fs/hpfs/super.c
@@ -502,8 +502,8 @@ static int hpfs_show_options(struct seq_file *seq, struct dentry *root)
 {
 	struct hpfs_sb_info *sbi = hpfs_sb(root->d_sb);
 
-	seq_printf(seq, ",uid=%u", from_kuid_munged(&init_user_ns, sbi->sb_uid));
-	seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, sbi->sb_gid));
+	seq_printf(seq, ",uid=%u", from_kuid(&init_user_ns, sbi->sb_uid));
+	seq_printf(seq, ",gid=%u", from_kgid(&init_user_ns, sbi->sb_gid));
 	seq_printf(seq, ",umask=%03o", (~sbi->sb_mode & 0777));
 	if (sbi->sb_lowercase)
 		seq_printf(seq, ",case=lower");
diff --git a/fs/ncpfs/inode.c b/fs/ncpfs/inode.c
index 1af15fc..43770d3 100644
--- a/fs/ncpfs/inode.c
+++ b/fs/ncpfs/inode.c
@@ -334,13 +334,13 @@ static int  ncp_show_options(struct seq_file *seq, struct dentry *root)
 
 	if (!uid_eq(server->m.uid, GLOBAL_ROOT_UID))
 		seq_printf(seq, ",uid=%u",
-			   from_kuid_munged(&init_user_ns, server->m.uid));
+			   from_kuid(&init_user_ns, server->m.uid));
 	if (!gid_eq(server->m.gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
-			   from_kgid_munged(&init_user_ns, server->m.gid));
+			   from_kgid(&init_user_ns, server->m.gid));
 	if (!uid_eq(server->m.mounted_uid, GLOBAL_ROOT_UID))
 		seq_printf(seq, ",owner=%u",
-			   from_kuid_munged(&init_user_ns, server->m.mounted_uid));
+			   from_kuid(&init_user_ns, server->m.mounted_uid));
 	tmp = server->m.file_mode & S_IALLUGO;
 	if (tmp != NCP_DEFAULT_FILE_MODE)
 		seq_printf(seq, ",mode=0%o", tmp);
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index f40972d..dc9dd5d 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -2314,8 +2314,8 @@ int ntfs_show_options(struct seq_file *sf, struct dentry *root)
 	ntfs_volume *vol = NTFS_SB(root->d_sb);
 	int i;
 
-	seq_printf(sf, ",uid=%i", from_kuid_munged(&init_user_ns, vol->uid));
-	seq_printf(sf, ",gid=%i", from_kgid_munged(&init_user_ns, vol->gid));
+	seq_printf(sf, ",uid=%i", from_kuid(&init_user_ns, vol->uid));
+	seq_printf(sf, ",gid=%i", from_kgid(&init_user_ns, vol->gid));
 	if (vol->fmask == vol->dmask)
 		seq_printf(sf, ",umask=0%o", vol->fmask);
 	else {
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 42305dd..4cba964 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -106,7 +106,7 @@ static int proc_show_options(struct seq_file *seq, struct dentry *root)
 	struct pid_namespace *pid = sb->s_fs_info;
 
 	if (!gid_eq(pid->pid_gid, GLOBAL_ROOT_GID))
-		seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, pid->pid_gid));
+		seq_printf(seq, ",gid=%u", from_kgid(&init_user_ns, pid->pid_gid));
 	if (pid->hide_pid != 0)
 		seq_printf(seq, ",hidepid=%u", pid->hide_pid);
 
diff --git a/fs/quota/netlink.c b/fs/quota/netlink.c
index 8b25267..042c799 100644
--- a/fs/quota/netlink.c
+++ b/fs/quota/netlink.c
@@ -83,7 +83,7 @@ void quota_send_warning(struct kqid qid, dev_t dev,
 	if (ret)
 		goto attr_err_out;
 	ret = nla_put_u64_64bit(skb, QUOTA_NL_A_CAUSED_ID,
-				from_kuid_munged(&init_user_ns, current_uid()),
+				from_kuid(&init_user_ns, current_uid()),
 				QUOTA_NL_A_PAD);
 	if (ret)
 		goto attr_err_out;
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 4a0e48f..74b6645 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -248,10 +248,10 @@ static int tracefs_show_options(struct seq_file *m, struct dentry *root)
 
 	if (!uid_eq(opts->uid, GLOBAL_ROOT_UID))
 		seq_printf(m, ",uid=%u",
-			   from_kuid_munged(&init_user_ns, opts->uid));
+			   from_kuid(&init_user_ns, opts->uid));
 	if (!gid_eq(opts->gid, GLOBAL_ROOT_GID))
 		seq_printf(m, ",gid=%u",
-			   from_kgid_munged(&init_user_ns, opts->gid));
+			   from_kgid(&init_user_ns, opts->gid));
 	if (opts->mode != TRACEFS_DEFAULT_MODE)
 		seq_printf(m, ",mode=%o", opts->mode);
 
diff --git a/kernel/cred.c b/kernel/cred.c
index 0c0cd8a..da39a3d 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -735,15 +735,15 @@ static void dump_invalid_creds(const struct cred *cred, const char *label,
 	       atomic_read(&cred->usage),
 	       read_cred_subscribers(cred));
 	printk(KERN_ERR "CRED: ->*uid = { %d,%d,%d,%d }\n",
-		from_kuid_munged(&init_user_ns, cred->uid),
-		from_kuid_munged(&init_user_ns, cred->euid),
-		from_kuid_munged(&init_user_ns, cred->suid),
-		from_kuid_munged(&init_user_ns, cred->fsuid));
+		from_kuid(&init_user_ns, cred->uid),
+		from_kuid(&init_user_ns, cred->euid),
+		from_kuid(&init_user_ns, cred->suid),
+		from_kuid(&init_user_ns, cred->fsuid));
 	printk(KERN_ERR "CRED: ->*gid = { %d,%d,%d,%d }\n",
-		from_kgid_munged(&init_user_ns, cred->gid),
-		from_kgid_munged(&init_user_ns, cred->egid),
-		from_kgid_munged(&init_user_ns, cred->sgid),
-		from_kgid_munged(&init_user_ns, cred->fsgid));
+		from_kgid(&init_user_ns, cred->gid),
+		from_kgid(&init_user_ns, cred->egid),
+		from_kgid(&init_user_ns, cred->sgid),
+		from_kgid(&init_user_ns, cred->fsgid));
 #ifdef CONFIG_SECURITY
 	printk(KERN_ERR "CRED: ->security is %p\n", cred->security);
 	if ((unsigned long) cred->security >= PAGE_SIZE &&
diff --git a/mm/shmem.c b/mm/shmem.c
index a361449..144b3d7 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2937,10 +2937,10 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
 		seq_printf(seq, ",mode=%03ho", sbinfo->mode);
 	if (!uid_eq(sbinfo->uid, GLOBAL_ROOT_UID))
 		seq_printf(seq, ",uid=%u",
-				from_kuid_munged(&init_user_ns, sbinfo->uid));
+				from_kuid(&init_user_ns, sbinfo->uid));
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
-				from_kgid_munged(&init_user_ns, sbinfo->gid));
+				from_kgid(&init_user_ns, sbinfo->gid));
 	shmem_show_mpol(seq, sbinfo->mpol);
 	return 0;
 }
diff --git a/net/netfilter/nf_log_common.c b/net/netfilter/nf_log_common.c
index a5aa596..b115895 100644
--- a/net/netfilter/nf_log_common.c
+++ b/net/netfilter/nf_log_common.c
@@ -141,8 +141,8 @@ void nf_log_dump_sk_uid_gid(struct nf_log_buf *m, struct sock *sk)
 	if (sk->sk_socket && sk->sk_socket->file) {
 		const struct cred *cred = sk->sk_socket->file->f_cred;
 		nf_log_buf_add(m, "UID=%u GID=%u ",
-			from_kuid_munged(&init_user_ns, cred->fsuid),
-			from_kgid_munged(&init_user_ns, cred->fsgid));
+			from_kuid(&init_user_ns, cred->fsuid),
+			from_kgid(&init_user_ns, cred->fsgid));
 	}
 	read_unlock_bh(&sk->sk_callback_lock);
 }
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 5d36a09..71e57bf 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -264,10 +264,10 @@ static int nfqnl_put_sk_uidgid(struct sk_buff *skb, struct sock *sk)
 	if (sk->sk_socket && sk->sk_socket->file) {
 		cred = sk->sk_socket->file->f_cred;
 		if (nla_put_be32(skb, NFQA_UID,
-		    htonl(from_kuid_munged(&init_user_ns, cred->fsuid))))
+		    htonl(from_kuid(&init_user_ns, cred->fsuid))))
 			goto nla_put_failure;
 		if (nla_put_be32(skb, NFQA_GID,
-		    htonl(from_kgid_munged(&init_user_ns, cred->fsgid))))
+		    htonl(from_kgid(&init_user_ns, cred->fsgid))))
 			goto nla_put_failure;
 	}
 	read_unlock_bh(&sk->sk_callback_lock);
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 16c50b0..fbdbb52 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -104,7 +104,7 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 			goto err;
 		}
 
-		*dest =	from_kuid_munged(&init_user_ns,
+		*dest =	from_kuid(&init_user_ns,
 				sk->sk_socket->file->f_cred->fsuid);
 		read_unlock_bh(&sk->sk_callback_lock);
 		break;
@@ -119,7 +119,7 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 			read_unlock_bh(&sk->sk_callback_lock);
 			goto err;
 		}
-		*dest =	from_kgid_munged(&init_user_ns,
+		*dest =	from_kgid(&init_user_ns,
 				 sk->sk_socket->file->f_cred->fsgid);
 		read_unlock_bh(&sk->sk_callback_lock);
 		break;
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1432128 — Re: [PATCH v2 2/2] namespaces: add transparent user namespaces

Added a few more relevant cc's.

Jann Horn <jannh@google.com> writes:

> This allows the admin of a user namespace to mark the namespace as
> transparent. All other namespaces, by default, are opaque.


I have just skimmed through this and at a high level this doesn't seem
too scary.  Having an identity mapped user namespace that just limits
you to using a subset of uids and gids while allowing displaying the
full range of uids and gids.

I don't quite get the use case and I would like to a little better
but in the long term this shouldn't cause any significant maintenance
issues, so I don't have any objects.

At the same time this isn't quite the time to merge this.  I am in the
process of slowly going through Seth's vfs changes to support things
such as truly unprivileged fuse support.  Those changes alter which
places can always be assumed to be init_user_ns (many fewer), and also
slightly change the set of from_kuid calls being made.

The changes that have made it through my review currently reside at:

    git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-next

Those vfs changes make it conceivable and simple from an infrastructure
standpoint to transition fileystems to unprivileged user namespace
mounts, with perhaps as little work as just setting FS_USER_NS.  At the
same time that won't be recommend because of the difficulty verifying
evil filesystem contents can't cause fs implementations to do bad things
is difficult.

That change means your first patch that just zaps all from_kuid_munged
users in init_user_ns isn't a particularly good idea.  I don't think it
is a good idea to have one set of rules for things that will always be
init_user_ns and another set of rules for code that will change.

The long and short of this is I am asking you to wait a week or so and
rebase this on my for-next branch so that we can confirm this change
interacts nicely will all of the other on-going work.

Thank you,
Eric Biederman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web