by Dmitry Kalinkin

[permalink] [raw]

Subject: Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors

On Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch <[email protected]> wrote:
> On 26/06/15 21:39, Dmitry Kalinkin wrote:
>>
>> Signed-off-by: Dmitry Kalinkin <[email protected]>
>> ---
>> 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.
Yes it does. And this information is useless in userspace.
Returning -EFAULT would be more informative in this case.
I couldn't find an example of kernel code doing this any other way:
http://lxr.free-electrons.com/ident?i=__copy_to_user
>
>> }
>> @@ -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 [email protected] | VAT:GB 927559189

2015-07-06 13:52:21

by Martyn Welch

[permalink] [raw]

Subject: Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors

On 06/07/15 14:10, Dmitry Kalinkin wrote:
> On Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch <[email protected]> wrote:
>> On 26/06/15 21:39, Dmitry Kalinkin wrote:
>>>
>>> Signed-off-by: Dmitry Kalinkin <[email protected]>
>>> ---
>>> 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.
> Yes it does. And this information is useless in userspace.
> Returning -EFAULT would be more informative in this case.
> I couldn't find an example of kernel code doing this any other way:
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lxr.free-2Delectrons.com_ident-3Fi-3D-5F-5Fcopy-5Fto-5Fuser&d=AwIBaQ&c=IV_clAzoPDE253xZdHuilRgztyh_RiV3wUrLrDQYWSI&r=o_jX-O9t9rDa5QmNKEzuslLxr8l2x1M2rHid0HdzmY4&m=YqOunx1QvC1jz8WjtHd_feWk5cdNWIVW0utSWesR8ag&s=BdRq0di7Bryi1LqA3LtL_Z9UeSHdpiYwghMElfWbR1A&e=

Fair enough. If that's the approach taken elsewhere, then I'm happy for
this to become consistent.

>>
>>> }
>>> @@ -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 [email protected] | VAT:GB 927559189

--
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 [email protected] | VAT:GB 927559189