linux-kernel - Re: [PATCH] staging: vt6655: wpactl.c: Fix sparse warnings

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]

Date:	Fri, 8 Aug 2014 06:47:25 -0700
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Martin Berglund <martin@...sta.net>
Cc:	devel@...verdev.osuosl.org,
	Forest Bond <forest@...ttletooquiet.net>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: vt6655: wpactl.c: Fix sparse warnings

On Fri, Aug 08, 2014 at 09:07:55AM +0200, Martin Berglund wrote:
> On Thu, Aug 07, 2014 at 07:18:13PM -0700, Greg Kroah-Hartman wrote:
> > On Thu, Aug 07, 2014 at 11:08:34PM +0100, Martin Berglund wrote:
> > > Add missing __user macro casting in the function wpa_set_keys.
> > > This is okay since the function handles the possibility of
> > > param->u.wpa_key.key and param->u.wpa_key.seq pointing to
> > > kernelspace using a flag, fcpfkernel.
> > > 
> > > Signed-off-by: Martin Berglund <martin@...sta.net>
> > > ---
> > > This was submitted as part of Eudyptula challenge task 16
> > > 
> > >  drivers/staging/vt6655/wpactl.c |    8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vt6655/wpactl.c b/drivers/staging/vt6655/wpactl.c
> > > index 5f454ca..d75dd79 100644
> > > --- a/drivers/staging/vt6655/wpactl.c
> > > +++ b/drivers/staging/vt6655/wpactl.c
> > > @@ -224,7 +224,9 @@ int wpa_set_keys(PSDevice pDevice, void *ctx,
> > >  	} else {
> > >  		spin_unlock_irq(&pDevice->lock);
> > >  		if (param->u.wpa_key.key &&
> > > -		    copy_from_user(&abyKey[0], param->u.wpa_key.key, param->u.wpa_key.key_len)) {
> > > +		    copy_from_user(&abyKey[0],
> > > +				   (void __user *)param->u.wpa_key.key,
> > 
> > Would it be better to mark this pointer as __user in the structure
> > itself?  Or is it also used as a kernel structure in other places?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Yes, the structure is used as a kernel structure in some other places. 
> Even this function is sometimes called with the pointers in the
> structure pointing to kernel memory. However, that is correctly
> handled with a flag also sent to the function.

Ugh, that's a mess.  And should be cleaned up...

> As a side note: there are some uses of memcpy in this file that
> might be good to switch to copy_from/to_user but it's not as clear
> to me if these pointers never can point to kernel memory (because of
> the mixing of the two). For example all copying of ssid and bssid.

That also is not good, if memcpy is used for userspace memory pointers,
bad things can happen on some machines...

Look at how this was fixed up in the other staging vt* driver, odds are
you can do the same thing here, right?

thanks,

greg k-h
--
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/