Re: [PATCH 2/3] prctl_set_mm: Refactor checks from validate_prctl_map

[Kirill Tkhai]

On 30.04.2019 11:18, Michal Koutnà wrote:
> Despite comment of validate_prctl_map claims there are no capability
> checks, it is not completely true since commit 4d28df6152aa ("prctl:
> Allow local CAP_SYS_ADMIN changing exe_file"). Extract the check out of
> the function and make the function perform purely arithmetic checks.
> 
> This patch should not change any behavior, it is mere refactoring for
> following patch.
> 
> CC: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>
> CC: Cyrill Gorcunov <gorcunov@xxxxxxxxx>
> Signed-off-by: Michal Koutnà <mkoutny@xxxxxxxx>

Reviewed-by: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>

> ---
>  kernel/sys.c | 45 ++++++++++++++++++++-------------------------
>  1 file changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 12df0e5434b8..e1acb444d7b0 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1882,10 +1882,12 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
>  }
>  
>  /*
> + * Check arithmetic relations of passed addresses.
> + *
>   * WARNING: we don't require any capability here so be very careful
>   * in what is allowed for modification from userspace.
>   */
> -static int validate_prctl_map(struct prctl_mm_map *prctl_map)
> +static int validate_prctl_map_addr(struct prctl_mm_map *prctl_map)
>  {
>  	unsigned long mmap_max_addr = TASK_SIZE;
>  	struct mm_struct *mm = current->mm;
> @@ -1949,24 +1951,6 @@ static int validate_prctl_map(struct prctl_mm_map *prctl_map)
>  			      prctl_map->start_data))
>  			goto out;
>  
> -	/*
> -	 * Someone is trying to cheat the auxv vector.
> -	 */
> -	if (prctl_map->auxv_size) {
> -		if (!prctl_map->auxv || prctl_map->auxv_size > sizeof(mm->saved_auxv))
> -			goto out;
> -	}
> -
> -	/*
> -	 * Finally, make sure the caller has the rights to
> -	 * change /proc/pid/exe link: only local sys admin should
> -	 * be allowed to.
> -	 */
> -	if (prctl_map->exe_fd != (u32)-1) {
> -		if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN))
> -			goto out;
> -	}
> -
>  	error = 0;
>  out:
>  	return error;
> @@ -1993,11 +1977,17 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data
>  	if (copy_from_user(&prctl_map, addr, sizeof(prctl_map)))
>  		return -EFAULT;
>  
> -	error = validate_prctl_map(&prctl_map);
> +	error = validate_prctl_map_addr(&prctl_map);
>  	if (error)
>  		return error;
>  
>  	if (prctl_map.auxv_size) {
> +		/*
> +		 * Someone is trying to cheat the auxv vector.
> +		 */
> +		if (!prctl_map.auxv || prctl_map.auxv_size > sizeof(mm->saved_auxv))
> +			return -EINVAL;
> +
>  		memset(user_auxv, 0, sizeof(user_auxv));
>  		if (copy_from_user(user_auxv,
>  				   (const void __user *)prctl_map.auxv,
> @@ -2010,6 +2000,14 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data
>  	}
>  
>  	if (prctl_map.exe_fd != (u32)-1) {
> +		/*
> +		 * Make sure the caller has the rights to
> +		 * change /proc/pid/exe link: only local sys admin should
> +		 * be allowed to.
> +		 */
> +		if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN))
> +			return -EINVAL;
> +
>  		error = prctl_set_mm_exe_file(mm, prctl_map.exe_fd);
>  		if (error)
>  			return error;
> @@ -2097,7 +2095,7 @@ static int prctl_set_mm(int opt, unsigned long addr,
>  			unsigned long arg4, unsigned long arg5)
>  {
>  	struct mm_struct *mm = current->mm;
> -	struct prctl_mm_map prctl_map;
> +	struct prctl_mm_map prctl_map = { .auxv = NULL, .auxv_size = 0, .exe_fd = -1 };
>  	struct vm_area_struct *vma;
>  	int error;
>  
> @@ -2139,9 +2137,6 @@ static int prctl_set_mm(int opt, unsigned long addr,
>  	prctl_map.arg_end	= mm->arg_end;
>  	prctl_map.env_start	= mm->env_start;
>  	prctl_map.env_end	= mm->env_end;
> -	prctl_map.auxv		= NULL;
> -	prctl_map.auxv_size	= 0;
> -	prctl_map.exe_fd	= -1;
>  
>  	switch (opt) {
>  	case PR_SET_MM_START_CODE:
> @@ -2181,7 +2176,7 @@ static int prctl_set_mm(int opt, unsigned long addr,
>  		goto out;
>  	}
>  
> -	error = validate_prctl_map(&prctl_map);
> +	error = validate_prctl_map_addr(&prctl_map);
>  	if (error)
>  		goto out;
>  
>