[PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest

Dan Carpenter dan.carpenter at oracle.com
Fri Jul 25 12:33:01 UTC 2014


On Fri, Jul 25, 2014 at 02:25:06PM +0200, Guillaume CLÉMENT wrote:
> On Fri, Jul 25, 2014 at 02:52:34PM +0300, Dan Carpenter wrote:
> > On Fri, Jul 25, 2014 at 01:07:40AM +0200, Guillaume Clement wrote:
> > > diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
> > > index 501cd64..9291259 100644
> > > --- a/drivers/staging/vt6655/iwctl.c
> > > +++ b/drivers/staging/vt6655/iwctl.c
> > > @@ -1621,14 +1621,17 @@ int iwctl_giwauth(struct net_device *dev,
> > >  int iwctl_siwgenie(struct net_device *dev,
> > >  		   struct iw_request_info *info,
> > >  		   struct iw_point *wrq,
> > > -		   char *extra)
> > > +		   char __user *extra)
> > >  {
> > >  	PSDevice			pDevice = (PSDevice)netdev_priv(dev);
> > >  	PSMgmtObject	pMgmt = &(pDevice->sMgmtObj);
> > >  	int ret = 0;
> > > +	char length;
> > >
> > >  	if (wrq->length) {
> > > -		if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> > > +		if (get_user(length, extra + 1))
> > > +			return -EFAULT;
> > > +		if ((wrq->length < 2) || (length != wrq->length)) {
> > >  			ret = -EINVAL;
> > >  			goto out;
> > >  		}
> >
> > Wow, this is confusing code.  The patch description isn't clear enough
> > that this is a bugfix patch and not just a "tag data" patch.
> >
> > I don't think this is correct.  We need to check the length of the input
> > buffer before we call get_user().  Can we return directly or do we
> > *need* to go to the mysteriously named "out"?  Also the + 2 is lost,
> > this would break everything if the current code works (not necessarily a
> > valid assumption).  Delete all my comments in the final code.
> 
> In this case, the "out" label just does "return ret;". But I agree this
> is better practice to jump to out, in case this changes at a later time,
> and to keep the code consistent.
>

No, absolutely not.  "out" labels are the worst.  We shouldn't make the
code unreadable and mysterious *now* just because of something which
is possible but frankly unlikely "at some later time".

Feel free to return directly if you want.

regards,
dan carpenter



More information about the devel mailing list