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
| ||
|
Date: Wed, 10 Mar 2010 18:19:32 -0600 From: H Hartley Sweeten <hartleys@...ionengravers.com> To: Simon Horman <horms@...ge.net.au> CC: Linux Kernel <linux-kernel@...r.kernel.org>, "greg@...ah.com" <greg@...ah.com>, "ss@....gov.au" <ss@....gov.au> Subject: RE: [PATCH] staging/dt3155: make copy_{to/from}_user and put_user typesafe On Wednesday, March 10, 2010 5:01 PM, Simon Horman wrote: > On Thu, Mar 04, 2010 at 10:20:18AM -0700, H Hartley Sweeten wrote: >> This makes the kernel to user space data transfers typesafe and removes >> a number of unnecessary (void *) casts. > > Could you be more specific about what you mean by typesafe? > There still seem to be casts in there. Although fewer than before :-) >From include/asm-generic/uaccess.h static inline long copy_from_user(void *to, const void __user * from, unsigned long n) { might_sleep(); if (access_ok(VERIFY_READ, from, n)) return __copy_from_user(to, from, n); else return n; } static inline long copy_to_user(void __user *to, const void *from, unsigned long n) { might_sleep(); if (access_ok(VERIFY_WRITE, to, n)) return __copy_to_user(to, from, n); else return n; } >> Signed-off-by: H Hartley Sweeten <hsweeten@...ionengravers.com> >> Cc: Greg Kroah-Hartman <greg@...ah.com> >> Cc: Scott Smedley <ss@....gov.au> >> >> --- >> >> diff --git a/drivers/staging/dt3155/dt3155_drv.c b/drivers/staging/dt3155/dt3155_drv.c >> index a67c622..76c9d94 100644 >> --- a/drivers/staging/dt3155/dt3155_drv.c >> +++ b/drivers/staging/dt3155/dt3155_drv.c >> @@ -528,6 +528,7 @@ static int dt3155_ioctl(struct inode *inode, >> unsigned long arg) >> { >> int minor = MINOR(inode->i_rdev); /* What device are we ioctl()'ing? */ >> + void __user *up = (void __user *)arg; This cast is needed, and common, since 'arg' is passed as an unsigned long. The cast to __user memory space does not produce a sparse error. >> >> if ( minor >= MAXBOARDS || minor < 0 ) >> return -ENODEV; >> @@ -554,7 +555,7 @@ static int dt3155_ioctl(struct inode *inode, >> >> { >> struct dt3155_config_s tmp; >> - if (copy_from_user((void *)&tmp, (void *) arg, sizeof(tmp))) The fist void * cast is unnecessary, the second produces a sparse warning since copy_from_user expects 'arg' to be a __user space pointer. >> + if (copy_from_user(&tmp, up, sizeof(tmp))) Using the 'up' (user pointer) variable quiets the warning and makes the function call typesafe (ok maybe that's the wrong term...). >> return -EFAULT; >> /* check for valid settings */ >> if (tmp.rows > DT3155_MAX_ROWS || >> @@ -572,8 +573,7 @@ static int dt3155_ioctl(struct inode *inode, >> } >> case DT3155_GET_CONFIG: >> { >> - if (copy_to_user((void *) arg, (void *) &dt3155_status[minor], >> - sizeof(dt3155_status_t) )) Here 'arg' should be a __user space pointer and the second cast is not necessary. >> + if (copy_to_user(up, &dt3155_status[minor], sizeof(dt3155_status_t))) Again, using 'up' fixes the sparse warning. The two below are the same. >> return -EFAULT; >> return 0; >> } >> @@ -593,8 +593,7 @@ static int dt3155_ioctl(struct inode *inode, >> return 0; >> >> quick_stop(minor); >> - if (copy_to_user((void *) arg, (void *) &dt3155_status[minor], >> - sizeof(dt3155_status_t))) >> + if (copy_to_user(up, &dt3155_status[minor], sizeof(dt3155_status_t))) >> return -EFAULT; >> return 0; >> } >> @@ -617,8 +616,7 @@ static int dt3155_ioctl(struct inode *inode, >> } >> >> dt3155_init_isr(minor); >> - if (copy_to_user( (void *) arg, (void *) &dt3155_status[minor], >> - sizeof(dt3155_status_t))) >> + if (copy_to_user(up, &dt3155_status[minor], sizeof(dt3155_status_t))) >> return -EFAULT; >> return 0; >> } >> @@ -819,11 +817,11 @@ static ssize_t dt3155_read(struct file *filep, char __user *buf, >> /* make this an offset */ >> offset = frame_info_p->addr - dt3155_status[minor].mem_addr; >> >> - put_user(offset, (unsigned int *) buf); >> + put_user(offset, (unsigned int __user *)buf); For put_user, the second argument should be a __user pointer. 'buf' comes into the function correctly (char __user *) but needs the cast here so that the correct 'size' of data is copied to __user space. Same below. >> buf += sizeof(u32); >> - put_user( dt3155_status[minor].fbuffer.frame_count, (unsigned int *) buf); >> + put_user( dt3155_status[minor].fbuffer.frame_count, (unsigned int __user *)buf); >> buf += sizeof(u32); >> - put_user(dt3155_status[minor].state, (unsigned int *) buf); >> + put_user(dt3155_status[minor].state, (unsigned int __user *)buf); >> buf += sizeof(u32); >> if (copy_to_user(buf, frame_info_p, sizeof(frame_info_t))) >> return -EFAULT; All of this seems pretty common in other kernel code. And it removes a number of the sparse warnings generated by this file. Regards, Hartley -- 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