Re: [PATCH] mm/hmm/test: Fix some copy_to_user() error handling

[Ralph Campbell <rcampbell@xxxxxxxxxx>]

On 5/11/20 11:37 AM, Dan Carpenter wrote:
> The copy_to_user() function returns the number of bytes which weren't
> copied but we want to return negative error codes.  Also in dmirror_write()
> if the copy_from_user() fails then there is some cleanup needed before
> we can return so I fixed that as well.
>
> Fixes: 5d5e54be8a1e3 ("mm/hmm/test: add selftest driver for HMM")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>

Thanks for fixing this.
Reviewed-by: Ralph Campbell <rcampbell@xxxxxxxxxx>

> ---
>   lib/test_hmm.c | 41 +++++++++++++++++++++++++----------------
>   1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 00bca6116f930..fd4889f7b3d90 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -360,9 +360,11 @@ static int dmirror_read(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
>   		cmd->faults++;
>   	}
>   
> -	if (ret == 0)
> -		ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> -					bounce.size);
> +	if (ret == 0) {
> +		if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> +				 bounce.size))
> +			ret = -EFAULT;
> +	}
>   	cmd->cpages = bounce.cpages;
>   	dmirror_bounce_fini(&bounce);
>   	return ret;
> @@ -412,10 +414,11 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
>   	ret = dmirror_bounce_init(&bounce, start, size);
>   	if (ret)
>   		return ret;
> -	ret = copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
> -				bounce.size);
> -	if (ret)
> -		return ret;
> +	if (copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
> +			   bounce.size)) {
> +		ret = -EFAULT;
> +		goto fini;
> +	}
>   
>   	while (1) {
>   		mutex_lock(&dmirror->mutex);
> @@ -431,6 +434,7 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
>   		cmd->faults++;
>   	}
>   
> +fini:
>   	cmd->cpages = bounce.cpages;
>   	dmirror_bounce_fini(&bounce);
>   	return ret;
> @@ -715,9 +719,11 @@ static int dmirror_migrate(struct dmirror *dmirror,
>   	mutex_lock(&dmirror->mutex);
>   	ret = dmirror_do_read(dmirror, start, end, &bounce);
>   	mutex_unlock(&dmirror->mutex);
> -	if (ret == 0)
> -		ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> -					bounce.size);
> +	if (ret == 0) {
> +		if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> +				 bounce.size))
> +			ret = -EFAULT;
> +	}
>   	cmd->cpages = bounce.cpages;
>   	dmirror_bounce_fini(&bounce);
>   	return ret;
> @@ -886,9 +892,10 @@ static int dmirror_snapshot(struct dmirror *dmirror,
>   			break;
>   
>   		n = (range.end - range.start) >> PAGE_SHIFT;
> -		ret = copy_to_user(uptr, perm, n);
> -		if (ret)
> +		if (copy_to_user(uptr, perm, n)) {
> +			ret = -EFAULT;
>   			break;
> +		}
>   
>   		cmd->cpages += n;
>   		uptr += n;
> @@ -911,9 +918,8 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
>   	if (!dmirror)
>   		return -EINVAL;
>   
> -	ret = copy_from_user(&cmd, uarg, sizeof(cmd));
> -	if (ret)
> -		return ret;
> +	if (copy_from_user(&cmd, uarg, sizeof(cmd)))
> +		return -EFAULT;
>   
>   	if (cmd.addr & ~PAGE_MASK)
>   		return -EINVAL;
> @@ -946,7 +952,10 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
>   	if (ret)
>   		return ret;
>   
> -	return copy_to_user(uarg, &cmd, sizeof(cmd));
> +	if (copy_to_user(uarg, &cmd, sizeof(cmd)))
> +		return -EFAULT;
> +
> +	return 0;
>   }
>   
>   static const struct file_operations dmirror_fops = {