lkml.org 
[lkml]   [2015]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors
    From
    On Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch <martyn.welch@ge.com> wrote:
    > On 26/06/15 21:39, Dmitry Kalinkin wrote:
    >>
    >> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.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@ge.com | VAT:GB 927559189


    \
     
     \ /
      Last update: 2015-07-06 15:21    [W:3.882 / U:0.348 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site