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

From: Oleg Nesterov
Date: Fri Feb 03 2012 - 11:11:40 EST


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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/