[SPAM] [PATCH 1/9] staging: dt3155: check put_user() return value

H Hartley Sweeten hartleys at visionengravers.com
Fri Jul 30 16:47:47 UTC 2010


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 at gmail.com>
> ---
>  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


More information about the devel mailing list