Re: [patch cr 4/4] c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries

Andrew Morton
2012-02-02T23:27:11Z

On Mon, 30 Jan 2012 18:09:09 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> After restore we would like the 'ps' command show the command
> line and evironment exactly the same it was at checkpoint time.
>
> So this additional PR_SET_MM_ allow us to do so. Note that
> these members of mm_struct is rather used for output in
> procfs, except auxv vector which is used by ld.so mostly.

This changelog is pretty darned hard to understand. Can we have a
version 2 please?

>
> ...
>
> @@ -1753,19 +1755,6 @@ static int prctl_set_mm(int opt, unsigne
> mm->end_data = addr;
> break;
>
> - case PR_SET_MM_START_STACK:
> -
> -#ifdef CONFIG_STACK_GROWSUP
> - vm_req_flags = VM_READ | VM_WRITE | VM_GROWSUP;
> -#else
> - vm_req_flags = VM_READ | VM_WRITE | VM_GROWSDOWN;
> -#endif
> - if ((vma->vm_flags & vm_req_flags) != vm_req_flags)
> - goto out;
> -
> - mm->start_stack = addr;
> - break;
> -
> case PR_SET_MM_START_BRK:
> if (addr <= mm->end_data)
> goto out;
> @@ -1790,16 +1779,53 @@ static int prctl_set_mm(int opt, unsigne
> mm->brk = addr;
> break;

Here would be a good place to add some nice comments explaining what
these do. Although I guess that isn't needed if one can get that info
by typing "man prctl".

> + case PR_SET_MM_START_STACK:
> + case PR_SET_MM_ARG_START:
> + case PR_SET_MM_ARG_END:
> + case PR_SET_MM_ENV_START:
> + case PR_SET_MM_ENV_END:
> +#ifdef CONFIG_STACK_GROWSUP
> + if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSUP, 0))
> +#else
> + if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSDOWN, 0))
> +#endif
> + goto out;
> + if (opt == PR_SET_MM_START_STACK)
> + mm->start_stack = addr;
> + else if (opt == PR_SET_MM_ARG_START)
> + mm->arg_start = addr;
> + else if (opt == PR_SET_MM_ARG_END)
> + mm->arg_end = addr;
> + else if (opt == PR_SET_MM_ENV_START)
> + mm->env_start = addr;
> + else if (opt == PR_SET_MM_ENV_END)
> + mm->env_end = addr;
> + break;
> +
> + case PR_SET_MM_AUXV: {
> + unsigned long user_auxv[AT_VECTOR_SIZE];
> +
> + if (arg4 > sizeof(mm->saved_auxv))
> + goto out;
> + up_read(&mm->mmap_sem);
> +
> + if (copy_from_user(user_auxv, (const void __user *)addr, arg4))
> + return EFAULT;
> +
> + task_lock(current);
> + memcpy(mm->saved_auxv, user_auxv, arg4);
> + task_unlock(current);
> +
> + return 0;
> + }

I worry a bit about this. We're giving userspace the ability to modify
various mm_struct fields. Userspace can already do this via
exec(elf-file), but perhaps this opens up a way in which userspace can
newly trigger kernel bugs.