staging: vt6655: wpactl.c: Fix sparse warnings

PATCH In review Martin Berglund 2 participants 7 messages Aug 7, 2014 – Aug 10, 2014 Staging

Share & tools

Martin Berglund

author

2014-08-07 22:18 UTC #0 lore raw

PATCH 1/1 staging: vt6655: wpactl.c: Fix sparse warnings

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 <[email protected]>

---
This was submitted as part of Eudyptula challenge task 16

 drivers/staging/vt6655/wpactl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/staging/vt6655/wpactl.c +6 -2

index 5f454ca..d75dd79 100644
@@ -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;
 		}

Signature · 1 line

1.7.10.4

Greg Kroah-Hartman

maintainer

2014-08-08 02:18 UTC #1 lore raw

Quoted 17 lines · 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 <[email protected]>
---
This was submitted as part of Eudyptula challenge task 16
 drivers/staging/vt6655/wpactl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vt6655/wpactl.c +0 -0
index 5f454ca..d75dd79 100644

drivers/staging/vt6655/wpactl.c int wpa_set_keys(PSDevice pDevice, void *ctx, · line 228

Quoted 6 lines

drivers/staging/vt6655/wpactl.c +2 -1

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

Martin Berglund

author

2014-08-08 07:10 UTC #2 lore raw

Quoted 28 lines · Greg Kroah-Hartman

Quoted 17 lines · 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 <[email protected]>
---
This was submitted as part of Eudyptula challenge task 16
 drivers/staging/vt6655/wpactl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vt6655/wpactl.c +0 -0
index 5f454ca..d75dd79 100644
drivers/staging/vt6655/wpactl.c int wpa_set_keys(PSDevice pDevice, void *ctx, · line 228
Quoted 6 lines
drivers/staging/vt6655/wpactl.c +2 -1
@@ -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?

Quoted 3 lines

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

Greg Kroah-Hartman

maintainer

2014-08-08 13:47 UTC #3 lore raw

Quoted 29 lines · Martin Berglund

Quoted 28 lines · Greg Kroah-Hartman
Quoted 17 lines · 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 <[email protected]>
---
This was submitted as part of Eudyptula challenge task 16
 drivers/staging/vt6655/wpactl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vt6655/wpactl.c +0 -0
index 5f454ca..d75dd79 100644
drivers/staging/vt6655/wpactl.c int wpa_set_keys(PSDevice pDevice, void *ctx, · line 228
Quoted 6 lines
drivers/staging/vt6655/wpactl.c +2 -1
@@ -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?

Quoted 8 lines

Quoted 3 lines
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...

Quoted 4 lines

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

Martin Berglund

author

2014-08-08 23:55 UTC #4 lore raw

Quoted 49 lines · Greg Kroah-Hartman

Quoted 29 lines · Martin Berglund
Quoted 28 lines · Greg Kroah-Hartman
Quoted 17 lines · 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 <[email protected]>
---
This was submitted as part of Eudyptula challenge task 16
 drivers/staging/vt6655/wpactl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vt6655/wpactl.c +0 -0
index 5f454ca..d75dd79 100644
drivers/staging/vt6655/wpactl.c int wpa_set_keys(PSDevice pDevice, void *ctx, · line 228
Quoted 6 lines
drivers/staging/vt6655/wpactl.c +2 -1
@@ -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?
Quoted 8 lines
Quoted 3 lines
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...
Quoted 4 lines
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...

Quoted 6 lines

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

Greg Kroah-Hartman

maintainer

2014-08-10 12:11 UTC #5 lore raw

Quoted 53 lines · Martin Berglund

Quoted 52 lines · Greg Kroah-Hartman
Quoted 29 lines · Martin Berglund
Quoted 28 lines · Greg Kroah-Hartman
Quoted 17 lines · 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 <[email protected]>
---
This was submitted as part of Eudyptula challenge task 16
 drivers/staging/vt6655/wpactl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vt6655/wpactl.c +0 -0
index 5f454ca..d75dd79 100644
drivers/staging/vt6655/wpactl.c int wpa_set_keys(PSDevice pDevice, void *ctx, · line 228
Quoted 6 lines
drivers/staging/vt6655/wpactl.c +2 -1
@@ -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?
Quoted 8 lines
Quoted 3 lines
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...
Quoted 4 lines
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?

Quoted 7 lines

Quoted 3 lines
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?

Quoted 2 lines

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?

Quoted 3 lines

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

Martin Berglund

author

2014-08-10 13:28 UTC #6 lore raw

Quoted 56 lines · Greg Kroah-Hartman

Quoted 55 lines · Martin Berglund
Quoted 54 lines · Greg Kroah-Hartman
Quoted 29 lines · Martin Berglund
Quoted 28 lines · Greg Kroah-Hartman
Quoted 17 lines · 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 <[email protected]>
---
This was submitted as part of Eudyptula challenge task 16
 drivers/staging/vt6655/wpactl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vt6655/wpactl.c +0 -0
index 5f454ca..d75dd79 100644
drivers/staging/vt6655/wpactl.c int wpa_set_keys(PSDevice pDevice, void *ctx, · line 228
Quoted 6 lines
drivers/staging/vt6655/wpactl.c +2 -1
@@ -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?
Quoted 8 lines
Quoted 3 lines
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...
Quoted 4 lines
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,

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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...

Quoted 5 lines

Quoted 3 lines
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.

Quoted 9 lines

Quoted 4 lines
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