Re: cgroup_release_agent() with call_usermodehelper() with UMH_WAIT_EXEC may crash

[Oleg Nesterov <oleg@redhat.com>]

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.