| 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
| ||
|
Message-ID: <CAM41TOuNWbwoO6g6reuaqm_oNgD7G9XMPystQRqLk+qOdCrG5A@mail.gmail.com>
Date: Mon, 6 Jul 2015 16:10:53 +0300
From: Dmitry Kalinkin <dmitry.kalinkin@...il.com>
To: Martyn Welch <martyn.welch@...com>
Cc: linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
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 Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch <martyn.welch@...com> wrote:
> 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.
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 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/
Powered by blists - more mailing lists