On Friday, July 30, 2010 4:07 AM, Kulikov Vasiliy wrote:
> put_user() may fail, if so return -EFAULT.
> Also compare count with copied data size, not size of struct with these
> fields.
>
> Signed-off-by: Kulikov Vasiliy <segooon@xxxxxxxxx>
> ---
> drivers/staging/dt3155/dt3155_drv.c | 121 ++++++++++++++++------------------
> 1 files changed, 57 insertions(+), 64 deletions(-)
As Jiri mentioned, whitespace cleanups should be a separate patch.
Also, please base your patch on the current linux-next tree. This patch will
not apply to the current source.
I do have a couple more comments below.
> diff --git a/drivers/staging/dt3155/dt3155_drv.c b/drivers/staging/dt3155/dt3155_drv.c
> index fed7e62..cfe4fae 100644
> --- a/drivers/staging/dt3155/dt3155_drv.c
> +++ b/drivers/staging/dt3155/dt3155_drv.c
> @@ -737,77 +737,70 @@ static int dt3155_close(struct inode *inode, struct file *filep)
> static ssize_t dt3155_read(struct file *filep, char __user *buf,
> size_t count, loff_t *ppos)
> {
> - /* which device are we reading from? */
> - int minor = MINOR(filep->f_dentry->d_inode->i_rdev);
> - u32 offset;
> - int frame_index;
> - struct dt3155_status *dts = &dt3155_status[minor];
> - struct dt3155_fbuffer *fb = &dts->fbuffer;
> - struct frame_info *frame_info;
> -
> - /* TODO: this should check the error flag and */
> - /* return an error on hardware failures */
> - if (count != sizeof(struct dt3155_read))
> - {
> - printk("DT3155 ERROR (NJC): count is not right\n");
> - return -EINVAL;
> - }
> -
> -
> - /* Hack here -- I'm going to allow reading even when idle.
> - * this is so that the frames can be read after STOP has
> - * been called. Leaving it here, commented out, as a reminder
> - * for a short while to make sure there are no problems.
> - * Note that if the driver is not opened in non_blocking mode,
> - * and the device is idle, then it could sit here forever! */
> + /* which device are we reading from? */
> + int minor = MINOR(filep->f_dentry->d_inode->i_rdev);
> + u32 offset;
> + int frame_index;
> + struct dt3155_status *dts = &dt3155_status[minor];
> + struct dt3155_fbuffer *fb = &dts->fbuffer;
> + struct frame_info *frame_info;
> + u32 *buffer = (u32 *)buf;
Why are you recasting the __user buffer? This will cause sparse warnings like:
warning: cast removes address space of expression
warning: incorrect type in argument 1 (different address spaces)
expected void const volatile [noderef] <asn:1>*<noident>
got unsigned int [usertype] *buffer
For each put_user and copy_to_user call.
> +
> + /* TODO: this should check the error flag and */
> + /* return an error on hardware failures */
> + if (count != 4*3 + sizeof(struct frame_info)) {
> + pr_err("DT3155 ERROR (NJC): count is not right\n");
> + return -EINVAL;
> + }
This is prone to breakage. It's safer to check against the struct size. Also,
the 4*3 is a bit ambiguous.
>
> - /* if (dts->state == DT3155_STATE_IDLE)*/
> - /* return -EBUSY;*/
>
> - /* non-blocking reads should return if no data */
> - if (filep->f_flags & O_NDELAY)
> - {
> - if ((frame_index = dt3155_get_ready_buffer(minor)) < 0) {
> - /*printk("dt3155: no buffers available (?)\n");*/
> - /* printques(minor); */
> - return -EAGAIN;
> - }
> - }
> - else
> - {
> - /*
> - * sleep till data arrives , or we get interrupted.
> - * Note that wait_event_interruptible() does not actually
> - * sleep/wait if it's condition evaluates to true upon entry.
> - */
> - wait_event_interruptible(dt3155_read_wait_queue[minor],
> - (frame_index = dt3155_get_ready_buffer(minor))
> - >= 0);
> -
> - if (frame_index < 0)
> - {
> - printk ("DT3155: read: interrupted\n");
> - quick_stop (minor);
> - printques(minor);
> - return -EINTR;
> + /* Hack here -- I'm going to allow reading even when idle.
> + * this is so that the frames can be read after STOP has
> + * been called. Leaving it here, commented out, as a reminder
> + * for a short while to make sure there are no problems.
> + * Note that if the driver is not opened in non_blocking mode,
> + * and the device is idle, then it could sit here forever! */
> +
> + /* if (dts->state == DT3155_STATE_IDLE)*/
> + /* return -EBUSY;*/
> +
> + /* non-blocking reads should return if no data */
> + if (filep->f_flags & O_NDELAY) {
> + frame_index = dt3155_get_ready_buffer(minor);
> + if (frame_index < 0)
> + /*printk("dt3155: no buffers available (?)\n");*/
> + /* printques(minor); */
> + return -EAGAIN;
> + } else {
> + /*
> + * sleep till data arrives , or we get interrupted.
> + * Note that wait_event_interruptible() does not actually
> + * sleep/wait if it's condition evaluates to true upon entry.
> + */
> + wait_event_interruptible(dt3155_read_wait_queue[minor],
> + (frame_index = dt3155_get_ready_buffer(minor))
> + >= 0);
> +
> + if (frame_index < 0) {
> + pr_info("DT3155: read: interrupted\n");
> + quick_stop(minor);
> + printques(minor);
> + return -EINTR;
> + }
> }
> - }
>
> - frame_info = &fb->frame_info[frame_index];
> + frame_info = &fb->frame_info[frame_index];
>
> - /* make this an offset */
> - offset = frame_info->addr - dts->mem_addr;
> + /* make this an offset */
> + offset = frame_info->addr - dts->mem_addr;
>
> - put_user(offset, (unsigned int __user *)buf);
> - buf += sizeof(u32);
> - put_user(fb->frame_count, (unsigned int __user *)buf);
> - buf += sizeof(u32);
> - put_user(dts->state, (unsigned int __user *)buf);
> - buf += sizeof(u32);
> - if (copy_to_user(buf, frame_info, sizeof(*frame_info)))
> - return -EFAULT;
> + if (put_user(offset, buffer) ||
> + put_user(fb->frame_count, buffer + 1) ||
> + put_user(dts->state, buffer + 2) ||
> + copy_to_user(buffer + 3, frame_info, sizeof(*frame_info)))
> + return -EFAULT;
It would be better to just create a temporary 'struct dt3155_read' variable, fill
in the fields, and then just do one copy_to_user(). That's basically what the
three put_user calls followed by the copy_to_user() are doing. By using a local
variable you also prevent any problems with the data order getting changed.
>
> - return sizeof(struct dt3155_read);
> + return count;
> }
>
> static unsigned int dt3155_poll (struct file * filp, poll_table *wait)
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
