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

[Andrew Morton <akpm@linux-foundation.org>]

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.