linux-kernel - Re: [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).

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] [day] [month] [year] [list]

Message-ID: <874ngqe4r5.fsf@xmission.com>
Date:	Mon, 04 Mar 2013 22:32:14 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	"Raphael S.Carvalho" <raphael.scarv@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).

"Raphael S.Carvalho" <raphael.scarv@...il.com> writes:

> Starting point: create_pid_namespace()
>
> Suppose create_pid_cachep() was executed sucessfully, thus:
> pcache was allocated by kmalloc().
> cachep received a cache created by kmem_cache_create().
> and pcache->list was added to the list pid_caches_lh.
>
> So what would happen if proc_alloc_inum() returns an error?
> The resources allocated by create_pid_namespace() would be deallocated!
> How about those resources allocated by create_pid_cachep()?
> By knowing that, I created this patch in order to fix that!

pid caches are not per namespace.  There are per pid namespace depth
and shared among many pid namespaces so in general a leak is fine.

We definitely can't do what you are doing.  There are no checks that
another pid namespace doesn't have pids allocated from the pid cache
you are freeing nor any checks to see that the pid cache was allocated
uniquely per pid namespace.

Under the right circumstances you might be able to free pid caches
but it is hard to figure out what those circumstances are and I don't
expect it is worth the trouble.

Eric


> Signed-off-by: Raphael S.Carvalho <raphael.scarv@...il.com>
> ---
>  kernel/pid_namespace.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index c1c3dc1..d94e4b6 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -37,7 +37,7 @@ static struct kmem_cache *pid_ns_cachep;
>   * @nr_ids: the number of numerical ids this pid will have to carry
>   */
>  
> -static struct kmem_cache *create_pid_cachep(int nr_ids)
> +static struct pid_cache *create_pid_cachep(int nr_ids)
>  {
>  	struct pid_cache *pcache;
>  	struct kmem_cache *cachep;
> @@ -63,7 +63,7 @@ static struct kmem_cache *create_pid_cachep(int nr_ids)
>  	list_add(&pcache->list, &pid_caches_lh);
>  out:
>  	mutex_unlock(&pid_caches_mutex);
> -	return pcache->cachep;
> +	return pcache;
>  
>  err_cachep:
>  	kfree(pcache);
> @@ -85,6 +85,7 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
>  	struct pid_namespace *parent_pid_ns)
>  {
>  	struct pid_namespace *ns;
> +	struct pid_cache *pcache;
>  	unsigned int level = parent_pid_ns->level + 1;
>  	int i;
>  	int err;
> @@ -103,15 +104,16 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
>  	if (!ns->pidmap[0].page)
>  		goto out_free;
>  
> -	ns->pid_cachep = create_pid_cachep(level + 1);
> -	if (ns->pid_cachep == NULL)
> +	pcache = create_pid_cachep(level + 1);
> +	if (pcache == NULL)
>  		goto out_free_map;
>  
>  	err = proc_alloc_inum(&ns->proc_inum);
>  	if (err)
> -		goto out_free_map;
> +		goto out_free_cachep;
>  
>  	kref_init(&ns->kref);
> +	ns->pid_cachep = pcache->cachep;
>  	ns->level = level;
>  	ns->parent = get_pid_ns(parent_pid_ns);
>  	ns->user_ns = get_user_ns(user_ns);
> @@ -126,6 +128,10 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
>  
>  	return ns;
>  
> +out_free_cachep:
> +	kmem_cache_destroy(pcache->cachep);
> +	list_del(&pcache->list);
> +	kfree(pcache);
>  out_free_map:
>  	kfree(ns->pidmap[0].page);
>  out_free:
--
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/