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

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

[Martin Berglund]

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,
+				   param->u.wpa_key.key_len)) {
 			spin_lock_irq(&pDevice->lock);
 			return -EINVAL;
 		}
@@ -262,7 +264,9 @@ int wpa_set_keys(PSDevice pDevice, void *ctx,
 	} else {
 		spin_unlock_irq(&pDevice->lock);
 		if (param->u.wpa_key.seq &&
-		    copy_from_user(&abySeq[0], param->u.wpa_key.seq, param->u.wpa_key.seq_len)) {
+		    copy_from_user(&abySeq[0],
+				   (void __user *)param->u.wpa_key.seq,
+				   param->u.wpa_key.seq_len)) {
 			spin_lock_irq(&pDevice->lock);
 			return -EINVAL;
 		}
-- 
1.7.10.4

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

[Greg Kroah-Hartman]

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

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

[Martin Berglund]

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.

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.

Cheers,
                Martin

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

[Greg Kroah-Hartman]

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

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

[Martin Berglund]

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).

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.

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

Cheers,
                   Martin

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

[Greg Kroah-Hartman]

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?

> 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 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,

greg k-h

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

[Martin Berglund]

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