linux-kernel - Re: [RFC PATCH] namespaces: fix leak on fork() failure

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: <m11un5zzzb.fsf@fess.ebiederm.org>
Date:	Sun, 29 Apr 2012 19:59:36 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Mike Galbraith <efault@....de>,
	LKML <linux-kernel@...r.kernel.org>,
	Pavel Emelyanov <xemul@...allels.com>,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	Louis Rilling <louis.rilling@...labs.com>
Subject: Re: [RFC PATCH] namespaces: fix leak on fork() failure

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

> On 04/29, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@...hat.com> writes:
>
>> > But this particular one looks simple iirc.
>> >
>> >> @@ -216,6 +216,14 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
>> >>  	rcu_assign_pointer(p->nsproxy, new);
>> >>
>> >>  	if (ns && atomic_dec_and_test(&ns->count)) {
>> >> +		/* Handle fork() failure, unmount proc before proceeding */
>> >> +		if (unlikely(!new && !((p->flags & PF_EXITING)))) {
>> >> +			struct pid_namespace *pid_ns = ns->pid_ns;
>> >> +
>> >> +			if (pid_ns && pid_ns != &init_pid_ns)
>> >> +				pid_ns_release_proc(pid_ns);
>> >> +		}
>> >> +
>> >>  		/*
>> >>  		 * wait for others to get what they want from this nsproxy.
>> >>  		 *
>> >
>> > At first glance this looks correct. But the PF_EXITING check doesn't
>> > look very nice imho. It is needed to detect the case when the caller
>> > is copy_process()->bad_fork_cleanup_namespaces and p is not current.
>>
>> Mike's proposed change to switch_task_namespace is most definitely not
>> correct.  This will potentially get called on unshare
>
> Yes, but please note that this change also checks "new == NULL", so I
> still think the patch is correct.

Sort of.  It is correct in the sense that it performs magic checks on
it's arguments to see that it's caller is exit_task_namespaces called
from the fork failure path.

It is incorrect in the case that it doesn't handle weird cases like
daemonize() which also call switch_namespaces.  So it is no better and
much more confusing and much less maintainable than your two line patch
below.

> But,
>
>> > 	 bad_fork_cleanup_namespaces:
>> > 	+	if (unlikely(clone_flags & CLONE_NEWPID))
>> > 	+		pid_ns_release_proc(...);
>> > 		exit_task_namespaces(p);
>> > 		
>> > 		
>> > code into this error path in copy_process?
>>
>> For now Oleg your minimal patch looks good.
>
> Good.
>
> Mike, could you please re-send the patch to akpm? Feel free to add my ack.
> I guess Eric will ack this fix too.

I will.

>> There are crazy code paths like daemonize()
>
> Forget. It has no callers anymore, should be killed. A user-space process
> should never use kernel_thread() and thus daemonize() is not needed.

Good point.  Oleg do you think you can send in the patches to kill
daemonize.  I make it a lot easier to sleep at night and review patches
if I did not have to think about that scary code path.

Eric



--
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/