linux-kernel - Re: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child

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: <87vbm4ff0r.fsf@x220.int.ebiederm.org>
Date:	Mon, 24 Nov 2014 15:46:28 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Aaron Tomlin <atomlin@...hat.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	Sterling Alexander <stalexan@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting

Oleg Nesterov <oleg@...hat.com> writes:

> alloc_pid() does get_pid_ns() beforehand but forgets to put_pid_ns()
> if it fails because disable_pid_allocation() was called by the exiting
> child_reaper. We can move get_pid_ns() down to successful return.
>
> While at it, simplify/cleanup the "goto out" mess, no need to confuse
> the success/error return paths.
>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  kernel/pid.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 9b9a266..dfc2f3b 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -320,7 +320,6 @@ struct pid *alloc_pid(struct pid_namespace *ns)
>  			goto out_free;
>  	}
>  
> -	get_pid_ns(ns);
>  	atomic_set(&pid->count, 1);
>  	for (type = 0; type < PIDTYPE_MAX; ++type)
>  		INIT_HLIST_HEAD(&pid->tasks[type]);
> @@ -336,7 +335,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
>  	}
>  	spin_unlock_irq(&pidmap_lock);
>  
> -out:
> +	get_pid_ns(ns);

Moving the label and changing the goto out logic is gratuitous confusing
and I think it probably even generates worse code.

Furthermore multiple exits make adding debugging code more difficult.

Moving get_pid_ns down does close a leak in the error handling path.

However at the moment my I can't figure out if it is safe to move
get_pid_ns elow hlist_add_head_rcu.  Because once we are on the rcu list
the pid is findable, and being publicly visible with a bad refcount could cause
problems.

The increment of ns->nr_hashed after the adding to the rcu list is only safe
because we always access ns->nr_hashed with the pidmap_lock held.

Eric


>  	return pid;
>  
>  out_unlock:
> @@ -346,8 +345,8 @@ out_free:
>  		free_pidmap(pid->numbers + i);
>  
>  	kmem_cache_free(ns->pid_cachep, pid);
> -	pid = NULL;
> -	goto out;
> +out:
> +	return NULL;
>  }
>  
>  void disable_pid_allocation(struct pid_namespace *ns)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/