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 <[email protected]>
---
drivers/staging/dt3155/dt3155_drv.c | 121 ++++++++++++++++------------------
1 files changed, 57 insertions(+), 64 deletions(-)
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;
+
+ /* 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;
+ }
- /* 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;
- return sizeof(struct dt3155_read);
+ return count;
}
static unsigned int dt3155_poll (struct file * filp, poll_table *wait)
--
1.7.0.4
On 07/30/2010 01:07 PM, 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.
Please do whitespace cleanup separately. We cannot review this.
--
js