linux-kernel - Re: [PATCH] staging: vt6655: tag data as _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140725122504.GA10945@pleinair.baobob.org>
Date:	Fri, 25 Jul 2014 14:25:06 +0200
From:	Guillaume CLÉMENT <gclement@...bob.org>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Guillaume Clement <gclement@...bob.org>,
	Forest Bond <forest@...ttletooquiet.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: vt6655: tag data as __user in struct
 tagSCmdRequest

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/