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.
About the missing + 2, and modifying the order of sorry about that, I
sent the patch too fast.
Thank you for your review, I will be submitting an updated patch with
your remarks.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
