linux-kernel - Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <559A79C6.4020601@ge.com>
Date:	Mon, 6 Jul 2015 13:51:18 +0100
From:	Martyn Welch <martyn.welch@...com>
To:	Dmitry Kalinkin <dmitry.kalinkin@...il.com>,
	<linux-kernel@...r.kernel.org>, <devel@...verdev.osuosl.org>
CC:	Manohar Vanga <manohar.vanga@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT
 on __copy_*_user errors

On 26/06/15 21:39, Dmitry Kalinkin wrote:
> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@...il.com>
> ---
>   drivers/staging/vme/devices/vme_user.c | 47 ++++++++--------------------------
>   1 file changed, 11 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
> index a2345db..ef876a4 100644
> --- a/drivers/staging/vme/devices/vme_user.c
> +++ b/drivers/staging/vme/devices/vme_user.c
> @@ -123,7 +123,6 @@ struct vme_user_vma_priv {
>   static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
>   				loff_t *ppos)
>   {
> -	ssize_t retval;
>   	ssize_t copied = 0;
>
>   	if (count > image[minor].size_buf)
> @@ -135,13 +134,8 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
>   	if (copied < 0)
>   		return (int)copied;
>
> -	retval = __copy_to_user(buf, image[minor].kern_buf,
> -				(unsigned long)copied);
> -	if (retval != 0) {
> -		copied = (copied - retval);
> -		pr_info("User copy failed\n");
> -		return -EINVAL;
> -	}
> +	if (__copy_to_user(buf, image[minor].kern_buf, (unsigned long)copied))
> +		return -EFAULT;
>
>   	return copied;

Does __copy_to_user() not return the number of bytes still to be copied? 
The above seems to add the assumption that __copy_to_user()
can't return part way through a copy.

>   }
> @@ -149,21 +143,16 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
>   static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
>   				  size_t count, loff_t *ppos)
>   {
> -	ssize_t retval;
>   	ssize_t copied = 0;
>
>   	if (count > image[minor].size_buf)
>   		count = image[minor].size_buf;
>
> -	retval = __copy_from_user(image[minor].kern_buf, buf,
> -				  (unsigned long)count);
> -	if (retval != 0)
> -		copied = (copied - retval);
> -	else
> -		copied = count;
> +	if (__copy_from_user(image[minor].kern_buf, buf, (unsigned long)count))
> +		return -EFAULT;
>

Same here.

>   	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
> -				  copied, *ppos);
> +				  count, *ppos);
>
>   	return copied;
>   }
> @@ -172,38 +161,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>   			      size_t count, loff_t *ppos)
>   {
>   	void *image_ptr;
> -	ssize_t retval;
>
>   	image_ptr = image[minor].kern_buf + *ppos;
> +	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
> +		return -EFAULT;
>

Ditto.

> -	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
> -	if (retval != 0) {
> -		retval = (count - retval);
> -		pr_warn("Partial copy to userspace\n");
> -	} else
> -		retval = count;
> -
> -	/* Return number of bytes successfully read */
> -	return retval;
> +	return count;
>   }
>
>   static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
>   				size_t count, loff_t *ppos)
>   {
>   	void *image_ptr;
> -	size_t retval;
>
>   	image_ptr = image[minor].kern_buf + *ppos;
> +	if (__copy_from_user(image_ptr, buf, (unsigned long)count))
> +		return -EFAULT;
>

And here.

> -	retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
> -	if (retval != 0) {
> -		retval = (count - retval);
> -		pr_warn("Partial copy to userspace\n");
> -	} else
> -		retval = count;
> -
> -	/* Return number of bytes successfully read */
> -	return retval;
> +	return count;
>   }
>
>   static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
>

-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch@...com                  | VAT:GB 927559189
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/