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

[Martin Berglund <martin@rogsta.net>]

On Sat, Aug 09, 2014 at 08:36:55PM -0700, Greg Kroah-Hartman wrote:
> On Sat, Aug 09, 2014 at 01:55:19AM +0200, Martin Berglund wrote:
> > On Fri, Aug 08, 2014 at 06:47:25AM -0700, Greg Kroah-Hartman wrote:
> > > 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@rogsta.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
> > 
> > I've looked into this driver some more now. It's definitely messy but not 
> > as bad as I said in my previous mail. I could not find any instances where 
> > copy_to/from_user was needed (the pointers were actually copied arrays).
> 
> Ok, then should the pointer just be marked as __user instead of casting
> it here?

No, this pointer is still used in both contexts. And as far as I know it's not 
possible to cast a __user marked variable back to kernel space without new 
warnings by sparse. I might be wrong though...

>    
> > As to solving it the same way as vt6656 was solved, of some reason vt6656
> > has no function for ndo_do_ioctl, and therefore no need for the ioctl-part.
> 
> Could it be that this function isn't needed here either?

I could not say if this function is redundant... This function is however 
linked into the module, so it is not unused in that sense.
 
> 
> > I just submitted a very similar patch to solve the last two address space
> > warnings in the vt6655 driver left after this patch.
> > https://lkml.org/lkml/2014/8/8/960
> 
> So do you think I still need to apply this patch, even after applying
> your other one?
> 
> confused,

Yes, this patch still stands. I linked the patch in relation to the discussion 
about the need to replace other memcpy in the driver. Just ignore that I
mentioned it here. Sorry for the confusion.

Cheers,
                  Martin