linux-kernel - Re: cgroup_release_agent() with call_usermodehelper() with UMH_WAIT

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: <20120203160454.GA3092@redhat.com>
Date:	Fri, 3 Feb 2012 17:04:54 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Heiko Carstens <heiko.carstens@...ibm.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Paul Menage <paul@...lmenage.org>,
	linux-kernel@...r.kernel.org,
	Sebastian Ott <sebott@...ux.vnet.ibm.com>
Subject: Re: cgroup_release_agent() with call_usermodehelper() with
	UMH_WAIT_EXEC may crash

On 02/03, Heiko Carstens wrote:
>
> setup_new_exec(...)
> 	[...]
> 	name = bprm->filename;
>
> 	/* Copies the binary name from after last slash */
> 	for (i=0; (ch = *(name++)) != '\0';) {	<-- crashes here
> 		if (ch == '/')

Ough, and this happens after flush_old_exec()...

> Looking into the dump I was able to tell that the piece of memory got freed
> by cgroup_release_agent().
> Which has the following code sequence:
>
> static void cgroup_release_agent(struct work_struct *work)
> {
> 		[...]
> 		agentbuf = kstrdup(cgrp->root->release_agent_path, GFP_KERNEL);
> 		[...]
> 		i = 0;
> 		argv[i++] = agentbuf;
> 		[...]
> 		call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
> 		[...]
> 		kfree(agentbuf);
> 		[...]
> }
>
> So obviously cgroup_release_agent() freed the filename before do_execve()
> was finished.

Good catch.

> So the question is: what is broken? The cgroup stuff which doesn't take
> into account that the passed path may still be in use and hence can't
> be freed (simple fix would be to simply use UMH_WAIT_PROC instead).
> Or is it that call_usermodehelper() still uses the passed path after
> it returned?

Well, it seems that do_coredump() has the same problem.

Can't we simply move that code into flush_old_exec() ? (wrapped into
the new helper).

Oleg.

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