* [PATCH] staging/dt3155: make copy_{to/from}_user and put_user typesafe
@ 2010-03-04 17:20 H Hartley Sweeten
2010-03-11 0:01 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: H Hartley Sweeten @ 2010-03-04 17:20 UTC (permalink / raw)
To: Linux Kernel; +Cc: greg, ss
This makes the kernel to user space data transfers typesafe and removes
a number of unnecessary (void *) casts.
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Greg Kroah-Hartman <greg@kroah.com>
Cc: Scott Smedley <ss@aao.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;
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)))
+ if (copy_from_user(&tmp, up, sizeof(tmp)))
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) ))
+ if (copy_to_user(up, &dt3155_status[minor], sizeof(dt3155_status_t)))
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);
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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging/dt3155: make copy_{to/from}_user and put_user typesafe
2010-03-04 17:20 [PATCH] staging/dt3155: make copy_{to/from}_user and put_user typesafe H Hartley Sweeten
@ 2010-03-11 0:01 ` Simon Horman
2010-03-11 0:19 ` H Hartley Sweeten
0 siblings, 1 reply; 4+ messages in thread
From: Simon Horman @ 2010-03-11 0:01 UTC (permalink / raw)
To: H Hartley Sweeten; +Cc: Linux Kernel, greg, ss
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 :-)
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: Greg Kroah-Hartman <greg@kroah.com>
> Cc: Scott Smedley <ss@aao.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;
>
> 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)))
> + if (copy_from_user(&tmp, up, sizeof(tmp)))
> 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) ))
> + if (copy_to_user(up, &dt3155_status[minor], sizeof(dt3155_status_t)))
> 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);
> 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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] staging/dt3155: make copy_{to/from}_user and put_user typesafe
2010-03-11 0:01 ` Simon Horman
@ 2010-03-11 0:19 ` H Hartley Sweeten
2010-03-11 3:20 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: H Hartley Sweeten @ 2010-03-11 0:19 UTC (permalink / raw)
To: Simon Horman; +Cc: Linux Kernel, greg, ss
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@visionengravers.com>
>> Cc: Greg Kroah-Hartman <greg@kroah.com>
>> Cc: Scott Smedley <ss@aao.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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging/dt3155: make copy_{to/from}_user and put_user typesafe
2010-03-11 0:19 ` H Hartley Sweeten
@ 2010-03-11 3:20 ` Simon Horman
0 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2010-03-11 3:20 UTC (permalink / raw)
To: H Hartley Sweeten; +Cc: Linux Kernel, greg, ss
On Wed, Mar 10, 2010 at 06:19:32PM -0600, H Hartley Sweeten wrote:
> 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@visionengravers.com>
> >> Cc: Greg Kroah-Hartman <greg@kroah.com>
> >> Cc: Scott Smedley <ss@aao.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.
Hi,
thanks for the clarification. I agree that your patch is an improvement
and I don't see any problems with it (other than typesafe possibly being the
wrong term).
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-11 3:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-04 17:20 [PATCH] staging/dt3155: make copy_{to/from}_user and put_user typesafe H Hartley Sweeten
2010-03-11 0:01 ` Simon Horman
2010-03-11 0:19 ` H Hartley Sweeten
2010-03-11 3:20 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®