* [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest
@ 2014-07-24 23:07 Guillaume Clement
2014-07-25 11:52 ` Dan Carpenter
0 siblings, 1 reply; 12+ messages in thread
From: Guillaume Clement @ 2014-07-24 23:07 UTC (permalink / raw)
To: Forest Bond, Greg Kroah-Hartman; +Cc: devel, linux-kernel, Guillaume Clement
Sparse reported that the data from tagSCmdRequest is given by
userspace, so it should be tagged as such.
Later, we were memcomparing and dereferencing it without first copying
it, fix that as well.
Signed-off-by: Guillaume Clement <gclement@baobob.org>
---
drivers/staging/vt6655/iocmd.h | 2 +-
drivers/staging/vt6655/iwctl.c | 22 +++++++++++++++-------
drivers/staging/vt6655/iwctl.h | 6 +++---
3 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/vt6655/iocmd.h b/drivers/staging/vt6655/iocmd.h
index e499f1b..dd12498 100644
--- a/drivers/staging/vt6655/iocmd.h
+++ b/drivers/staging/vt6655/iocmd.h
@@ -100,7 +100,7 @@ typedef enum tagWZONETYPE {
#pragma pack(1)
typedef struct tagSCmdRequest {
u8 name[16];
- void *data;
+ void __user *data;
u16 wResult;
u16 wCmdCode;
} SCmdRequest, *PSCmdRequest;
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;
}
@@ -1654,7 +1657,7 @@ out://not completely ...not necessary in wpa_supplicant 0.5.8
int iwctl_giwgenie(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);
@@ -1801,18 +1804,23 @@ int iwctl_giwencodeext(struct net_device *dev,
int iwctl_siwmlme(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);
- struct iw_mlme *mlme = (struct iw_mlme *)extra;
+ struct iw_mlme mime;
+
int ret = 0;
- if (memcmp(pMgmt->abyCurrBSSID, mlme->addr.sa_data, ETH_ALEN)) {
+ ret = copy_from_user(&mime, extra, sizeof(mime));
+ if (ret)
+ return -EFAULT;
+
+ if (memcmp(pMgmt->abyCurrBSSID, mime.addr.sa_data, ETH_ALEN)) {
ret = -EINVAL;
return ret;
}
- switch (mlme->cmd) {
+ switch (mime.cmd) {
case IW_MLME_DEAUTH:
//this command seems to be not complete,please test it --einsnliu
//bScheduleCommand((void *) pDevice, WLAN_CMD_DEAUTH, (unsigned char *)&reason);
diff --git a/drivers/staging/vt6655/iwctl.h b/drivers/staging/vt6655/iwctl.h
index de0a337..7dd6310 100644
--- a/drivers/staging/vt6655/iwctl.h
+++ b/drivers/staging/vt6655/iwctl.h
@@ -176,12 +176,12 @@ 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);
int iwctl_giwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);
int iwctl_siwencodeext(struct net_device *dev,
struct iw_request_info *info,
@@ -196,7 +196,7 @@ int iwctl_giwencodeext(struct net_device *dev,
int iwctl_siwmlme(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);
#endif // #ifdef WPA_SUPPLICANT_DRIVER_WEXT_SUPPORT
//End Add -- //2008-0409-07, <Add> by Einsn Liu
--
1.8.5.5
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest
2014-07-24 23:07 [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest Guillaume Clement
@ 2014-07-25 11:52 ` Dan Carpenter
2014-07-25 12:25 ` Guillaume CLÉMENT
0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2014-07-25 11:52 UTC (permalink / raw)
To: Guillaume Clement; +Cc: Forest Bond, Greg Kroah-Hartman, devel, linux-kernel
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.
/* 2 because we skip the first byte and read length from the
second byte */
if (wrq->length < 2) {
ret = -EINVAL;
goto out;
}
ret = get_user(length, extra + 1);
if (ret)
goto out;
if (length + 2 != wrq->length) {
ret = -EINVAL;
goto out;
}
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest
2014-07-25 11:52 ` Dan Carpenter
@ 2014-07-25 12:25 ` Guillaume CLÉMENT
2014-07-25 12:33 ` Dan Carpenter
0 siblings, 1 reply; 12+ messages in thread
From: Guillaume CLÉMENT @ 2014-07-25 12:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: Guillaume Clement, Forest Bond, Greg Kroah-Hartman, devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest
2014-07-25 12:25 ` Guillaume CLÉMENT
@ 2014-07-25 12:33 ` Dan Carpenter
2014-07-25 12:47 ` [PATCH] staging: vt6655: fix direct dereferencing of user pointer Guillaume Clement
0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2014-07-25 12:33 UTC (permalink / raw)
To: Guillaume CLÉMENT
Cc: Forest Bond, Greg Kroah-Hartman, devel, linux-kernel
On Fri, Jul 25, 2014 at 02:25:06PM +0200, Guillaume CLÉMENT wrote:
> 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.
>
No, absolutely not. "out" labels are the worst. We shouldn't make the
code unreadable and mysterious *now* just because of something which
is possible but frankly unlikely "at some later time".
Feel free to return directly if you want.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] staging: vt6655: fix direct dereferencing of user pointer
2014-07-25 12:33 ` Dan Carpenter
@ 2014-07-25 12:47 ` Guillaume Clement
2014-07-25 23:09 ` Malcolm Priestley
2014-07-25 23:16 ` Malcolm Priestley
0 siblings, 2 replies; 12+ messages in thread
From: Guillaume Clement @ 2014-07-25 12:47 UTC (permalink / raw)
To: Forest Bond, Greg Kroah-Hartman
Cc: devel, Dan Carpenter, linux-kernel, Guillaume Clement
Sparse reported that the data from tagSCmdRequest is given by
userspace, so it should be tagged as such.
Later, we were memcomparing and dereferencing it without first copying
it, fix that as well.
Signed-off-by: Guillaume Clement <gclement@baobob.org>
---
drivers/staging/vt6655/iocmd.h | 2 +-
drivers/staging/vt6655/iwctl.c | 32 ++++++++++++++++++++++----------
drivers/staging/vt6655/iwctl.h | 6 +++---
3 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/vt6655/iocmd.h b/drivers/staging/vt6655/iocmd.h
index e499f1b..dd12498 100644
--- a/drivers/staging/vt6655/iocmd.h
+++ b/drivers/staging/vt6655/iocmd.h
@@ -100,7 +100,7 @@ typedef enum tagWZONETYPE {
#pragma pack(1)
typedef struct tagSCmdRequest {
u8 name[16];
- void *data;
+ void __user *data;
u16 wResult;
u16 wCmdCode;
} SCmdRequest, *PSCmdRequest;
diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
index 501cd64..7ce23b5 100644
--- a/drivers/staging/vt6655/iwctl.c
+++ b/drivers/staging/vt6655/iwctl.c
@@ -1621,17 +1621,24 @@ 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)) {
- ret = -EINVAL;
- goto out;
- }
+ if (wrq->length < 2)
+ return -EINVAL;
+
+ ret = get_user(length, extra + 1);
+ if (ret)
+ return ret;
+
+ if (length + 2 != wrq->length)
+ return -EINVAL;
+
if (wrq->length > MAX_WPA_IE_LEN) {
ret = -ENOMEM;
goto out;
@@ -1654,7 +1661,7 @@ out://not completely ...not necessary in wpa_supplicant 0.5.8
int iwctl_giwgenie(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);
@@ -1801,18 +1808,23 @@ int iwctl_giwencodeext(struct net_device *dev,
int iwctl_siwmlme(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);
- struct iw_mlme *mlme = (struct iw_mlme *)extra;
+ struct iw_mlme mime;
+
int ret = 0;
- if (memcmp(pMgmt->abyCurrBSSID, mlme->addr.sa_data, ETH_ALEN)) {
+ ret = copy_from_user(&mime, extra, sizeof(mime));
+ if (ret)
+ return -EFAULT;
+
+ if (memcmp(pMgmt->abyCurrBSSID, mime.addr.sa_data, ETH_ALEN)) {
ret = -EINVAL;
return ret;
}
- switch (mlme->cmd) {
+ switch (mime.cmd) {
case IW_MLME_DEAUTH:
//this command seems to be not complete,please test it --einsnliu
//bScheduleCommand((void *) pDevice, WLAN_CMD_DEAUTH, (unsigned char *)&reason);
diff --git a/drivers/staging/vt6655/iwctl.h b/drivers/staging/vt6655/iwctl.h
index de0a337..7dd6310 100644
--- a/drivers/staging/vt6655/iwctl.h
+++ b/drivers/staging/vt6655/iwctl.h
@@ -176,12 +176,12 @@ 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);
int iwctl_giwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);
int iwctl_siwencodeext(struct net_device *dev,
struct iw_request_info *info,
@@ -196,7 +196,7 @@ int iwctl_giwencodeext(struct net_device *dev,
int iwctl_siwmlme(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);
#endif // #ifdef WPA_SUPPLICANT_DRIVER_WEXT_SUPPORT
//End Add -- //2008-0409-07, <Add> by Einsn Liu
--
1.8.5.5
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer
2014-07-25 12:47 ` [PATCH] staging: vt6655: fix direct dereferencing of user pointer Guillaume Clement
@ 2014-07-25 23:09 ` Malcolm Priestley
2014-07-26 8:24 ` Guillaume CLÉMENT
2014-07-25 23:16 ` Malcolm Priestley
1 sibling, 1 reply; 12+ messages in thread
From: Malcolm Priestley @ 2014-07-25 23:09 UTC (permalink / raw)
To: Guillaume Clement, Forest Bond, Greg Kroah-Hartman
Cc: devel, linux-kernel, Dan Carpenter
Hi Guillaume
On 25/07/14 13:47, Guillaume Clement wrote:
> Sparse reported that the data from tagSCmdRequest is given by
> userspace, so it should be tagged as such.
extra is not in user space
All Wireless Extensions ioctl extra calls originate from
ioctl_standard_iw_point in wext-core.
Either through ioctl or iw_handler
All these functions should have been converted to iw_handler.
Regards
Malcolm
>
> Later, we were memcomparing and dereferencing it without first copying
> it, fix that as well.
>
> Signed-off-by: Guillaume Clement <gclement@baobob.org>
> ---
> drivers/staging/vt6655/iocmd.h | 2 +-
> drivers/staging/vt6655/iwctl.c | 32 ++++++++++++++++++++++----------
> drivers/staging/vt6655/iwctl.h | 6 +++---
> 3 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/vt6655/iocmd.h b/drivers/staging/vt6655/iocmd.h
> index e499f1b..dd12498 100644
> --- a/drivers/staging/vt6655/iocmd.h
> +++ b/drivers/staging/vt6655/iocmd.h
> @@ -100,7 +100,7 @@ typedef enum tagWZONETYPE {
> #pragma pack(1)
> typedef struct tagSCmdRequest {
> u8 name[16];
> - void *data;
> + void __user *data;
> u16 wResult;
> u16 wCmdCode;
> } SCmdRequest, *PSCmdRequest;
> diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
> index 501cd64..7ce23b5 100644
> --- a/drivers/staging/vt6655/iwctl.c
> +++ b/drivers/staging/vt6655/iwctl.c
> @@ -1621,17 +1621,24 @@ 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)) {
> - ret = -EINVAL;
> - goto out;
> - }
> + if (wrq->length < 2)
> + return -EINVAL;
> +
> + ret = get_user(length, extra + 1);
> + if (ret)
> + return ret;
> +
> + if (length + 2 != wrq->length)
> + return -EINVAL;
> +
> if (wrq->length > MAX_WPA_IE_LEN) {
> ret = -ENOMEM;
> goto out;
> @@ -1654,7 +1661,7 @@ out://not completely ...not necessary in wpa_supplicant 0.5.8
> int iwctl_giwgenie(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);
> @@ -1801,18 +1808,23 @@ int iwctl_giwencodeext(struct net_device *dev,
> int iwctl_siwmlme(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);
> - struct iw_mlme *mlme = (struct iw_mlme *)extra;
> + struct iw_mlme mime;
> +
> int ret = 0;
>
> - if (memcmp(pMgmt->abyCurrBSSID, mlme->addr.sa_data, ETH_ALEN)) {
> + ret = copy_from_user(&mime, extra, sizeof(mime));
> + if (ret)
> + return -EFAULT;
> +
> + if (memcmp(pMgmt->abyCurrBSSID, mime.addr.sa_data, ETH_ALEN)) {
> ret = -EINVAL;
> return ret;
> }
> - switch (mlme->cmd) {
> + switch (mime.cmd) {
> case IW_MLME_DEAUTH:
> //this command seems to be not complete,please test it --einsnliu
> //bScheduleCommand((void *) pDevice, WLAN_CMD_DEAUTH, (unsigned char *)&reason);
> diff --git a/drivers/staging/vt6655/iwctl.h b/drivers/staging/vt6655/iwctl.h
> index de0a337..7dd6310 100644
> --- a/drivers/staging/vt6655/iwctl.h
> +++ b/drivers/staging/vt6655/iwctl.h
> @@ -176,12 +176,12 @@ 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);
>
> int iwctl_giwgenie(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra);
> + char __user *extra);
>
> int iwctl_siwencodeext(struct net_device *dev,
> struct iw_request_info *info,
> @@ -196,7 +196,7 @@ int iwctl_giwencodeext(struct net_device *dev,
> int iwctl_siwmlme(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra);
> + char __user *extra);
> #endif // #ifdef WPA_SUPPLICANT_DRIVER_WEXT_SUPPORT
> //End Add -- //2008-0409-07, <Add> by Einsn Liu
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer
2014-07-25 12:47 ` [PATCH] staging: vt6655: fix direct dereferencing of user pointer Guillaume Clement
2014-07-25 23:09 ` Malcolm Priestley
@ 2014-07-25 23:16 ` Malcolm Priestley
1 sibling, 0 replies; 12+ messages in thread
From: Malcolm Priestley @ 2014-07-25 23:16 UTC (permalink / raw)
To: Guillaume Clement, Forest Bond, Greg Kroah-Hartman
Cc: devel, linux-kernel, Dan Carpenter
Hi Guillaume
On 25/07/14 13:47, Guillaume Clement wrote:
> Sparse reported that the data from tagSCmdRequest is given by
> userspace, so it should be tagged as such.
extra is not in user space
All Wireless Extensions ioctl extra calls originate from
ioctl_standard_iw_point in wext-core.
Either through ioctl or iw_handler
All these functions should have been converted to iw_handler.
Regards
Malcolm
>
> Later, we were memcomparing and dereferencing it without first copying
> it, fix that as well.
>
> Signed-off-by: Guillaume Clement <gclement@baobob.org>
> ---
> drivers/staging/vt6655/iocmd.h | 2 +-
> drivers/staging/vt6655/iwctl.c | 32 ++++++++++++++++++++++----------
> drivers/staging/vt6655/iwctl.h | 6 +++---
> 3 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/vt6655/iocmd.h b/drivers/staging/vt6655/iocmd.h
> index e499f1b..dd12498 100644
> --- a/drivers/staging/vt6655/iocmd.h
> +++ b/drivers/staging/vt6655/iocmd.h
> @@ -100,7 +100,7 @@ typedef enum tagWZONETYPE {
> #pragma pack(1)
> typedef struct tagSCmdRequest {
> u8 name[16];
> - void *data;
> + void __user *data;
> u16 wResult;
> u16 wCmdCode;
> } SCmdRequest, *PSCmdRequest;
> diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
> index 501cd64..7ce23b5 100644
> --- a/drivers/staging/vt6655/iwctl.c
> +++ b/drivers/staging/vt6655/iwctl.c
> @@ -1621,17 +1621,24 @@ 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)) {
> - ret = -EINVAL;
> - goto out;
> - }
> + if (wrq->length < 2)
> + return -EINVAL;
> +
> + ret = get_user(length, extra + 1);
> + if (ret)
> + return ret;
> +
> + if (length + 2 != wrq->length)
> + return -EINVAL;
> +
> if (wrq->length > MAX_WPA_IE_LEN) {
> ret = -ENOMEM;
> goto out;
> @@ -1654,7 +1661,7 @@ out://not completely ...not necessary in wpa_supplicant 0.5.8
> int iwctl_giwgenie(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);
> @@ -1801,18 +1808,23 @@ int iwctl_giwencodeext(struct net_device *dev,
> int iwctl_siwmlme(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);
> - struct iw_mlme *mlme = (struct iw_mlme *)extra;
> + struct iw_mlme mime;
> +
> int ret = 0;
>
> - if (memcmp(pMgmt->abyCurrBSSID, mlme->addr.sa_data, ETH_ALEN)) {
> + ret = copy_from_user(&mime, extra, sizeof(mime));
> + if (ret)
> + return -EFAULT;
> +
> + if (memcmp(pMgmt->abyCurrBSSID, mime.addr.sa_data, ETH_ALEN)) {
> ret = -EINVAL;
> return ret;
> }
> - switch (mlme->cmd) {
> + switch (mime.cmd) {
> case IW_MLME_DEAUTH:
> //this command seems to be not complete,please test it --einsnliu
> //bScheduleCommand((void *) pDevice, WLAN_CMD_DEAUTH, (unsigned char *)&reason);
> diff --git a/drivers/staging/vt6655/iwctl.h b/drivers/staging/vt6655/iwctl.h
> index de0a337..7dd6310 100644
> --- a/drivers/staging/vt6655/iwctl.h
> +++ b/drivers/staging/vt6655/iwctl.h
> @@ -176,12 +176,12 @@ 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);
>
> int iwctl_giwgenie(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra);
> + char __user *extra);
>
> int iwctl_siwencodeext(struct net_device *dev,
> struct iw_request_info *info,
> @@ -196,7 +196,7 @@ int iwctl_giwencodeext(struct net_device *dev,
> int iwctl_siwmlme(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra);
> + char __user *extra);
> #endif // #ifdef WPA_SUPPLICANT_DRIVER_WEXT_SUPPORT
> //End Add -- //2008-0409-07, <Add> by Einsn Liu
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer
2014-07-25 23:09 ` Malcolm Priestley
@ 2014-07-26 8:24 ` Guillaume CLÉMENT
2014-07-26 9:18 ` Guillaume CLÉMENT
0 siblings, 1 reply; 12+ messages in thread
From: Guillaume CLÉMENT @ 2014-07-26 8:24 UTC (permalink / raw)
To: Malcolm Priestley
Cc: Guillaume Clement, Forest Bond, Greg Kroah-Hartman, devel,
linux-kernel, Dan Carpenter
Hi Malcolm,
On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
> Hi Guillaume
>
> On 25/07/14 13:47, Guillaume Clement wrote:
> > Sparse reported that the data from tagSCmdRequest is given by
> > userspace, so it should be tagged as such.
> extra is not in user space
>
All right.
This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
an example, in device_main.c, we have this code:
rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
Here the extra parameter is the last one, wrq->u.data.pointer.
I was led to believe that wrq->u.data.pointer is in userspace (this was
reported by sparse actually) because the pointer field in data is
actually defined as __user.
I can understand it's not though, if it was pre-processed by another
function. Or if that pointer was never given by userspace in the first
place (if so, why would it be inside a __user pointer) ?
> All Wireless Extensions ioctl extra calls originate from
> ioctl_standard_iw_point in wext-core.
>
> Either through ioctl or iw_handler
After digging into the code a bit more, I think that there may still be
an issue. Are we really going through ioctl_standard_iw_point in this
specific case ?
In the iwctl_handler definition, we have no handler because of:
(iw_handler) NULL, // SIOCSIWGENIE
In the wireless_process_ioctl function, the returned handler should be
NULL if I understand correctly. I believe we're going through the "old
API" part with ndo_do_ioctl being called, which is consistent with the
fact that the code I showed earlier comes from that big switch in
device_ioctl in device_main.c.
This means we don't go to ioctl_standard_call, that would have called
ioctl_standard_iw_point, the function that should have done the
copy_from_user.
I didn't see a copy_from_user of the pointer field in the paths that I
think lead to device_ioctl in device_main.c.
Maybe the proper fix should have been to copy the content of
wrq->u.data.pointer to some buffer before calling iwctl_siwgenie, and
let this function not taking __user data?
This way, the driver is still ready to be converted to iw_handler
because it keeps the proper signature.
> All these functions should have been converted to iw_handler.
Yes certainly, but with no access to the real hardware for testing, I'd
rather not make deep changes like this for now.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer
2014-07-26 8:24 ` Guillaume CLÉMENT
@ 2014-07-26 9:18 ` Guillaume CLÉMENT
2014-07-26 10:44 ` Malcolm Priestley
0 siblings, 1 reply; 12+ messages in thread
From: Guillaume CLÉMENT @ 2014-07-26 9:18 UTC (permalink / raw)
To: Malcolm Priestley
Cc: Malcolm Priestley, Forest Bond, Greg Kroah-Hartman, devel,
linux-kernel, Dan Carpenter
On Sat, Jul 26, 2014 at 10:24:30AM +0200, Guillaume CLÉMENT wrote:
> Hi Malcolm,
>
> On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
> > Hi Guillaume
> >
> > On 25/07/14 13:47, Guillaume Clement wrote:
> > > Sparse reported that the data from tagSCmdRequest is given by
> > > userspace, so it should be tagged as such.
> > extra is not in user space
> >
>
> All right.
>
> This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
> an example, in device_main.c, we have this code:
>
> rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
>
> Here the extra parameter is the last one, wrq->u.data.pointer.
>
> I was led to believe that wrq->u.data.pointer is in userspace (this was
> reported by sparse actually) because the pointer field in data is
> actually defined as __user.
>
>
By the way, the original code (before my patch) reads:
if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
ret = -EINVAL;
goto out;
}
if (wrq->length > MAX_WPA_IE_LEN) {
ret = -ENOMEM;
goto out;
}
memset(pMgmt->abyWPAIE, 0, MAX_WPA_IE_LEN);
if (copy_from_user(pMgmt->abyWPAIE, extra, wrq->length)) {
ret = -EFAULT;
goto out;
}
Note extra[1] and later copy_from_user(x, extra, y).
If extra is not in userspace, we should not call copy_from_user, and if
it is, we should not dereference it. There is definitely something fishy
here.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer
2014-07-26 9:18 ` Guillaume CLÉMENT
@ 2014-07-26 10:44 ` Malcolm Priestley
2014-07-27 18:21 ` Greg Kroah-Hartman
0 siblings, 1 reply; 12+ messages in thread
From: Malcolm Priestley @ 2014-07-26 10:44 UTC (permalink / raw)
To: Guillaume CLÉMENT
Cc: Forest Bond, Greg Kroah-Hartman, devel, linux-kernel, Dan Carpenter
On 26/07/14 10:18, Guillaume CLÉMENT wrote:
> On Sat, Jul 26, 2014 at 10:24:30AM +0200, Guillaume CLÉMENT wrote:
>> Hi Malcolm,
>>
>> On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
>>> Hi Guillaume
>>>
>>> On 25/07/14 13:47, Guillaume Clement wrote:
>>>> Sparse reported that the data from tagSCmdRequest is given by
>>>> userspace, so it should be tagged as such.
>>> extra is not in user space
>>>
>>
>> All right.
>>
>> This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
>> an example, in device_main.c, we have this code:
>>
>> rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
>>
>> Here the extra parameter is the last one, wrq->u.data.pointer.
>>
>> I was led to believe that wrq->u.data.pointer is in userspace (this was
>> reported by sparse actually) because the pointer field in data is
>> actually defined as __user.
>>
>>
> By the way, the original code (before my patch) reads:
>
> if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> ret = -EINVAL;
> goto out;
> }
> if (wrq->length > MAX_WPA_IE_LEN) {
> ret = -ENOMEM;
> goto out;
> }
> memset(pMgmt->abyWPAIE, 0, MAX_WPA_IE_LEN);
> if (copy_from_user(pMgmt->abyWPAIE, extra, wrq->length)) {
> ret = -EFAULT;
> goto out;
> }
>
> Note extra[1] and later copy_from_user(x, extra, y).
>
> If extra is not in userspace, we should not call copy_from_user, and if
> it is, we should not dereference it. There is definitely something fishy
> here.
>
I got it wrong when the iw_handler is not present a standard ioctl is
called extra is in userspace.
Sorry for the noise.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer
2014-07-26 10:44 ` Malcolm Priestley
@ 2014-07-27 18:21 ` Greg Kroah-Hartman
2014-07-27 18:44 ` Malcolm Priestley
0 siblings, 1 reply; 12+ messages in thread
From: Greg Kroah-Hartman @ 2014-07-27 18:21 UTC (permalink / raw)
To: Malcolm Priestley
Cc: Guillaume CLÉMENT, devel, Forest Bond, Dan Carpenter, linux-kernel
On Sat, Jul 26, 2014 at 11:44:57AM +0100, Malcolm Priestley wrote:
> On 26/07/14 10:18, Guillaume CLÉMENT wrote:
> >On Sat, Jul 26, 2014 at 10:24:30AM +0200, Guillaume CLÉMENT wrote:
> >>Hi Malcolm,
> >>
> >>On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
> >>>Hi Guillaume
> >>>
> >>>On 25/07/14 13:47, Guillaume Clement wrote:
> >>>>Sparse reported that the data from tagSCmdRequest is given by
> >>>>userspace, so it should be tagged as such.
> >>>extra is not in user space
> >>>
> >>
> >>All right.
> >>
> >>This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
> >>an example, in device_main.c, we have this code:
> >>
> >>rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
> >>
> >>Here the extra parameter is the last one, wrq->u.data.pointer.
> >>
> >>I was led to believe that wrq->u.data.pointer is in userspace (this was
> >>reported by sparse actually) because the pointer field in data is
> >>actually defined as __user.
> >>
> >>
> >By the way, the original code (before my patch) reads:
> >
> > if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> > ret = -EINVAL;
> > goto out;
> > }
> > if (wrq->length > MAX_WPA_IE_LEN) {
> > ret = -ENOMEM;
> > goto out;
> > }
> > memset(pMgmt->abyWPAIE, 0, MAX_WPA_IE_LEN);
> > if (copy_from_user(pMgmt->abyWPAIE, extra, wrq->length)) {
> > ret = -EFAULT;
> > goto out;
> > }
> >
> >Note extra[1] and later copy_from_user(x, extra, y).
> >
> >If extra is not in userspace, we should not call copy_from_user, and if
> >it is, we should not dereference it. There is definitely something fishy
> >here.
> >
>
> I got it wrong when the iw_handler is not present a standard ioctl is called
> extra is in userspace.
>
> Sorry for the noise.
So this patch is acceptable?
confused,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer
2014-07-27 18:21 ` Greg Kroah-Hartman
@ 2014-07-27 18:44 ` Malcolm Priestley
0 siblings, 0 replies; 12+ messages in thread
From: Malcolm Priestley @ 2014-07-27 18:44 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Guillaume CLÉMENT, devel, Forest Bond, Dan Carpenter, linux-kernel
On 27/07/14 19:21, Greg Kroah-Hartman wrote:
> On Sat, Jul 26, 2014 at 11:44:57AM +0100, Malcolm Priestley wrote:
>> On 26/07/14 10:18, Guillaume CLÉMENT wrote:
>>> On Sat, Jul 26, 2014 at 10:24:30AM +0200, Guillaume CLÉMENT wrote:
>>>> Hi Malcolm,
>>>>
>>>> On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
>>>>> Hi Guillaume
>>>>>
>>>>> On 25/07/14 13:47, Guillaume Clement wrote:
>>>>>> Sparse reported that the data from tagSCmdRequest is given by
>>>>>> userspace, so it should be tagged as such.
>>>>> extra is not in user space
>>>>>
>>>>
>>>> All right.
>>>>
>>>> This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
>>>> an example, in device_main.c, we have this code:
>>>>
>>>> rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
>>>>
>>>> Here the extra parameter is the last one, wrq->u.data.pointer.
>>>>
>>>> I was led to believe that wrq->u.data.pointer is in userspace (this was
>>>> reported by sparse actually) because the pointer field in data is
>>>> actually defined as __user.
>>>>
>>>>
>>> By the way, the original code (before my patch) reads:
>>>
>>> if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
>>> ret = -EINVAL;
>>> goto out;
>>> }
>>> if (wrq->length > MAX_WPA_IE_LEN) {
>>> ret = -ENOMEM;
>>> goto out;
>>> }
>>> memset(pMgmt->abyWPAIE, 0, MAX_WPA_IE_LEN);
>>> if (copy_from_user(pMgmt->abyWPAIE, extra, wrq->length)) {
>>> ret = -EFAULT;
>>> goto out;
>>> }
>>>
>>> Note extra[1] and later copy_from_user(x, extra, y).
>>>
>>> If extra is not in userspace, we should not call copy_from_user, and if
>>> it is, we should not dereference it. There is definitely something fishy
>>> here.
>>>
>>
>> I got it wrong when the iw_handler is not present a standard ioctl is called
>> extra is in userspace.
>>
>> Sorry for the noise.
>
> So this patch is acceptable?
>
Yes, this patch is okay.
> confused,
>
> greg k-h
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-07-27 18:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-24 23:07 [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest Guillaume Clement
2014-07-25 11:52 ` Dan Carpenter
2014-07-25 12:25 ` Guillaume CLÉMENT
2014-07-25 12:33 ` Dan Carpenter
2014-07-25 12:47 ` [PATCH] staging: vt6655: fix direct dereferencing of user pointer Guillaume Clement
2014-07-25 23:09 ` Malcolm Priestley
2014-07-26 8:24 ` Guillaume CLÉMENT
2014-07-26 9:18 ` Guillaume CLÉMENT
2014-07-26 10:44 ` Malcolm Priestley
2014-07-27 18:21 ` Greg Kroah-Hartman
2014-07-27 18:44 ` Malcolm Priestley
2014-07-25 23:16 ` Malcolm Priestley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.