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

[Vasiliy Kulikov]

On Fri, Jul 30, 2010 at 11:47 -0500, H Hartley Sweeten wrote:
> 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.

No problem, I'll be able to do it on Monday or a bit later.

> 
> I do have a couple more comments below.
> 

> > +	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.

Right, I've thought that it's just a zero macro, but now I see that
it is compiler __attribute__().

> 
> > +
> > +	/* 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.
> 

I think current variant is ambiguous too as it copies concrete data
chunks, but measures struct size. In generic case sizeof(struct) != sum
of sizeof(fields) (current case is ok).

> >  
> > -  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.

Yes, using struct for copying is the best variant IMO, but its second field
is named frame_seq, not frame_count:

struct dt3155_read {
	u32 offset;
	u32 frame_seq;
	u32 state;

	struct frame_info frame_info;
};

maybe it should be renamed to frame_count?

> 
> >  
> > -  return sizeof(struct dt3155_read);
> > +	return count;
> >  }
> >  
> >  static unsigned int dt3155_poll (struct file * filp, poll_table *wait)
> 
> Regards,
> Hartley